Re: [Xen-devel] [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.03.17 at 19:30, <tamas@xxxxxxxxxxxxx> wrote:
> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
> <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> For the default EPT view we have xc_set_mem_access_multi(), which
>> is able to set an array of pages to an array of access rights with
>> a single hypercall. However, this functionality was lacking for the
>> altp2m subsystem, which could only set page restrictions for one
>> page at a time. This patch addresses the gap.
>>
>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> ---
>>  tools/libxc/include/xenctrl.h   |  3 +++
>>  tools/libxc/xc_altp2m.c         | 41 
> +++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/x86/hvm/hvm.c          | 30 +++++++++++++++++++++++++++---
>>  xen/include/public/hvm/hvm_op.h | 28 +++++++++++++++++++++++-----
>>  4 files changed, 94 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index a48981a..645b5bd 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, 
> domid_t domid,
>>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>                               uint16_t view_id, xen_pfn_t gfn,
>>                               xenmem_access_t access);
>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
>> +                                   uint16_t view_id, uint8_t *access,
>> +                                   uint64_t *pages, uint32_t nr);
> 
> IMHO we might as well take an array of view_ids as well so you can set
> multiple pages in multiple views at the same time.
> 
>>  int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>                           uint16_t view_id, xen_pfn_t old_gfn,
>>                           xen_pfn_t new_gfn);
>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>> index 0639632..f202ca1 100644
>> --- a/tools/libxc/xc_altp2m.c
>> +++ b/tools/libxc/xc_altp2m.c
>> @@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, 
> domid_t domid,
>>      return rc;
>>  }
>>
>> +int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
>> +                                   uint16_t view_id, uint8_t *access,
>> +                                   uint64_t *pages, uint32_t nr)
>> +{
>> +    int rc;
>> +
>> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>> +    DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
>> +    DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
>> +                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
>> +
>> +    arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
>> +    if ( arg == NULL )
>> +        return -1;
>> +
>> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>> +    arg->cmd = HVMOP_altp2m_set_mem_access_multi;
>> +    arg->domain = domid;
>> +    arg->u.set_mem_access_multi.view = view_id;
>> +    arg->u.set_mem_access_multi.nr = nr;
>> +
>> +    if ( xc_hypercall_bounce_pre(xch, pages) ||
>> +         xc_hypercall_bounce_pre(xch, access) )
>> +    {
>> +        PERROR("Could not bounce memory for 
> HVMOP_altp2m_set_mem_access_multi");
>> +        return -1;
>> +    }
>> +
>> +    set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
>> +    set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
>> +
>> +    rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>> +                 HYPERCALL_BUFFER_AS_ARG(arg));
>> +
>> +    xc_hypercall_buffer_free(xch, arg);
>> +    xc_hypercall_bounce_post(xch, access);
>> +    xc_hypercall_bounce_post(xch, pages);
>> +
>> +    return rc;
>> +}
>> +
>>  int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
>>                           uint16_t view_id, xen_pfn_t old_gfn,
>>                           xen_pfn_t new_gfn)
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index ccfae4f..cc9b207 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4394,11 +4394,13 @@ static int hvmop_get_param(
>>  }
>>
>>  static int do_altp2m_op(
>> +    unsigned long cmd,
>>      XEN_GUEST_HANDLE_PARAM(void) arg)
>>  {
>>      struct xen_hvm_altp2m_op a;
>>      struct domain *d = NULL;
>> -    int rc = 0;
>> +    long rc = 0;
>> +    unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
> 
> I believe we are trying to transition away from stashing the
> continuation values into the cmd itself. In another patch of mine the
> new way to do this has been by introducing an opaque variable into the
> structure passed in by the user to be used for storing the
> continuation value. Take a look at
> https://xenbits.xenproject.org/gitweb/?p=xen.git;a=commit;h=f3356e1d4db14439 
> fcca47c493d902bbbb5ec17e
> for an example.

I think it was a mistake to allow this in - imo memop-s should be
consistent in how they handle continuations. For new hypercalls
(like dmop) the story is different of course.

Jan

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