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

Commit Message

Oleksii Nov. 27, 2023, 2:13 p.m. UTC

The header is shared between several archs so it is
moved to asm-generic.

Signed-off-by: Oleksii Kurochko <[e-mail]>
Acked-by: Jan Beulich <[e-mail]>.
---
Changes in V4:
 - Removed the double blank line.
 - Added Acked-by: Jan Beulich <[e-mail]>.
 - Update the commit message
---
Changes in V3:
 - Use forward-declaration of struct domain instead of " #include <xen/sched.h> ".
 - Add ' include <xen/errno.h> '
 - Drop PPC's monitor.h.
---
Changes in V2:
	- remove inclusion of "+#include <public/domctl.h>"
	- add "struct xen_domctl_monitor_op;"
	- remove one of SPDX tags.
---
 xen/arch/ppc/include/asm/Makefile  |  1 +
 xen/arch/ppc/include/asm/monitor.h | 43 ---------------------
 xen/include/asm-generic/monitor.h  | 62 ++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 43 deletions(-)
 delete mode 100644 xen/arch/ppc/include/asm/monitor.h
 create mode 100644 xen/include/asm-generic/monitor.h

Comments

Shawn Anastasio Nov. 28, 2023, 10:21 p.m. UTC | #1

Hi Oleksii,

On 11/27/23 8:13 AM, Oleksii Kurochko wrote:
> The header is shared between several archs so it is
> moved to asm-generic.
> 
> Signed-off-by: Oleksii Kurochko <[e-mail]>
> Acked-by: Jan Beulich <[e-mail]>.
> ---
> Changes in V4:
>  - Removed the double blank line.
>  - Added Acked-by: Jan Beulich <[e-mail]>.
>  - Update the commit message
> ---
> Changes in V3:
>  - Use forward-declaration of struct domain instead of " #include <xen/sched.h> ".
>  - Add ' include <xen/errno.h> '
>  - Drop PPC's monitor.h.
> ---
> Changes in V2:
> 	- remove inclusion of "+#include <public/domctl.h>"
> 	- add "struct xen_domctl_monitor_op;"
> 	- remove one of SPDX tags.
> ---
>  xen/arch/ppc/include/asm/Makefile  |  1 +
>  xen/arch/ppc/include/asm/monitor.h | 43 ---------------------
>  xen/include/asm-generic/monitor.h  | 62 ++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 43 deletions(-)
>  delete mode 100644 xen/arch/ppc/include/asm/monitor.h
>  create mode 100644 xen/include/asm-generic/monitor.h
> 
> 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.

> -    return 0;
> -}
> -
> -#endif /* __ASM_PPC_MONITOR_H__ */
> diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-generic/monitor.h
> new file mode 100644
> index 0000000000..6be8614431
> --- /dev/null
> +++ b/xen/include/asm-generic/monitor.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * include/asm-GENERIC/monitor.h
> + *
> + * Arch-specific monitor_op domctl handler.
> + *
> + * Copyright (c) 2015 Tamas K Lengyel ([e-mail])
> + * 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);
> +
> +static inline
> +int arch_monitor_init_domain(struct domain *d)
> +{
> +    /* No arch-specific domain initialization on GENERIC. */
> +    return 0;
> +}
> +
> +static inline
> +void arch_monitor_cleanup_domain(struct domain *d)
> +{
> +    /* No arch-specific domain cleanup on GENERIC. */
> +}
> +
> +static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
> +{

See previous comment.

> +    return 0;
> +}
> +
> +#endif /* __ASM_GENERIC_MONITOR_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: BSD
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Thanks,
Shawn

Jan Beulich Nov. 29, 2023, 8:19 a.m. UTC | #2

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?

Jan

Jan Beulich Nov. 29, 2023, 8:21 a.m. UTC | #3

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?

Jan

