Re: [PATCH v6 4/9] xen/asm-generic: introduce stub header monitor.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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@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);
> 
> 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