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