The header is shared between several archs so it is
moved to asm-generic.
Switch partly Arm and PPC to asm-generic/monitor.h and only
arch_monitor_get_capabilities() left in arch-specific/monitor.h.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in V6:
- Rebase only.
---
Changes in V5:
- Switched partly Arm and PPC to asm-generic monitor.h only
arch_monitor_get_capabilities() left in arch-specific/monitor.h.
- Updated the commit message.
---
Changes in V4:
- Removed the double blank line.
- Added Acked-by: Jan Beulich <jbeulich@suse.com>.
- Update the commit message
---
Changes in V3:
- Use forward-declaration of struct domain instead of " #include <xen/sched.h> ".
- Add ' include <xen/errno.h> '
- Drop PPC's monitor.h.
---
Changes in V2:
- remove inclusion of "+#include <public/domctl.h>"
- add "struct xen_domctl_monitor_op;"
- remove one of SPDX tags.
---
xen/arch/arm/include/asm/monitor.h | 28 +--------------
xen/arch/ppc/include/asm/monitor.h | 28 +--------------
xen/include/asm-generic/monitor.h | 57 ++++++++++++++++++++++++++++++
3 files changed, 59 insertions(+), 54 deletions(-)
create mode 100644 xen/include/asm-generic/monitor.h
diff --git a/xen/arch/arm/include/asm/monitor.h b/xen/arch/arm/include/asm/monitor.h
index 7567be66bd..045217c310 100644
--- a/xen/arch/arm/include/asm/monitor.h
+++ b/xen/arch/arm/include/asm/monitor.h
@@ -25,33 +25,7 @@
#include <xen/sched.h>
#include <public/domctl.h>
-static inline
-void arch_monitor_allow_userspace(struct domain *d, bool allow_userspace)
-{
-}
-
-static inline
-int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
-{
- /* No arch-specific monitor ops on ARM. */
- return -EOPNOTSUPP;
-}
-
-int arch_monitor_domctl_event(struct domain *d,
- struct xen_domctl_monitor_op *mop);
-
-static inline
-int arch_monitor_init_domain(struct domain *d)
-{
- /* No arch-specific domain initialization on ARM. */
- return 0;
-}
-
-static inline
-void arch_monitor_cleanup_domain(struct domain *d)
-{
- /* No arch-specific domain cleanup on ARM. */
-}
+#include <asm-generic/monitor.h>
static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
{
diff --git a/xen/arch/ppc/include/asm/monitor.h b/xen/arch/ppc/include/asm/monitor.h
index e5b0282bf1..89000dacc6 100644
--- a/xen/arch/ppc/include/asm/monitor.h
+++ b/xen/arch/ppc/include/asm/monitor.h
@@ -6,33 +6,7 @@
#include <public/domctl.h>
#include <xen/errno.h>
-static inline
-void arch_monitor_allow_userspace(struct domain *d, bool allow_userspace)
-{
-}
-
-static inline
-int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
-{
- /* No arch-specific monitor ops on PPC. */
- return -EOPNOTSUPP;
-}
-
-int arch_monitor_domctl_event(struct domain *d,
- struct xen_domctl_monitor_op *mop);
-
-static inline
-int arch_monitor_init_domain(struct domain *d)
-{
- /* No arch-specific domain initialization on PPC. */
- return 0;
-}
-
-static inline
-void arch_monitor_cleanup_domain(struct domain *d)
-{
- /* No arch-specific domain cleanup on PPC. */
-}
+#include <asm-generic/monitor.h>
static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
{
diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-generic/monitor.h
new file mode 100644
index 0000000000..74e4870cd7
--- /dev/null
+++ b/xen/include/asm-generic/monitor.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * include/asm-generic/monitor.h
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
+ * Copyright (c) 2016, Bitdefender S.R.L.
+ *
+ */
+
+#ifndef __ASM_GENERIC_MONITOR_H__
+#define __ASM_GENERIC_MONITOR_H__
+
+#include <xen/errno.h>
+
+struct domain;
+struct xen_domctl_monitor_op;
+
+static inline
+void arch_monitor_allow_userspace(struct domain *d, bool allow_userspace)
+{
+}
+
+static inline
+int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
+{
+ /* No arch-specific monitor ops on GENERIC. */
+ return -EOPNOTSUPP;
+}
+
+int arch_monitor_domctl_event(struct domain *d,
+ struct xen_domctl_monitor_op *mop);
+
+static inline
+int arch_monitor_init_domain(struct domain *d)
+{
+ /* No arch-specific domain initialization on GENERIC. */
+ return 0;
+}
+
+static inline
+void arch_monitor_cleanup_domain(struct domain *d)
+{
+ /* No arch-specific domain cleanup on GENERIC. */
+}
+
+#endif /* __ASM_GENERIC_MONITOR_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: BSD
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.43.0On 20/12/2023 2:08 pm, Oleksii Kurochko wrote:
> diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-generic/monitor.h
> new file mode 100644
> index 0000000000..74e4870cd7
> --- /dev/null
> +++ b/xen/include/asm-generic/monitor.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * include/asm-generic/monitor.h
> + *
> + * Arch-specific monitor_op domctl handler.
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> + *
> + */
> +
> +#ifndef __ASM_GENERIC_MONITOR_H__
> +#define __ASM_GENERIC_MONITOR_H__
> +
> +#include <xen/errno.h>
> +
> +struct domain;
> +struct xen_domctl_monitor_op;
> +
> +static inline
> +void arch_monitor_allow_userspace(struct domain *d, bool allow_userspace)
> +{
> +}
> +
> +static inline
> +int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
> +{
> + /* No arch-specific monitor ops on GENERIC. */
> + return -EOPNOTSUPP;
> +}
> +
> +int arch_monitor_domctl_event(struct domain *d,
> + struct xen_domctl_monitor_op *mop);
Turn this into a static inline like the others, and you can delete:
arch/ppc/stubs.c:100
int arch_monitor_domctl_event(struct domain *d,
struct xen_domctl_monitor_op *mop)
{
BUG_ON("unimplemented");
}
because new architectures shouldn't have to stub one random piece of a
subsystem when using the generic "nothing special" header.
Given the filtering for arch_monitor_domctl_op(), this one probably
wants to be ASSERT_UNREACHABLE(); return 0.
~Andrew
On Wed, 2023-12-20 at 16:33 +0000, Andrew Cooper wrote:
> On 20/12/2023 2:08 pm, Oleksii Kurochko wrote:
> > diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-
> > generic/monitor.h
> > new file mode 100644
> > index 0000000000..74e4870cd7
> > --- /dev/null
> > +++ b/xen/include/asm-generic/monitor.h
> > @@ -0,0 +1,57 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * include/asm-generic/monitor.h
> > + *
> > + * Arch-specific monitor_op domctl handler.
> > + *
> > + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> > + * Copyright (c) 2016, Bitdefender S.R.L.
> > + *
> > + */
> > +
> > +#ifndef __ASM_GENERIC_MONITOR_H__
> > +#define __ASM_GENERIC_MONITOR_H__
> > +
> > +#include <xen/errno.h>
> > +
> > +struct domain;
> > +struct xen_domctl_monitor_op;
> > +
> > +static inline
> > +void arch_monitor_allow_userspace(struct domain *d, bool
> > allow_userspace)
> > +{
> > +}
> > +
> > +static inline
> > +int arch_monitor_domctl_op(struct domain *d, struct
> > xen_domctl_monitor_op *mop)
> > +{
> > + /* No arch-specific monitor ops on GENERIC. */
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +int arch_monitor_domctl_event(struct domain *d,
> > + struct xen_domctl_monitor_op *mop);
>
> Turn this into a static inline like the others, and you can delete:
>
> arch/ppc/stubs.c:100
>
> int arch_monitor_domctl_event(struct domain *d,
> struct xen_domctl_monitor_op *mop)
> {
> BUG_ON("unimplemented");
> }
>
> because new architectures shouldn't have to stub one random piece of
> a
> subsystem when using the generic "nothing special" header.
>
> Given the filtering for arch_monitor_domctl_op(), this one probably
> wants to be ASSERT_UNREACHABLE(); return 0.
What you wrote makes sense. However, doing it that way may limit the
reuse of other parts of the asm-generic header. It would require
introducing an architecture-specific monitor.h header, which would be
nearly identical.
For instance, at present, the only difference between Arm, PPC, and
RISC-V is arch_monitor_domctl_event(). If this function is implemented
with BUG_ON("unimplemented"), reusing the asm-generic monitor.h header
for Arm (as it is partly done now) becomes challenging.
To address this, I propose wrapping arch_monitor_domctl_event() in
#ifdef:
#ifndef HAS_ARCH_MONITOR_DOMCTL_EVENT
int arch_monitor_domctl_event(struct domain *d,
struct xen_domctl_monitor_op *mop)
{
BUG_ON("unimplemented");
}
#endif
In the architecture-specific monitor.h, you would define
HAS_ARCH_MONITOR_DOMCTL_EVENT and provide the architecture-specific
implementation of the function. For example, in the case of Arm:
#ifndef __ASM_ARM_MONITOR_H__
#define __ASM_ARM_MONITOR_H__
#include <xen/sched.h>
#include <public/domctl.h>
#define HAS_ARCH_MONITOR_DOMCTL_EVENT
#include <asm-generic/monitor.h>
static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
{
uint32_t capabilities = 0;
capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST |
1U << XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL);
return capabilities;
}
int monitor_smc(void);
#endif /* __ASM_ARM_MONITOR_H__ */
This approach maintains a clean and modular structure, allowing for
architecture-specific variations while reusing the majority of the code
from the generic header.
Does it make sense?
~ Oleksii
On 22.12.2023 14:02, Oleksii wrote:
> On Wed, 2023-12-20 at 16:33 +0000, Andrew Cooper wrote:
>> On 20/12/2023 2:08 pm, Oleksii Kurochko wrote:
>>> diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-
>>> generic/monitor.h
>>> new file mode 100644
>>> index 0000000000..74e4870cd7
>>> --- /dev/null
>>> +++ b/xen/include/asm-generic/monitor.h
>>> @@ -0,0 +1,57 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * include/asm-generic/monitor.h
>>> + *
>>> + * Arch-specific monitor_op domctl handler.
>>> + *
>>> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
>>> + * Copyright (c) 2016, Bitdefender S.R.L.
>>> + *
>>> + */
>>> +
>>> +#ifndef __ASM_GENERIC_MONITOR_H__
>>> +#define __ASM_GENERIC_MONITOR_H__
>>> +
>>> +#include <xen/errno.h>
>>> +
>>> +struct domain;
>>> +struct xen_domctl_monitor_op;
>>> +
>>> +static inline
>>> +void arch_monitor_allow_userspace(struct domain *d, bool
>>> allow_userspace)
>>> +{
>>> +}
>>> +
>>> +static inline
>>> +int arch_monitor_domctl_op(struct domain *d, struct
>>> xen_domctl_monitor_op *mop)
>>> +{
>>> + /* No arch-specific monitor ops on GENERIC. */
>>> + return -EOPNOTSUPP;
>>> +}
>>> +
>>> +int arch_monitor_domctl_event(struct domain *d,
>>> + struct xen_domctl_monitor_op *mop);
>>
>> Turn this into a static inline like the others, and you can delete:
>>
>> arch/ppc/stubs.c:100
>>
>> int arch_monitor_domctl_event(struct domain *d,
>> struct xen_domctl_monitor_op *mop)
>> {
>> BUG_ON("unimplemented");
>> }
>>
>> because new architectures shouldn't have to stub one random piece of
>> a
>> subsystem when using the generic "nothing special" header.
>>
>> Given the filtering for arch_monitor_domctl_op(), this one probably
>> wants to be ASSERT_UNREACHABLE(); return 0.
> What you wrote makes sense. However, doing it that way may limit the
> reuse of other parts of the asm-generic header. It would require
> introducing an architecture-specific monitor.h header, which would be
> nearly identical.
>
> For instance, at present, the only difference between Arm, PPC, and
> RISC-V is arch_monitor_domctl_event(). If this function is implemented
> with BUG_ON("unimplemented"), reusing the asm-generic monitor.h header
> for Arm (as it is partly done now) becomes challenging.
>
> To address this, I propose wrapping arch_monitor_domctl_event() in
> #ifdef:
>
> #ifndef HAS_ARCH_MONITOR_DOMCTL_EVENT
> int arch_monitor_domctl_event(struct domain *d,
> struct xen_domctl_monitor_op *mop)
> {
> BUG_ON("unimplemented");
> }
> #endif
>
> In the architecture-specific monitor.h, you would define
> HAS_ARCH_MONITOR_DOMCTL_EVENT and provide the architecture-specific
> implementation of the function. For example, in the case of Arm:
>
> #ifndef __ASM_ARM_MONITOR_H__
> #define __ASM_ARM_MONITOR_H__
>
> #include <xen/sched.h>
> #include <public/domctl.h>
>
> #define HAS_ARCH_MONITOR_DOMCTL_EVENT
>
> #include <asm-generic/monitor.h>
>
> static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
> {
> uint32_t capabilities = 0;
>
> capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST |
> 1U << XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL);
>
> return capabilities;
> }
>
> int monitor_smc(void);
>
> #endif /* __ASM_ARM_MONITOR_H__ */
>
> This approach maintains a clean and modular structure, allowing for
> architecture-specific variations while reusing the majority of the code
> from the generic header.
>
> Does it make sense?
With the state things are in right now in the tree, perhaps yes. But
as with NUMA and other subsystems: Generally the case of the subsystem
not used should be handled in common code. What's in asm-generic/ is
supposed to be a default implementation when the subsystem _is_ used.
Unlike NUMA, there's no Kconfig control for MONITOR (or VM_EVENT).
Hence why getting this sorted is somewhat more involved here; (ab)using
the asm-generic/ header for the time being is an option, but would then
need properly justifying (imo).
Jan
It is necessary to remove unnecessary inclusions of headers in
arch/*/asm/monitor.h. This was overlooked.
Sorry for the inconvenience.
~ Oleksii
On Wed, 2023-12-20 at 16:08 +0200, Oleksii Kurochko wrote:
> The header is shared between several archs so it is
> moved to asm-generic.
>
> Switch partly Arm and PPC to asm-generic/monitor.h and only
> arch_monitor_get_capabilities() left in arch-specific/monitor.h.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
> Changes in V6:
> - Rebase only.
> ---
> Changes in V5:
> - Switched partly Arm and PPC to asm-generic monitor.h only
> arch_monitor_get_capabilities() left in arch-specific/monitor.h.
> - Updated the commit message.
> ---
> Changes in V4:
> - Removed the double blank line.
> - Added Acked-by: Jan Beulich <jbeulich@suse.com>.
> - Update the commit message
> ---
> Changes in V3:
> - Use forward-declaration of struct domain instead of " #include
> <xen/sched.h> ".
> - Add ' include <xen/errno.h> '
> - Drop PPC's monitor.h.
> ---
> Changes in V2:
> - remove inclusion of "+#include <public/domctl.h>"
> - add "struct xen_domctl_monitor_op;"
> - remove one of SPDX tags.
> ---
> xen/arch/arm/include/asm/monitor.h | 28 +--------------
> xen/arch/ppc/include/asm/monitor.h | 28 +--------------
> xen/include/asm-generic/monitor.h | 57
> ++++++++++++++++++++++++++++++
> 3 files changed, 59 insertions(+), 54 deletions(-)
> create mode 100644 xen/include/asm-generic/monitor.h
>
> diff --git a/xen/arch/arm/include/asm/monitor.h
> b/xen/arch/arm/include/asm/monitor.h
> index 7567be66bd..045217c310 100644
> --- a/xen/arch/arm/include/asm/monitor.h
> +++ b/xen/arch/arm/include/asm/monitor.h
> @@ -25,33 +25,7 @@
> #include <xen/sched.h>
> #include <public/domctl.h>
>
> -static inline
> -void arch_monitor_allow_userspace(struct domain *d, bool
> allow_userspace)
> -{
> -}
> -
> -static inline
> -int arch_monitor_domctl_op(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> -{
> - /* No arch-specific monitor ops on ARM. */
> - return -EOPNOTSUPP;
> -}
> -
> -int arch_monitor_domctl_event(struct domain *d,
> - struct xen_domctl_monitor_op *mop);
> -
> -static inline
> -int arch_monitor_init_domain(struct domain *d)
> -{
> - /* No arch-specific domain initialization on ARM. */
> - return 0;
> -}
> -
> -static inline
> -void arch_monitor_cleanup_domain(struct domain *d)
> -{
> - /* No arch-specific domain cleanup on ARM. */
> -}
> +#include <asm-generic/monitor.h>
>
> static inline uint32_t arch_monitor_get_capabilities(struct domain
> *d)
> {
> diff --git a/xen/arch/ppc/include/asm/monitor.h
> b/xen/arch/ppc/include/asm/monitor.h
> index e5b0282bf1..89000dacc6 100644
> --- a/xen/arch/ppc/include/asm/monitor.h
> +++ b/xen/arch/ppc/include/asm/monitor.h
> @@ -6,33 +6,7 @@
> #include <public/domctl.h>
> #include <xen/errno.h>
>
> -static inline
> -void arch_monitor_allow_userspace(struct domain *d, bool
> allow_userspace)
> -{
> -}
> -
> -static inline
> -int arch_monitor_domctl_op(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> -{
> - /* No arch-specific monitor ops on PPC. */
> - return -EOPNOTSUPP;
> -}
> -
> -int arch_monitor_domctl_event(struct domain *d,
> - struct xen_domctl_monitor_op *mop);
> -
> -static inline
> -int arch_monitor_init_domain(struct domain *d)
> -{
> - /* No arch-specific domain initialization on PPC. */
> - return 0;
> -}
> -
> -static inline
> -void arch_monitor_cleanup_domain(struct domain *d)
> -{
> - /* No arch-specific domain cleanup on PPC. */
> -}
> +#include <asm-generic/monitor.h>
>
> static inline uint32_t arch_monitor_get_capabilities(struct domain
> *d)
> {
> diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-
> generic/monitor.h
> new file mode 100644
> index 0000000000..74e4870cd7
> --- /dev/null
> +++ b/xen/include/asm-generic/monitor.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * include/asm-generic/monitor.h
> + *
> + * Arch-specific monitor_op domctl handler.
> + *
> + * Copyright (c) 2015 Tamas K Lengyel (tamas@tklengyel.com)
> + * Copyright (c) 2016, Bitdefender S.R.L.
> + *
> + */
> +
> +#ifndef __ASM_GENERIC_MONITOR_H__
> +#define __ASM_GENERIC_MONITOR_H__
> +
> +#include <xen/errno.h>
> +
> +struct domain;
> +struct xen_domctl_monitor_op;
> +
> +static inline
> +void arch_monitor_allow_userspace(struct domain *d, bool
> allow_userspace)
> +{
> +}
> +
> +static inline
> +int arch_monitor_domctl_op(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> +{
> + /* No arch-specific monitor ops on GENERIC. */
> + return -EOPNOTSUPP;
> +}
> +
> +int arch_monitor_domctl_event(struct domain *d,
> + struct xen_domctl_monitor_op *mop);
> +
> +static inline
> +int arch_monitor_init_domain(struct domain *d)
> +{
> + /* No arch-specific domain initialization on GENERIC. */
> + return 0;
> +}
> +
> +static inline
> +void arch_monitor_cleanup_domain(struct domain *d)
> +{
> + /* No arch-specific domain cleanup on GENERIC. */
> +}
> +
> +#endif /* __ASM_GENERIC_MONITOR_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: BSD
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
© 2016 - 2026 Red Hat, Inc.