|
[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 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?
>> 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?).
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.
>>>> 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,
Razvan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |