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 (ta...@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
