Re: [Xen-devel] [PATCH 2/2] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Adrian Pop <apop@xxxxxxxxxxxxxxx>]

On Fri, Jun 16, 2017 at 09:29:49AM -0600, Jan Beulich wrote:
> >>> On 09.06.17 at 18:51, <apop@xxxxxxxxxxxxxxx> wrote:
> > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> > privileged domain to change the value of the #VE suppress bit for a
> > page.
> > 
> > Add a libxc wrapper for invoking this hvmop.
> > 
> > Signed-off-by: Adrian Pop <apop@xxxxxxxxxxxxxxx>
> > ---
> 
> Please properly version your patch submissions, and please put
> here a brief summary of what changed from the previous version.
 
OK.  I've mistakenly sent the mail without setting the patch version.
I've written the change list in the cover letter, but in hindsight it
would have been a better idea to add the list of changes per patch
instead.

> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, 
> > xenmem_access_t *access)
> >  }
> >  
> >  /*
> > + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> > + */
> > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
> > +                        unsigned int altp2m_idx)
> > +{
> > +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> > +    struct p2m_domain *ap2m = NULL;
> > +    struct p2m_domain *p2m;
> > +    mfn_t mfn;
> > +    p2m_access_t a;
> > +    p2m_type_t t;
> > +    int rc;
> > +
> > +    if ( !cpu_has_vmx_virt_exceptions )
> > +        return -EOPNOTSUPP;
> > +
> > +    /* This subop should only be used from a privileged domain. */
> > +    if ( !current->domain->is_privileged )
> > +        return -EINVAL;
> 
> Beyond the question of what check to use, perhaps -EPERM?
 
OK.

> > +    /* #VE should be enabled for this vcpu. */
> > +    if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
> > +        return -EINVAL;
> 
> This also doesn't really is an invalid argument error - perhaps e.g.
> -ENXIO or -ENOENT? Be creative, but don't use -EINVAL for
> everything.

All right.

> > --- a/xen/include/public/hvm/hvm_op.h
> > +++ b/xen/include/public/hvm/hvm_op.h
> > @@ -237,6 +237,18 @@ struct xen_hvm_altp2m_set_mem_access {
> >  typedef struct xen_hvm_altp2m_set_mem_access 
> > xen_hvm_altp2m_set_mem_access_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
> >  
> > +struct xen_hvm_altp2m_set_suppress_ve {
> > +    /* view */
> > +    uint16_t view;
> > +    uint8_t suppress_ve;
> > +    uint8_t pad1;
> > +    uint32_t pad2;
> > +    /* gfn */
> > +    uint64_t gfn;
> 
> Commenting fields with their field names is, I'm sorry, rather pointless.
> What gfn means is most likely clear without comment. For view I'm not
> sure (depends on conventions elsewhere), but the boolean nature of
> suppress_ve clearly wants commenting on (especially also to clarify
> behavior of values other than 0 and 1).

OK then.

> > +};
> > +typedef struct xen_hvm_altp2m_set_suppress_ve 
> > xen_hvm_altp2m_set_suppress_ve_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t);
> 
> I think we should stop the habit of creating such typedefs and handles
> when ...
> 
> > @@ -276,6 +290,7 @@ struct xen_hvm_altp2m_op {
> >          struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
> >          struct xen_hvm_altp2m_view               view;
> >          struct xen_hvm_altp2m_set_mem_access     set_mem_access;
> > +        struct xen_hvm_altp2m_set_suppress_ve    set_suppress_ve;
> 
> ... a structure isn't meant to be used on its own anyway.
 
Yes, I agree with that.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel