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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> --- 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?

> +    /* #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.

> --- 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).

> +};
> +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.

Jan


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