Re: [Xen-devel] [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 03/30/2018 02:16 PM, George Dunlap wrote:
> On Wed, Dec 13, 2017 at 2:22 PM, Petre Pircalabu
> <ppircalabu@xxxxxxxxxxxxxxx> wrote:
>> From: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>
>> 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.
>>
>> HVMOP_altp2m_set_mem_access_multi has been added as a HVMOP (as opposed to a
>> DOMCTL) for consistency with its HVMOP_altp2m_set_mem_access counterpart (and
>> hence with the original altp2m design, where domains are allowed - with the
>> proper altp2m access rights - to alter these settings), in the absence of an
>> official position on the issue from the original altp2m designers.
> 
> This mostly looks good to me, with a couple of nitpicks...
> 
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 666db0b..f171668 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -1974,6 +1974,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, 
>> uint32_t domid,
>>  int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
>>                           uint16_t view_id, xen_pfn_t old_gfn,
>>                           xen_pfn_t new_gfn);
>> +int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
>> +                                   uint16_t view_id, uint8_t *access,
>> +                                   uint64_t *pages, uint32_t nr);
> 
> Two minor things:
> 
> * It seems like it would make sense to put this directly under the
> non-multi version of this call (even though that does put it out of
> order with the command number)

Not a problem. We'll move it.

> * 'Pages' is ambiguous here, as it could be interpreted to mean Linux
> virtual pages rather than gfn.  Is there a reason not to call this
> argument 'gfns' (as in the other xc call) or 'pfn_list' (as in the
> hypercall)?

No, we'll rename it to 'gfns'.

> (And sorry if this has been covered before; I did do a quick look over
> the history and didn't notice anything.)
> 
>> @@ -4619,6 +4623,37 @@ static int do_altp2m_op(
>>                                      a.u.set_mem_access.view);
>>          break;
>>
>> +    case HVMOP_altp2m_set_mem_access_multi:
>> +        if ( a.u.set_mem_access_multi.pad ||
>> +             a.u.set_mem_access_multi.opaque > a.u.set_mem_access_multi.nr )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +
>> +        /*
>> +         * The mask was set (arbitrary) to 0x3F to match the value used for
>> +         * MEMOP, despite the fact there are no encoding limitations for the
>> +         * start parameter.
>> +         */
> 
> This comment isn't actually very enlightening if you're not already
> intimately familiar with the code; it took me at least 10 minutes of
> grepping around to figure out what this was about.
> 
> What about this:
> 
> "Unlike XENMEM_access_op_set_access_multi, we don't need any bits of
> the 'continuation' counter to be zero (to stash a command in).
> However, 0x40 is a good 'stride' to make sure
> that we make a reasonable amount of forward progress before yielding,
> so use a mask of 0x3F here."

We have no objection to the change (you're right, the original is quite
terse).


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel