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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 03/09/2017 12:16 PM, Jan Beulich wrote:
>>>> 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.

So should I revert to a V3 that's basically V1 then? I've been
trigger-happy and submitted V2 roughly an hour ago based on Tamas'
suggestion.


Thanks,
Razvan

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