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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Shawn,

On Tue, 2023-11-28 at 16:21 -0600, Shawn Anastasio wrote:
> 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@xxxxxxxxx>
> > Acked-by: Jan Beulich <jbeulich@xxxxxxxx>.
> > ---
> > Changes in V4:
> >  - Removed the double blank line.
> >  - Added Acked-by: Jan Beulich <jbeulich@xxxxxxxx>.
> >  - 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.
Thanks for the comment.

For RISC-V, I did in the same way ( about BUG() and unimplemented for
time being functions ).

I agree that such implementation isn't correct for generic header. I
think the best one solution will be to include <asm-generic/monitor.h>
in <asm/monitor.h> whwere only this function will be implemented (
because implementation of other functions are the same for Arm, PPC and
RISC-V ).

> 
> > -    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@xxxxxxxxxxxxx)
> > + * 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:
> > + */

~ Oleksii