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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Mar 8, 2017 at 2:36 PM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> 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.

That's true. In light of that it indeed might be saner to keep your
current design.

Thanks,
Tamas

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