Re: [PATCH v2 2/2] domain: expose newly introduced hypercalls as XENFEAT

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
> On 06.10.2023 15:00, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1580,6 +1580,10 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >      {
> >          struct vcpu_register_time_memory_area area;
> >  
> > +        rc = -ENOSYS;
> > +        if ( 0 /* TODO: Dom's XENFEAT_vcpu_time_phys_area setting */ )
> > +            break;
> > +
> >          rc = -EFAULT;
> >          if ( copy_from_guest(&area.addr.p, arg, 1) )
> >              break;
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >      {
> >          struct vcpu_register_runstate_memory_area area;
> >  
> > +        rc = -ENOSYS;
> > +        if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ )
> > +            break;
> > +
> >          rc = -EFAULT;
> >          if ( copy_from_guest(&area.addr.p, arg, 1) )
> >              break;
> 
> ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
> correct.

I don't think so, common_vcpu_op() default case does return -ENOSYS,
and the point of this path is to mimic the behavior of an hypervisor
that doesn't have the hypercalls implemented, hence -ENOSYS is the
correct behavior.

> 
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -607,7 +607,11 @@ long do_xen_version(int cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >          switch ( fi.submap_idx )
> >          {
> >          case 0:
> > -            fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
> > +            fi.submap = (1U << XENFEAT_memory_op_vnode_supported) |
> > +#ifdef CONFIG_X86
> > +                        (1U << XENFEAT_vcpu_time_phys_area) |
> > +#endif
> > +                        (1U << XENFEAT_runstate_phys_area);
> 
> No provisions here for the "disabled for this domain" case?

TBH I'm not sure the `if ( 0` above are of much help, if we ever want
to provide toolstack overwrites for those it's fairly easy to spot the
paths that need to be patched anyway.

Thanks, Roger.