| Message ID | 1453925201-15926-1-git-send-email-tlengyel@novetta.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/27/2016 10:06 PM, Tamas K Lengyel wrote: > The altp2m subsystem in its current form uses its own HVMOP hypercall to set > mem_access permissions, duplicating some of the code already present for > setting regular mem_access permissions. In this patch we consolidate the two, > deprecate the HVMOP hypercall and update the corresponding tools. > > Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: Stefano Stabellini <stefano.stabellini@citrix.com> > Cc: Keir Fraser <keir@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <george.dunlap@eu.citrix.com> > --- > tools/libxc/include/xenctrl.h | 5 +- > tools/libxc/xc_altp2m.c | 25 ------ > tools/libxc/xc_mem_access.c | 14 +-- > tools/tests/xen-access/xen-access.c | 53 ++++------- > xen/arch/arm/p2m.c | 9 +- > xen/arch/x86/hvm/hvm.c | 9 -- > xen/arch/x86/mm/p2m.c | 169 ++++++++++++++++-------------------- > xen/common/mem_access.c | 2 +- > xen/include/asm-x86/p2m.h | 4 - > xen/include/public/hvm/hvm_op.h | 15 +--- > xen/include/public/memory.h | 3 + > xen/include/xen/p2m-common.h | 3 +- > 12 files changed, 115 insertions(+), 196 deletions(-) For the part of the code in the files we're maintaining: Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com> Thanks for doing this, as also discussed in private I think something along these lines is a much needed change here. Thanks, Razvan
On Wed, Jan 27, 2016 at 01:06:40PM -0700, Tamas K Lengyel wrote: > The altp2m subsystem in its current form uses its own HVMOP hypercall to set > mem_access permissions, duplicating some of the code already present for > setting regular mem_access permissions. In this patch we consolidate the two, > deprecate the HVMOP hypercall and update the corresponding tools. > > Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> > Cc: Stefano Stabellini <stefano.stabellini@citrix.com> > Cc: Keir Fraser <keir@xen.org> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <george.dunlap@eu.citrix.com> > --- > tools/libxc/include/xenctrl.h | 5 +- > tools/libxc/xc_altp2m.c | 25 ------ > tools/libxc/xc_mem_access.c | 14 +-- > tools/tests/xen-access/xen-access.c | 53 ++++------- > xen/arch/arm/p2m.c | 9 +- > xen/arch/x86/hvm/hvm.c | 9 -- > xen/arch/x86/mm/p2m.c | 169 ++++++++++++++++-------------------- > xen/common/mem_access.c | 2 +- > xen/include/asm-x86/p2m.h | 4 - > xen/include/public/hvm/hvm_op.h | 15 +--- > xen/include/public/memory.h | 3 + > xen/include/xen/p2m-common.h | 3 +- > 12 files changed, 115 insertions(+), 196 deletions(-) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index e632b1e..b4e57d8 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, > /* Switch all vCPUs of the domain to the specified altp2m view */ > int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > uint16_t view_id); > -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > - uint16_t view_id, xen_pfn_t gfn, > - xenmem_access_t access); What is the support status of these APIs in libxc? Are they supposed to be stable now? Do you have opinion on making them stable interfaces? If you consider them stable, you can't just remove it. Presumably you can reimplement it as a wrapper to xc_set_mem_access if we decide to keep it around. Wei.
--- a/tools/libxc/include/xenctrl.h > > +++ b/tools/libxc/include/xenctrl.h > > @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle, > > domid_t domid, > > /* Switch all vCPUs of the domain to the specified altp2m view */ > > int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > > uint16_t view_id); > > -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > > - uint16_t view_id, xen_pfn_t gfn, > > - xenmem_access_t access); > > What is the support status of these APIs in libxc? Are they supposed to > be stable now? Do you have opinion on making them stable interfaces? Nothing in libxc is ever a stable interface. It is always completely fine to change them if that is what is needed. Ian.
On Thu, Jan 28, 2016 at 10:55:36AM +0000, Ian Campbell wrote: > --- a/tools/libxc/include/xenctrl.h > > > +++ b/tools/libxc/include/xenctrl.h > > > @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle, > > > domid_t domid, > > > /* Switch all vCPUs of the domain to the specified altp2m view */ > > > int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > > > uint16_t view_id); > > > -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > > > - uint16_t view_id, xen_pfn_t gfn, > > > - xenmem_access_t access); > > > > What is the support status of these APIs in libxc? Are they supposed to > > be stable now? Do you have opinion on making them stable interfaces? > > Nothing in libxc is ever a stable interface. It is always completely fine > to change them if that is what is needed. > Right. Ignore the first question and this patch is ready to go in when those nits are fixed. It would still be nice if we can get a libmemaccess or some sort so that people can build on top of it. Just curious if that's feasible. Wei. > Ian.
>>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote: > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -431,18 +431,6 @@ struct xen_hvm_altp2m_view { > typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t; > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t); > > -struct xen_hvm_altp2m_set_mem_access { > - /* view */ > - uint16_t view; > - /* Memory type */ > - uint16_t hvmmem_access; /* xenmem_access_t */ > - uint32_t pad; > - /* gfn */ > - uint64_t gfn; > -}; > -typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; > -DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); This is a guest visible interface, and hence can't be removed (nor be replaced by a tools only one). > --- a/xen/include/public/memory.h > +++ b/xen/include/public/memory.h > @@ -423,11 +423,14 @@ struct xen_mem_access_op { > /* xenmem_access_t */ > uint8_t access; > domid_t domid; > + uint16_t altp2m_idx; So this is a tools only interface, yes. But it's not versioned (other than e.g. domctl and sysctl), so altering the interface structure is at least fragile. And then, with this ... > --- a/xen/include/xen/p2m-common.h > +++ b/xen/include/xen/p2m-common.h > @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d, > * If gfn == INVALID_GFN, sets the default access type. > */ > long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, > - uint32_t start, uint32_t mask, xenmem_access_t access); > + uint32_t start, uint32_t mask, xenmem_access_t access, > + unsigned long altp2m_idx); ... why "unsigned long" instead of e.g. "unsigned int" here? Jan
On Jan 28, 2016 4:00 AM, "Wei Liu" <wei.liu2@citrix.com> wrote: > > On Thu, Jan 28, 2016 at 10:55:36AM +0000, Ian Campbell wrote: > > --- a/tools/libxc/include/xenctrl.h > > > > +++ b/tools/libxc/include/xenctrl.h > > > > @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle, > > > > domid_t domid, > > > > /* Switch all vCPUs of the domain to the specified altp2m view */ > > > > int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > > > > uint16_t view_id); > > > > -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > > > > - uint16_t view_id, xen_pfn_t gfn, > > > > - xenmem_access_t access); > > > > > > What is the support status of these APIs in libxc? Are they supposed to > > > be stable now? Do you have opinion on making them stable interfaces? > > > > Nothing in libxc is ever a stable interface. It is always completely fine > > to change them if that is what is needed. > > > > Right. Ignore the first question and this patch is ready to go in when > those nits are fixed. > > It would still be nice if we can get a libmemaccess or some sort so that > people can build on top of it. Just curious if that's feasible. > > Wei. There is LibVMI and Bitdefender also released its library so there are choices if folks find libxc too volatile. Tamas
On Jan 28, 2016 6:18 AM, "Jan Beulich" <JBeulich@suse.com> wrote: > > >>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote: > > --- a/xen/include/public/hvm/hvm_op.h > > +++ b/xen/include/public/hvm/hvm_op.h > > @@ -431,18 +431,6 @@ struct xen_hvm_altp2m_view { > > typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t; > > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t); > > > > -struct xen_hvm_altp2m_set_mem_access { > > - /* view */ > > - uint16_t view; > > - /* Memory type */ > > - uint16_t hvmmem_access; /* xenmem_access_t */ > > - uint32_t pad; > > - /* gfn */ > > - uint64_t gfn; > > -}; > > -typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; > > -DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); > > This is a guest visible interface, and hence can't be removed (nor > be replaced by a tools only one). What is your suggestion? > > > --- a/xen/include/public/memory.h > > +++ b/xen/include/public/memory.h > > @@ -423,11 +423,14 @@ struct xen_mem_access_op { > > /* xenmem_access_t */ > > uint8_t access; > > domid_t domid; > > + uint16_t altp2m_idx; > > So this is a tools only interface, yes. But it's not versioned (other > than e.g. domctl and sysctl), so altering the interface structure is > at least fragile. > > And then, with this ... > > > --- a/xen/include/xen/p2m-common.h > > +++ b/xen/include/xen/p2m-common.h > > @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d, > > * If gfn == INVALID_GFN, sets the default access type. > > */ > > long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, > > - uint32_t start, uint32_t mask, xenmem_access_t access); > > + uint32_t start, uint32_t mask, xenmem_access_t access, > > + unsigned long altp2m_idx); > > ... why "unsigned long" instead of e.g. "unsigned int" here? > These were used interchangebly in the code, I've just picked on. The max value it can legitimetly have is 10 so there is not much difference in going with int instead of long. Tamas
On Thu, Jan 28, 2016 at 07:30:31AM -0700, Lengyel, Tamas wrote: > On Jan 28, 2016 4:00 AM, "Wei Liu" <wei.liu2@citrix.com> wrote: > > > > On Thu, Jan 28, 2016 at 10:55:36AM +0000, Ian Campbell wrote: > > > --- a/tools/libxc/include/xenctrl.h > > > > > +++ b/tools/libxc/include/xenctrl.h > > > > > @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface > *handle, > > > > > domid_t domid, > > > > > /* Switch all vCPUs of the domain to the specified altp2m view */ > > > > > int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, > > > > > uint16_t view_id); > > > > > -int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid, > > > > > - uint16_t view_id, xen_pfn_t gfn, > > > > > - xenmem_access_t access); > > > > > > > > What is the support status of these APIs in libxc? Are they supposed > to > > > > be stable now? Do you have opinion on making them stable interfaces? > > > > > > Nothing in libxc is ever a stable interface. It is always completely > fine > > > to change them if that is what is needed. > > > > > > > Right. Ignore the first question and this patch is ready to go in when > > those nits are fixed. > > > > It would still be nice if we can get a libmemaccess or some sort so that > > people can build on top of it. Just curious if that's feasible. > > > > Wei. > > There is LibVMI and Bitdefender also released its library so there are > choices if folks find libxc too volatile. Thanks, this makes sense. Wei. > > Tamas
>>> On 28.01.16 at 15:34, <tlengyel@novetta.com> wrote: > On Jan 28, 2016 6:18 AM, "Jan Beulich" <JBeulich@suse.com> wrote: >> >> >>> On 27.01.16 at 21:06, <tlengyel@novetta.com> wrote: >> > --- a/xen/include/public/hvm/hvm_op.h >> > +++ b/xen/include/public/hvm/hvm_op.h >> > @@ -431,18 +431,6 @@ struct xen_hvm_altp2m_view { >> > typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t; >> > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t); >> > >> > -struct xen_hvm_altp2m_set_mem_access { >> > - /* view */ >> > - uint16_t view; >> > - /* Memory type */ >> > - uint16_t hvmmem_access; /* xenmem_access_t */ >> > - uint32_t pad; >> > - /* gfn */ >> > - uint64_t gfn; >> > -}; >> > -typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; >> > -DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); >> >> This is a guest visible interface, and hence can't be removed (nor >> be replaced by a tools only one). > > What is your suggestion? At the very least keep the old interface, perhaps backed by the new implementation. >> > --- a/xen/include/public/memory.h >> > +++ b/xen/include/public/memory.h >> > @@ -423,11 +423,14 @@ struct xen_mem_access_op { >> > /* xenmem_access_t */ >> > uint8_t access; >> > domid_t domid; >> > + uint16_t altp2m_idx; >> >> So this is a tools only interface, yes. But it's not versioned (other >> than e.g. domctl and sysctl), so altering the interface structure is >> at least fragile. >> >> And then, with this ... >> >> > --- a/xen/include/xen/p2m-common.h >> > +++ b/xen/include/xen/p2m-common.h >> > @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d, >> > * If gfn == INVALID_GFN, sets the default access type. >> > */ >> > long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, >> > - uint32_t start, uint32_t mask, xenmem_access_t access); >> > + uint32_t start, uint32_t mask, xenmem_access_t access, >> > + unsigned long altp2m_idx); >> >> ... why "unsigned long" instead of e.g. "unsigned int" here? > > These were used interchangebly in the code, I've just picked on. The max > value it can legitimetly have is 10 so there is not much difference in > going with int instead of long. Since "int" is slightly less expensive to operate on, please use it whenever possible. Jan
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index e632b1e..b4e57d8 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -2027,9 +2027,6 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid, /* Switch all vCPUs of the domain to the specified altp2m view */ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, uint16_t view_id); -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_change_gfn(xc_interface *handle, domid_t domid, uint16_t view_id, xen_pfn_t old_gfn, xen_pfn_t new_gfn); @@ -2062,7 +2059,7 @@ int xc_mem_paging_load(xc_interface *xch, domid_t domain_id, */ int xc_set_mem_access(xc_interface *xch, domid_t domain_id, xenmem_access_t access, uint64_t first_pfn, - uint32_t nr); + uint32_t nr, uint16_t altp2m_idx); /* * Gets the mem access for the given page (returned in access on success) diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c index 0639632..4f44a7b 100644 --- a/tools/libxc/xc_altp2m.c +++ b/tools/libxc/xc_altp2m.c @@ -163,31 +163,6 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid, return rc; } -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 rc; - DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg); - - arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg)); - if ( arg == NULL ) - return -1; - - arg->version = HVMOP_ALTP2M_INTERFACE_VERSION; - arg->cmd = HVMOP_altp2m_set_mem_access; - arg->domain = domid; - arg->u.set_mem_access.view = view_id; - arg->u.set_mem_access.hvmmem_access = access; - arg->u.set_mem_access.gfn = gfn; - - rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, - HYPERCALL_BUFFER_AS_ARG(arg)); - - xc_hypercall_buffer_free(handle, arg); - 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/tools/libxc/xc_mem_access.c b/tools/libxc/xc_mem_access.c index 3634c39..d6fb409 100644 --- a/tools/libxc/xc_mem_access.c +++ b/tools/libxc/xc_mem_access.c @@ -27,15 +27,17 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id, xenmem_access_t access, uint64_t first_pfn, - uint32_t nr) + uint32_t nr, + uint16_t altp2m_idx) { xen_mem_access_op_t mao = { - .op = XENMEM_access_op_set_access, - .domid = domain_id, - .access = access, - .pfn = first_pfn, - .nr = nr + .op = XENMEM_access_op_set_access, + .domid = domain_id, + .access = access, + .pfn = first_pfn, + .nr = nr, + .altp2m_idx = altp2m_idx }; return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao)); diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c index 7993947..2300e9a 100644 --- a/tools/tests/xen-access/xen-access.c +++ b/tools/tests/xen-access/xen-access.c @@ -448,9 +448,6 @@ int main(int argc, char *argv[]) /* With altp2m we just create a new, restricted view of the memory */ if ( altp2m ) { - xen_pfn_t gfn = 0; - unsigned long perm_set = 0; - rc = xc_altp2m_set_domain_state( xch, domain_id, 1 ); if ( rc < 0 ) { @@ -466,17 +463,6 @@ int main(int argc, char *argv[]) } DPRINTF("altp2m view created with id %u\n", altp2m_view_id); - DPRINTF("Setting altp2m mem_access permissions.. "); - - for(; gfn < xenaccess->max_gpfn; ++gfn) - { - rc = xc_altp2m_set_mem_access( xch, domain_id, altp2m_view_id, gfn, - default_access); - if ( !rc ) - perm_set++; - } - - DPRINTF("done! Permissions set on %lu pages.\n", perm_set); rc = xc_altp2m_switch_to_view( xch, domain_id, altp2m_view_id ); if ( rc < 0 ) @@ -493,25 +479,22 @@ int main(int argc, char *argv[]) } } - if ( !altp2m ) + /* Set the default access type and convert all pages to it */ + rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0, 0); + if ( rc < 0 ) { - /* Set the default access type and convert all pages to it */ - rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0); - if ( rc < 0 ) - { - ERROR("Error %d setting default mem access type\n", rc); - goto exit; - } + ERROR("Error %d setting default mem access type\n", rc); + goto exit; + } - rc = xc_set_mem_access(xch, domain_id, default_access, START_PFN, - (xenaccess->max_gpfn - START_PFN) ); + rc = xc_set_mem_access(xch, domain_id, default_access, START_PFN, + (xenaccess->max_gpfn - START_PFN), altp2m_view_id ); - if ( rc < 0 ) - { - ERROR("Error %d setting all memory to access type %d\n", rc, - default_access); - goto exit; - } + if ( rc < 0 ) + { + ERROR("Error %d setting all memory to access type %d\n", rc, + default_access); + goto exit; } if ( breakpoint ) @@ -547,12 +530,12 @@ int main(int argc, char *argv[]) for ( vcpu_id = 0; vcpu_id<XEN_LEGACY_MAX_VCPUS; vcpu_id++) rc = control_singlestep(xch, domain_id, vcpu_id, 0); - } else { - rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0); - rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN, - (xenaccess->max_gpfn - START_PFN) ); } + rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, ~0ull, 0, 0); + rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN, + (xenaccess->max_gpfn - START_PFN), 0 ); + shutting_down = 1; } @@ -623,7 +606,7 @@ int main(int argc, char *argv[]) else if ( default_access != after_first_access ) { rc = xc_set_mem_access(xch, domain_id, after_first_access, - req.u.mem_access.gfn, 1); + req.u.mem_access.gfn, 1, 0); if (rc < 0) { ERROR("Error %d setting gfn to access_type %d\n", rc, diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 2190908..8e9b4be 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1709,13 +1709,13 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) if ( npfec.write_access && xma == XENMEM_access_rx2rw ) { rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1, - 0, ~0, XENMEM_access_rw); + 0, ~0, XENMEM_access_rw, 0); return false; } else if ( xma == XENMEM_access_n2rwx ) { rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1, - 0, ~0, XENMEM_access_rwx); + 0, ~0, XENMEM_access_rwx, 0); } /* Otherwise, check if there is a vm_event monitor subscriber */ @@ -1737,7 +1737,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) /* A listener is not required, so clear the access * restrictions. */ rc = p2m_set_mem_access(v->domain, _gfn(paddr_to_pfn(gpa)), 1, - 0, ~0, XENMEM_access_rwx); + 0, ~0, XENMEM_access_rwx, 0); } } @@ -1788,7 +1788,8 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec) * If gfn == INVALID_GFN, sets the default access type. */ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, - uint32_t start, uint32_t mask, xenmem_access_t access) + uint32_t start, uint32_t mask, xenmem_access_t access, + unsigned long altp2m_idx) { struct p2m_domain *p2m = p2m_get_hostp2m(d); p2m_access_t a; diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 674feea..2262abf 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -6394,15 +6394,6 @@ static int do_altp2m_op( rc = p2m_switch_domain_altp2m_by_id(d, a.u.view.view); break; - case HVMOP_altp2m_set_mem_access: - if ( a.u.set_mem_access.pad ) - rc = -EINVAL; - else - rc = p2m_set_altp2m_mem_access(d, a.u.set_mem_access.view, - _gfn(a.u.set_mem_access.gfn), - a.u.set_mem_access.hvmmem_access); - break; - case HVMOP_altp2m_change_gfn: if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) rc = -EINVAL; diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index a45ee35..95bf7ce 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -1777,14 +1777,57 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, return (p2ma == p2m_access_n2rwx); } +static int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m, + struct p2m_domain *ap2m, p2m_access_t a, + unsigned long gfn) +{ + mfn_t mfn; + p2m_type_t t; + p2m_access_t old_a; + unsigned int page_order; + int rc; + + mfn = ap2m->get_entry(ap2m, gfn, &t, &old_a, 0, NULL, NULL); + + /* Check host p2m if no valid entry in alternate */ + if ( !mfn_valid(mfn) ) + { + mfn = hp2m->get_entry(hp2m, gfn, &t, &old_a, + P2M_ALLOC | P2M_UNSHARE, &page_order, NULL); + + rc = -ESRCH; + if ( !mfn_valid(mfn) || t != p2m_ram_rw ) + goto out; + + /* If this is a superpage, copy that first */ + if ( page_order != PAGE_ORDER_4K ) + { + unsigned long mask = ~((1UL << page_order) - 1); + gfn_t gfn2 = _gfn(gfn & mask); + mfn_t mfn2 = _mfn(mfn_x(mfn) & mask); + + rc = ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1); + if ( rc ) + goto out; + } + } + + rc = ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, + (current->domain != d)); + + out: + return rc; +} + /* * Set access type for a region of gfns. * If gfn == INVALID_GFN, sets the default access type. */ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, - uint32_t start, uint32_t mask, xenmem_access_t access) + uint32_t start, uint32_t mask, xenmem_access_t access, + unsigned long altp2m_idx) { - struct p2m_domain *p2m = p2m_get_hostp2m(d); + struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL; p2m_access_t a, _a; p2m_type_t t; mfn_t mfn; @@ -1806,6 +1849,16 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, #undef ACCESS }; + /* altp2m view 0 is treated as the hostp2m */ + if ( altp2m_idx ) + { + if ( altp2m_idx >= MAX_ALTP2M || + d->arch.altp2m_eptp[altp2m_idx] == INVALID_MFN ) + return -EINVAL; + + ap2m = d->arch.altp2m_p2m[altp2m_idx]; + } + switch ( access ) { case 0 ... ARRAY_SIZE(memaccess) - 1: @@ -1826,12 +1879,25 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, } p2m_lock(p2m); + if ( ap2m ) + p2m_lock(ap2m); + for ( gfn_l = gfn_x(gfn) + start; nr > start; ++gfn_l ) { - mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL); - rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); - if ( rc ) - break; + if ( ap2m ) + { + rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, gfn_l); + /* If the corresponding mfn is invalid we will just skip it */ + if ( rc && rc != -ESRCH ) + break; + } + else + { + mfn = p2m->get_entry(p2m, gfn_l, &t, &_a, 0, NULL, NULL); + rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1); + if ( rc ) + break; + } /* Check for continuation if it's not the last iteration. */ if ( nr > ++start && !(start & mask) && hypercall_preempt_check() ) @@ -1840,7 +1906,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, break; } } + + if ( ap2m ) + p2m_unlock(ap2m); p2m_unlock(p2m); + return rc; } @@ -2395,93 +2465,6 @@ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx) return rc; } -int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx, - gfn_t gfn, xenmem_access_t access) -{ - struct p2m_domain *hp2m, *ap2m; - p2m_access_t req_a, old_a; - p2m_type_t t; - mfn_t mfn; - unsigned int page_order; - int rc = -EINVAL; - - static const p2m_access_t memaccess[] = { -#define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac - ACCESS(n), - ACCESS(r), - ACCESS(w), - ACCESS(rw), - ACCESS(x), - ACCESS(rx), - ACCESS(wx), - ACCESS(rwx), -#undef ACCESS - }; - - if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == INVALID_MFN ) - return rc; - - ap2m = d->arch.altp2m_p2m[idx]; - - switch ( access ) - { - case 0 ... ARRAY_SIZE(memaccess) - 1: - req_a = memaccess[access]; - break; - case XENMEM_access_default: - req_a = ap2m->default_access; - break; - default: - return rc; - } - - /* If request to set default access */ - if ( gfn_x(gfn) == INVALID_GFN ) - { - ap2m->default_access = req_a; - return 0; - } - - hp2m = p2m_get_hostp2m(d); - - p2m_lock(ap2m); - - mfn = ap2m->get_entry(ap2m, gfn_x(gfn), &t, &old_a, 0, NULL, NULL); - - /* Check host p2m if no valid entry in alternate */ - if ( !mfn_valid(mfn) ) - { - mfn = hp2m->get_entry(hp2m, gfn_x(gfn), &t, &old_a, - P2M_ALLOC | P2M_UNSHARE, &page_order, NULL); - - if ( !mfn_valid(mfn) || t != p2m_ram_rw ) - goto out; - - /* If this is a superpage, copy that first */ - if ( page_order != PAGE_ORDER_4K ) - { - gfn_t gfn2; - unsigned long mask; - mfn_t mfn2; - - mask = ~((1UL << page_order) - 1); - gfn2 = _gfn(gfn_x(gfn) & mask); - mfn2 = _mfn(mfn_x(mfn) & mask); - - if ( ap2m->set_entry(ap2m, gfn_x(gfn2), mfn2, page_order, t, old_a, 1) ) - goto out; - } - } - - if ( !ap2m->set_entry(ap2m, gfn_x(gfn), mfn, PAGE_ORDER_4K, t, req_a, - (current->domain != d)) ) - rc = 0; - - out: - p2m_unlock(ap2m); - return rc; -} - int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, gfn_t old_gfn, gfn_t new_gfn) { diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c index 159c036..0411443 100644 --- a/xen/common/mem_access.c +++ b/xen/common/mem_access.c @@ -67,7 +67,7 @@ int mem_access_memop(unsigned long cmd, break; rc = p2m_set_mem_access(d, _gfn(mao.pfn), mao.nr, start_iter, - MEMOP_CMD_MASK, mao.access); + MEMOP_CMD_MASK, mao.access, mao.altp2m_idx); if ( rc > 0 ) { ASSERT(!(rc & MEMOP_CMD_MASK)); diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index fa46dd9..c0df1ea 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -808,10 +808,6 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx); /* Switch alternate p2m for entire domain */ int p2m_switch_domain_altp2m_by_id(struct domain *d, unsigned int idx); -/* Set access type for a gfn */ -int p2m_set_altp2m_mem_access(struct domain *d, unsigned int idx, - gfn_t gfn, xenmem_access_t access); - /* Change a gfn->mfn mapping */ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx, gfn_t old_gfn, gfn_t new_gfn); diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index 1606185..dfc9db7 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -431,18 +431,6 @@ struct xen_hvm_altp2m_view { typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t; DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t); -struct xen_hvm_altp2m_set_mem_access { - /* view */ - uint16_t view; - /* Memory type */ - uint16_t hvmmem_access; /* xenmem_access_t */ - uint32_t pad; - /* gfn */ - uint64_t gfn; -}; -typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t; -DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); - struct xen_hvm_altp2m_change_gfn { /* view */ uint16_t view; @@ -470,7 +458,7 @@ struct xen_hvm_altp2m_op { #define HVMOP_altp2m_destroy_p2m 5 /* Switch view for an entire domain */ #define HVMOP_altp2m_switch_p2m 6 -/* Notify that a page of memory is to have specific access types */ +/* Deprecated by XENMEM_access_op_set_access */ #define HVMOP_altp2m_set_mem_access 7 /* Change a p2m entry to have a different gfn->mfn mapping */ #define HVMOP_altp2m_change_gfn 8 @@ -481,7 +469,6 @@ struct xen_hvm_altp2m_op { struct xen_hvm_altp2m_domain_state domain_state; struct xen_hvm_altp2m_vcpu_enable_notify enable_notify; struct xen_hvm_altp2m_view view; - struct xen_hvm_altp2m_set_mem_access set_mem_access; struct xen_hvm_altp2m_change_gfn change_gfn; uint8_t pad[64]; } u; diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index 4df38d6..30ccb41 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -423,11 +423,14 @@ struct xen_mem_access_op { /* xenmem_access_t */ uint8_t access; domid_t domid; + uint16_t altp2m_idx; + uint16_t _pad; /* * Number of pages for set op * Ignored on setting default access and other ops */ uint32_t nr; + uint32_t _pad2; /* * First pfn for set op * pfn for get op diff --git a/xen/include/xen/p2m-common.h b/xen/include/xen/p2m-common.h index 47c40c7..0643ad5 100644 --- a/xen/include/xen/p2m-common.h +++ b/xen/include/xen/p2m-common.h @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d, * If gfn == INVALID_GFN, sets the default access type. */ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr, - uint32_t start, uint32_t mask, xenmem_access_t access); + uint32_t start, uint32_t mask, xenmem_access_t access, + unsigned long altp2m_idx); /* * Get access type for a gfn.
The altp2m subsystem in its current form uses its own HVMOP hypercall to set mem_access permissions, duplicating some of the code already present for setting regular mem_access permissions. In this patch we consolidate the two, deprecate the HVMOP hypercall and update the corresponding tools. Signed-off-by: Tamas K Lengyel <tlengyel@novetta.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Cc: Ian Campbell <ian.campbell@citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com> Cc: Stefano Stabellini <stefano.stabellini@citrix.com> Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <george.dunlap@eu.citrix.com> --- tools/libxc/include/xenctrl.h | 5 +- tools/libxc/xc_altp2m.c | 25 ------ tools/libxc/xc_mem_access.c | 14 +-- tools/tests/xen-access/xen-access.c | 53 ++++------- xen/arch/arm/p2m.c | 9 +- xen/arch/x86/hvm/hvm.c | 9 -- xen/arch/x86/mm/p2m.c | 169 ++++++++++++++++-------------------- xen/common/mem_access.c | 2 +- xen/include/asm-x86/p2m.h | 4 - xen/include/public/hvm/hvm_op.h | 15 +--- xen/include/public/memory.h | 3 + xen/include/xen/p2m-common.h | 3 +- 12 files changed, 115 insertions(+), 196 deletions(-)