Re: [PATCH v3 3/3] xen: mem_access: conditionally compile vm_event.c & monitor.c

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Tue, Mar 11, 2025 at 7:59 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 11.03.2025 11:27, Sergiy Kibrik wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> >
> > Extend coverage of CONFIG_VM_EVENT option and make the build of VM events
> > and monitoring support optional. Also make MEM_PAGING option depend on 
> > VM_EVENT
> > to document that mem_paging is relying on vm_event.
> > This is to reduce code size on Arm when this option isn't enabled.
> >
> > CC: Jan Beulich <jbeulich@xxxxxxxx>
> > CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> > Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > Signed-off-by: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
>
> Please can tags be kept in chronological order? It's impossible to review
> a patch that wasn't first signed-off on by the author(s).
>
> > ---
> > changes in v3:
> >  - add dependency MEM_PAGING -> VM_EVENT

This seems to be largely unnecessary since on x86 selecting HVM
already selects it but I guess it also doesn't hurt to explicitly mark
it like this either.

> >  - monitor_domctl() stub returns -EOPNOTSUPP
>
> Seeing this, i.e. ...
>
> > --- a/xen/include/xen/monitor.h
> > +++ b/xen/include/xen/monitor.h
> > @@ -27,8 +27,17 @@
> >  struct domain;
> >  struct xen_domctl_monitor_op;
> >
> > +#ifdef CONFIG_VM_EVENT
> >  int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop);
> >  void monitor_guest_request(void);
> > +#else
> > +static inline int monitor_domctl(struct domain *d,
> > +                                 struct xen_domctl_monitor_op *mop)
> > +{
> > +    return -EOPNOTSUPP;
>
> ... this, why ...
>
> > @@ -88,7 +85,18 @@ void vm_event_cancel_slot(struct domain *d, struct 
> > vm_event_domain *ved);
> >  void vm_event_put_request(struct domain *d, struct vm_event_domain *ved,
> >                            vm_event_request_t *req);
> >
> > +#ifdef CONFIG_VM_EVENT
> > +/* Clean up on domain destruction */
> > +void vm_event_cleanup(struct domain *d);
> >  int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec);
> > +#else
> > +static inline void vm_event_cleanup(struct domain *d) {}
> > +static inline int vm_event_domctl(struct domain *d,
> > +                                  struct xen_domctl_vm_event_op *vec)
> > +{
> > +    return -EINVAL;
>
> ... is it EINVAL here?

I would prefer if it was consistent too with -EOPNOTSUPP for both when
the subsystems are compiled out.

Thanks,
Tamas