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

[Tamas K Lengyel <tamas@tklengyel.com>]

On Wed, Mar 8, 2017 at 1:17 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 03/08/2017 09:53 PM, Tamas K Lengyel wrote:
>> On Wed, Mar 8, 2017 at 12:19 PM, Razvan Cojocaru
>> <rcojocaru@bitdefender.com> wrote:
>>> On 03/08/2017 08:30 PM, Tamas K Lengyel wrote:
>>>> On Wed, Mar 8, 2017 at 2:01 AM, Razvan Cojocaru
>>>> <rcojocaru@bitdefender.com> 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@bitdefender.com>
>>>>> ---
>>>>>  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..
>
> Fair enough, but the question is, when you do, is it likely that you'll
> want to set the same restrictions for the same multiple pages in several
> views at one time?

Well, if you have the view id as an extra array, you could set
different permissions in different views using a single hypercall. For
example, you could do something along the lines:

view_ids[0,1,2]=1, access[0,1,2]=rw pages[0,1,2] = {10,11,12}
view_ids[3,4,5]=2, access[3,4,5]=x,  pages[3,4,5] = {10,11,12}.
view_ids[6]=0,        access[6]=rwx,    pages[6] = 13

>
>>> 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.
>
> Passing the array / size to the hypervisor is obviously trivial, my
> concern is that p2m_set_mem_access_multi() would need to be reworked to
> use the array, which might also require special error handling (which
> view had a problem?) and continuation logic (do we now require two start
> indices, one for the gfn list and one for the view list and reset the
> one for the gfn list at the end of processing a view?).

I'm not sure I follow. I would imagine the size of the view_ids array
would be the same as the other arrays, so there is only one loop that
goes through the whole thing.

>
> On an unrelated note, running scripts/get-maintainers.pl on this patch
> did not list you - is that something that should be fixed? I value your
> opinion and expertise with altp2m so I've CCd you anyway but this may be
> something you may want to address in the MAINTAINERS file.
>

Yea, I'm not the maintainer of altp2m or any of the files you touched
in this patch so not being listed as a maintainer is correct.

>>>>>  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.
>
> I'll look into it, and come back when I get a better feel of the
> obstacles. In the meantime, of course, other opinions are welcome.

Thanks!
Tamas

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