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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 03/08/2017 10:53 PM, Tamas K Lengyel wrote:
> 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

I see what you mean now. I thought you meant:

view_ids={1,2,3,4,5}, access={rw,rx,rwx} pages={0xff,0xfa,0xfb}

In which case, obviously the view_ids array would need its own size
sent, and for each view we'd go through the pages/access arrays and set
page restrictions.

>>>> 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.

Right, we clearly had different pictures of what you meant by "an array
of view_ids" (as seen above).

That's somewhat more approachable, although it still poses the question
of what happens when we set page X 'r' in view 1, for example, and then
again set page X, but 'rwx' in view 0 (the default EPT, which then also
alters all the other views). This can have subtle consequences.

It would also require p2m switching in the loop, so a different or
significantly altered p2m_set_mem_access_multi(). Also with potential
inefficient TLB flushing consequences (though this needs checking).


Thanks,
Razvan

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