| Message ID | 49c8e0f5997063ed65c3b135c0003ca9570c0bc6.1703072575.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce generic headers | expand |
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: > + */
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. ~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
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: + */