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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Mar 8, 2017 at 12:19 PM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 03/08/2017 08:30 PM, Tamas K Lengyel 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.
>
> I'm not sure there's a real use case for that. The _multi() function has
> been added to help with cases where we need to set thousands of pages -
> in which case condensing it to a single hypercall vs. thousands of them
> noticeably improves performance.
>
> But with views, I'd bet that in almost all cases people will want to
> only use one extra view, and even if they'd like to use more, it'll
> still be at most MAX_ALTP2M + 1 hypercalls, which currently means 11.
> That's actually not even a valid use case, since if we're setting
> restrictions on _all_ the views we might as well be simply using the
> default view alone.

FYI I already use more then one view..

> In other words, I would argue that the small gain does not justify the
> extra development effort.

I don't think it would be much extra development effort considering
both the access and page fields are passed in as an array already. But
anyway, it was just a suggestion, I won't hold the patch up over it.

>>>  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=f3356e1d4db14439fcca47c493d902bbbb5ec17e
>> for an example.
>
> Are we? I'm also not a big fan of all the mask / bit-fiddling for
> continuation purposes, but that's how p2m_set_mem_access_multi() works
> at the moment (which I've used for both
> XENMEM_access_op_set_access_multi and, in this patch,
> HVMOP_altp2m_set_mem_access_multi). It's also used by
> p2m_set_mem_access() / XENMEM_access_op_set_access.
>
> Changing the way continuation works in this patch would mean reworking
> all that code, which would effectively transform this relatively small
> patch into a series. If that's required we can go in that direction, but
> I'm not sure we want that. Waiting for further opinions.

I'm not saying you need to rework all pre-existing code to do that,
but at least for new ops being introduced it should be the way we
continue. If you can't reuse existing functions for it, introducing a
new one is desirable. We can figure out how to migrate pre-existing
hypercalls to the new method later.

Tamas

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