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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2023-11-29 at 09:21 +0100, Jan Beulich wrote:
> On 29.11.2023 09:19, Jan Beulich wrote:
> > On 28.11.2023 23:21, Shawn Anastasio wrote:
> > > On 11/27/23 8:13 AM, Oleksii Kurochko wrote:
> > > > --- 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.
> > 
> > Yet then where would that function live, if not in
> > arch/*/include/asm/monitor.h?
> 
> Hmm, maybe implicitly you're proposing that
> arch/*/include/asm/monitor.h
> include include/asm-generic/monitor.h in such a case, and define this
> one
> function on top?
I think it can be a solution. The same I suggest in my direct reply to
Shawn message ( I didn't see your answer ).

If everyone is OK with such solution, I can apply it for the next
version of patch series.

~ Oleksii