|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On Wed, Mar 8, 2017 at 1:17 PM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 03/08/2017 09:53 PM, Tamas K Lengyel wrote:
>> 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..
>
> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |