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

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 11/29/23 6:39 AM, Oleksii wrote:
> 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:
>>> 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 ).
>

That approach sounds great to me.

> ~ Oleksii

Thanks,
Shawn