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.