Oleksii Nov. 29, 2023, 12:39 p.m. UTC | #4

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:
> > The header is shared between several archs so it is
> > moved to asm-generic.
> > 
> > Signed-off-by: Oleksii Kurochko <[e-mail]>
> > Acked-by: Jan Beulich <[e-mail]>.
> > ---
> > Changes in V4:
> >  - Removed the double blank line.
> >  - Added Acked-by: Jan Beulich <[e-mail]>.
> >  - Update the commit message
> > ---
> > Changes in V3:
> >  - Use forward-declaration of struct domain instead of " #include
> > <xen/sched.h> ".
> >  - Add ' include <xen/errno.h> '
> >  - Drop PPC's monitor.h.
> > ---
> > Changes in V2:
> > 	- remove inclusion of "+#include <public/domctl.h>"
> > 	- add "struct xen_domctl_monitor_op;"
> > 	- remove one of SPDX tags.
> > ---
> >  xen/arch/ppc/include/asm/Makefile  |  1 +
> >  xen/arch/ppc/include/asm/monitor.h | 43 ---------------------
> >  xen/include/asm-generic/monitor.h  | 62
> > ++++++++++++++++++++++++++++++
> >  3 files changed, 63 insertions(+), 43 deletions(-)
> >  delete mode 100644 xen/arch/ppc/include/asm/monitor.h
> >  create mode 100644 xen/include/asm-generic/monitor.h
> > 
> > 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 ).

> 
> > -    return 0;
> > -}
> > -
> > -#endif /* __ASM_PPC_MONITOR_H__ */
> > diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-
> > generic/monitor.h
> > new file mode 100644
> > index 0000000000..6be8614431
> > --- /dev/null
> > +++ b/xen/include/asm-generic/monitor.h
> > @@ -0,0 +1,62 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * include/asm-GENERIC/monitor.h
> > + *
> > + * Arch-specific monitor_op domctl handler.
> > + *
> > + * Copyright (c) 2015 Tamas K Lengyel ([e-mail])
> > + * 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);
> > +
> > +static inline
> > +int arch_monitor_init_domain(struct domain *d)
> > +{
> > +    /* No arch-specific domain initialization on GENERIC. */
> > +    return 0;
> > +}
> > +
> > +static inline
> > +void arch_monitor_cleanup_domain(struct domain *d)
> > +{
> > +    /* No arch-specific domain cleanup on GENERIC. */
> > +}
> > +
> > +static inline uint32_t arch_monitor_get_capabilities(struct domain
> > *d)
> > +{
> 
> See previous comment.
> 
> > +    return 0;
> > +}
> > +
> > +#endif /* __ASM_GENERIC_MONITOR_H__ */
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: BSD
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */

~ Oleksii

Oleksii Nov. 29, 2023, 12:41 p.m. UTC | #5

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

Shawn Anastasio Nov. 29, 2023, 7:27 p.m. UTC | #6

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

Patch

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");
-    return 0;
-}
-
-#endif /* __ASM_PPC_MONITOR_H__ */
diff --git a/xen/include/asm-generic/monitor.h b/xen/include/asm-generic/monitor.h
new file mode 100644
index 0000000000..6be8614431
--- /dev/null
+++ b/xen/include/asm-generic/monitor.h
@@ -0,0 +1,62 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * include/asm-GENERIC/monitor.h
+ *
+ * Arch-specific monitor_op domctl handler.
+ *
+ * Copyright (c) 2015 Tamas K Lengyel ([e-mail])
+ * 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);
+
+static inline
+int arch_monitor_init_domain(struct domain *d)
+{
+    /* No arch-specific domain initialization on GENERIC. */
+    return 0;
+}
+
+static inline
+void arch_monitor_cleanup_domain(struct domain *d)
+{
+    /* No arch-specific domain cleanup on GENERIC. */
+}
+
+static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
+{
+    return 0;
+}
+
+#endif /* __ASM_GENERIC_MONITOR_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: BSD
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */