Re: [PATCH v4 10/14] xen/asm-generic: introduce stub header monitor.h

[Shawn Anastasio <sanastasio@raptorengineering.com>]

Hi Oleksii,

On 11/27/23 8:13 AM, Oleksii Kurochko wrote:
> The header is shared between several archs so it is
> moved to asm-generic.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>.
> ---
> 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/ppc/include/asm/Makefile  |  1 +
>  xen/arch/ppc/include/asm/monitor.h | 43 ---------------------
>  xen/include/asm-generic/monitor.h  | 62 ++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 43 deletions(-)
>  delete mode 100644 xen/arch/ppc/include/asm/monitor.h
>  create mode 100644 xen/include/asm-generic/monitor.h
> 
> diff --git a/xen/arch/ppc/include/asm/Makefile b/xen/arch/ppc/include/asm/Makefile
> index 319e90955b..bcddcc181a 100644
> --- a/xen/arch/ppc/include/asm/Makefile
> +++ b/xen/arch/ppc/include/asm/Makefile
> @@ -5,6 +5,7 @@ generic-y += div64.h
>  generic-y += hardirq.h
>  generic-y += hypercall.h
>  generic-y += iocap.h
> +generic-y += monitor.h
>  generic-y += paging.h
>  generic-y += percpu.h
>  generic-y += random.h
> diff --git a/xen/arch/ppc/include/asm/monitor.h b/xen/arch/ppc/include/asm/monitor.h
> deleted file mode 100644
> index e5b0282bf1..0000000000
> --- a/xen/arch/ppc/include/asm/monitor.h
> +++ /dev/null
> @@ -1,43 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/* Derived from xen/arch/arm/include/asm/monitor.h */
> -#ifndef __ASM_PPC_MONITOR_H__
> -#define __ASM_PPC_MONITOR_H__
> -
> -#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. */
> -}
> -
> -static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
> -{
> -    BUG_ON("unimplemented");

I'm not sure how I feel about this assertion being dropped in the
generic header. In general my philosophy when creating these stub
headers was to insert as many of these assertions as possible so we
don't end up in a scenario where during early bringup I miss a function
that was originally stubbed but ought to be implemented eventually.

Looking at ARM's monitor.h too, it seems that this function is the only
one that differs from the generic/stub version. I'm wondering if it
would make sense to leave this function out of the generic header, to be
defined by the arch similar to what you've done with some other
functions in this series. That would also allow ARM to be brought in to
using the generic variant.

> -    return 0;
> -}
> -
> -#endif /* __ASM_PPC_MONITOR_H__ */
> diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-generic/monitor.h
> new file mode 100644
> index 0000000000..6be8614431
> --- /dev/null
> +++ b/xen/include/asm-generic/monitor.h
> @@ -0,0 +1,62 @@
> +/* 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. */
> +}
> +
> +static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
> +{

See previous comment.

> +    return 0;
> +}
> +
> +#endif /* __ASM_GENERIC_MONITOR_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: BSD
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Thanks,
Shawn