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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <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>
> ---
>  tools/libxc/include/xenctrl.h   |  2 ++
>  tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
>  xen/arch/x86/mm/mem_access.c    | 52 
> +++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
>  xen/include/xen/mem_access.h    |  3 +++
>  6 files changed, 110 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 1629f412dd..f6ba8635bf 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, 
> domid_t domid,
>  /* Switch all vCPUs of the domain to the specified altp2m view */
>  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>                               uint16_t view_id);
> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
> +                              uint16_t view_id, xen_pfn_t gfn, bool sve);
>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_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 0639632477..4710133918 100644
> --- a/tools/libxc/xc_altp2m.c
> +++ b/tools/libxc/xc_altp2m.c
> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, 
> domid_t domid,
>      return rc;
>  }
>
> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_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_set_suppress_ve;
> +    arg->domain = domid;
> +    arg->u.set_suppress_ve.view = view_id;
> +    arg->u.set_suppress_ve.gfn = gfn;
> +    arg->u.set_suppress_ve.suppress_ve = sve;
> +
> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
> +                 HYPERCALL_BUFFER_AS_ARG(arg));
> +
> +    xc_hypercall_buffer_free(handle, arg);
> +    return rc;
> +}
> +
>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>                               uint16_t view_id, xen_pfn_t gfn,
>                               xenmem_access_t access)
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 70ddc81d44..dd8e205551 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4358,6 +4358,7 @@ static int do_altp2m_op(
>      case HVMOP_altp2m_destroy_p2m:
>      case HVMOP_altp2m_switch_p2m:
>      case HVMOP_altp2m_set_mem_access:
> +    case HVMOP_altp2m_set_suppress_ve:
>      case HVMOP_altp2m_change_gfn:
>          break;
>      default:
> @@ -4475,6 +4476,19 @@ static int do_altp2m_op(
>                                      a.u.set_mem_access.view);
>          break;
>
> +    case HVMOP_altp2m_set_suppress_ve:
> +        if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
> +            rc = -EINVAL;
> +        else
> +        {
> +            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
> +            unsigned int altp2m_idx = a.u.set_mem_access.view;
> +            bool suppress_ve = a.u.set_suppress_ve.suppress_ve;
> +
> +            rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
> +        }
> +        break;
> +
>      case HVMOP_altp2m_change_gfn:
>          if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>              rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index d0b0767855..8c39db13e3 100644
> --- 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;

This check looks wrong to me. If this subop should only be used by an
external (privileged) domain then I don't think this should be
implemented as an HVMOP, looks more like a domctl to me.

Tamas

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