Re: [PATCH v5 3/3] x86/altp2m: Add a hvmop for querying the suppress #VE bit

[George Dunlap <george.dunlap@citrix.com>]

On 09/03/2018 04:48 PM, Adrian Pop wrote:
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---
>  tools/libxc/include/xenctrl.h   |  2 ++
>  tools/libxc/xc_altp2m.c         | 26 +++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c          | 19 ++++++++++++++
>  xen/arch/x86/mm/mem_access.c    | 45 +++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/hvm_op.h |  2 ++
>  xen/include/xen/mem_access.h    |  3 +++
>  6 files changed, 97 insertions(+)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index cc8b3e7dce..59955f0357 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1954,6 +1954,8 @@ int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
>                               uint16_t view_id);
>  int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
>                                uint16_t view_id, xen_pfn_t gfn, bool sve);
> +int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_t domid,
> +                              uint16_t view_id, xen_pfn_t gfn, bool *sve);
>  int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
>                               uint16_t view_id, xen_pfn_t gfn,
>                               xenmem_access_t access);
> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
> index f883d0b392..1c9b572e2b 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -163,6 +163,32 @@ int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
>      return rc;
>  }
>  
> +int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_t domid,
> +                              uint16_t view_id, xen_pfn_t gfn, bool *sve)
> +{
> +    int rc;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +    if ( arg == NULL )
> +        return -1;
> +
> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
> +    arg->cmd = HVMOP_altp2m_get_suppress_ve;
> +    arg->domain = domid;
> +    arg->u.suppress_ve.view = view_id;
> +    arg->u.suppress_ve.gfn = gfn;
> +
> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> +                  HYPERCALL_BUFFER_AS_ARG(arg));
> +
> +    if ( !rc )
> +        *sve = arg->u.suppress_ve.suppress_ve;
> +
> +    xc_hypercall_buffer_free(handle, arg);
> +    return rc;
> +}
> +
>  int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
>                                uint16_t view_id, xen_pfn_t gfn, bool sve)
>  {
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 64ab36ff53..6f6efb0d8a 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4525,6 +4525,7 @@ static int do_altp2m_op(
>      case HVMOP_altp2m_destroy_p2m:
>      case HVMOP_altp2m_switch_p2m:
>      case HVMOP_altp2m_set_suppress_ve:
> +    case HVMOP_altp2m_get_suppress_ve:
>      case HVMOP_altp2m_set_mem_access:
>      case HVMOP_altp2m_set_mem_access_multi:
>      case HVMOP_altp2m_change_gfn:
> @@ -4655,6 +4656,24 @@ static int do_altp2m_op(
>          }
>          break;
>  
> +    case HVMOP_altp2m_get_suppress_ve:
> +        if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
> +            rc = -EINVAL;
> +        else
> +        {
> +            gfn_t gfn = _gfn(a.u.suppress_ve.gfn);
> +            unsigned int altp2m_idx = a.u.suppress_ve.view;
> +            bool suppress_ve;
> +
> +            rc = p2m_get_suppress_ve(d, gfn, &suppress_ve, altp2m_idx);
> +            if ( !rc )
> +            {
> +                a.u.suppress_ve.suppress_ve = suppress_ve;
> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +            }
> +        }
> +        break;
> +
>      case HVMOP_altp2m_set_mem_access:
>          if ( a.u.set_mem_access.pad )
>              rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 4d49025cbe..df78c93cfd 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -550,6 +550,51 @@ out:
>      return rc;
>  }
>  
> +int p2m_get_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;
> +
> +    if ( !cpu_has_vmx_virt_exceptions )
> +        return -EOPNOTSUPP;
> +
> +    /* #VE should be enabled for this vcpu. */
> +    if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
> +        return -ENXIO;

Basically the same comments as for 2/3:  Move to p2m.c, and get rid of
the vmx-ism.

Anothre idea is to get rid of these checks altogether -- returning
'false' when the feature isn't supported or enabled shouldn't be a big
deal.  But I don't feel strongly enough about it to argue either way.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel