[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v10] x86/altp2m: support for setting restrictions for an array of pages
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) * '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)? (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." Everything else looks good to me. -George _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |