[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Thu, Jun 15, 2017 at 8:01 PM, Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
> 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.

Well after patch 1, isn't it the case that a guest has no way of
clearing the suppress_ve bit?

I was going to say we want the XSM_TARGET "default action" (which
allows a guest to do things on itself, or a privileged domain to do it
to any domain); but I think really we probably we don't want a guest
to be able to *clear* the suppress_ve bit on a page for which a
privileged domain has *set*; this would allow a domain to prevent the
other domain from effectively introspecting on a page.

This is starting to sound like another conversation I think I remember
recently about making sure that *only* the guest *or* an introspection
engine can use the altp2m functionality, but I can't seem to find that
with a quick look. Tamas, does that ring any bells?

 -George

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.