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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@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?

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