Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On 23.10.2017 11:41, Jan Beulich wrote: On 23.10.17 at 10:34, wrote: > >> >> On 23.10.2017 11:10, Jan Beulich wrote: >> On 20.10.17 at 18:32, wrote: On 10/20/2017 07:15 PM, Wei Liu wrote: > On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote: >> From: Razvan Cojocaru >> >> 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. >> >> Signed-off-by: Razvan Cojocaru >> Signed-off-by: Petre Pircalabu >> > > The title is a bit misleading -- this patch actually contains changes to > hypervisor as well. Sorry, I have assumed that the hypervisor changes are implied. We're happy to change it. Would "x86/altp2m: Added xc_altp2m_set_mem_access_multi() and hypervisor support" be better? >>> >>> But please not again "Added" - we've had this discussion before. >>> The title is supposed to tell what a patch does, not what the state >>> of the code is after it was applied. >> >> Will do, how does "{xen,libxc}/altp2m: support for setting restrictions >> for an array of pages" sound? > > The text is fine, but I'm not sure the {xen,libxc} part of the prefix > is really very useful. I was hoping to address Wei's comment with it - 'xen' would stand for the hypervisor part, and 'libxc' for the toolstack part. However, you're right: for one, the 'x86' part was useful, and then the problem before was not so much that it didn't explicitly specify 'xen', but that it implied that the changes have more to do with libxc (because it mentioned xc_altp2m_set_mem_access_multi()). "x86/altp2m: support for setting restrictions for an array of pages" it is then. :) Sorry for causing confusion! Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
>>> On 23.10.17 at 10:34, wrote: > > On 23.10.2017 11:10, Jan Beulich wrote: > On 20.10.17 at 18:32, wrote: >>> On 10/20/2017 07:15 PM, Wei Liu wrote: On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote: > From: Razvan Cojocaru > > 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. > > Signed-off-by: Razvan Cojocaru > Signed-off-by: Petre Pircalabu > The title is a bit misleading -- this patch actually contains changes to hypervisor as well. >>> >>> Sorry, I have assumed that the hypervisor changes are implied. We're >>> happy to change it. Would "x86/altp2m: Added >>> xc_altp2m_set_mem_access_multi() and hypervisor support" be better? >> >> But please not again "Added" - we've had this discussion before. >> The title is supposed to tell what a patch does, not what the state >> of the code is after it was applied. > > Will do, how does "{xen,libxc}/altp2m: support for setting restrictions > for an array of pages" sound? The text is fine, but I'm not sure the {xen,libxc} part of the prefix is really very useful. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On 23.10.2017 11:10, Jan Beulich wrote: On 20.10.17 at 18:32, wrote: >> On 10/20/2017 07:15 PM, Wei Liu wrote: >>> On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote: From: Razvan Cojocaru 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. Signed-off-by: Razvan Cojocaru Signed-off-by: Petre Pircalabu >>> >>> The title is a bit misleading -- this patch actually contains changes to >>> hypervisor as well. >> >> Sorry, I have assumed that the hypervisor changes are implied. We're >> happy to change it. Would "x86/altp2m: Added >> xc_altp2m_set_mem_access_multi() and hypervisor support" be better? > > But please not again "Added" - we've had this discussion before. > The title is supposed to tell what a patch does, not what the state > of the code is after it was applied. Will do, how does "{xen,libxc}/altp2m: support for setting restrictions for an array of pages" sound? We'll change the title as soon as we have comments to address for a new version. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
>>> On 20.10.17 at 18:32, wrote: > On 10/20/2017 07:15 PM, Wei Liu wrote: >> On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote: >>> From: Razvan Cojocaru >>> >>> 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. >>> >>> Signed-off-by: Razvan Cojocaru >>> Signed-off-by: Petre Pircalabu >>> >> >> The title is a bit misleading -- this patch actually contains changes to >> hypervisor as well. > > Sorry, I have assumed that the hypervisor changes are implied. We're > happy to change it. Would "x86/altp2m: Added > xc_altp2m_set_mem_access_multi() and hypervisor support" be better? But please not again "Added" - we've had this discussion before. The title is supposed to tell what a patch does, not what the state of the code is after it was applied. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On 10/20/2017 07:39 PM, Wei Liu wrote: > On Fri, Oct 20, 2017 at 07:32:50PM +0300, Razvan Cojocaru wrote: >> On 10/20/2017 07:15 PM, Wei Liu wrote: >>> On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote: From: Razvan Cojocaru 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. Signed-off-by: Razvan Cojocaru Signed-off-by: Petre Pircalabu >>> >>> The title is a bit misleading -- this patch actually contains changes to >>> hypervisor as well. >> >> Sorry, I have assumed that the hypervisor changes are implied. > > And to expound my thought on this -- change in hypervisor is not > implied. We have had cases in which only toolstack change was needed > because hypervisor code was already there. Getting the title correct > will help reviewers identify patches they need to review. Got it. We'll change the title in the next iteration. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On Fri, Oct 20, 2017 at 07:32:50PM +0300, Razvan Cojocaru wrote: > On 10/20/2017 07:15 PM, Wei Liu wrote: > > On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote: > >> From: Razvan Cojocaru > >> > >> 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. > >> > >> Signed-off-by: Razvan Cojocaru > >> Signed-off-by: Petre Pircalabu > >> > > > > The title is a bit misleading -- this patch actually contains changes to > > hypervisor as well. > > Sorry, I have assumed that the hypervisor changes are implied. And to expound my thought on this -- change in hypervisor is not implied. We have had cases in which only toolstack change was needed because hypervisor code was already there. Getting the title correct will help reviewers identify patches they need to review. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On Fri, Oct 20, 2017 at 07:32:50PM +0300, Razvan Cojocaru wrote: > On 10/20/2017 07:15 PM, Wei Liu wrote: > > On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote: > >> From: Razvan Cojocaru > >> > >> 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. > >> > >> Signed-off-by: Razvan Cojocaru > >> Signed-off-by: Petre Pircalabu > >> > > > > The title is a bit misleading -- this patch actually contains changes to > > hypervisor as well. > > Sorry, I have assumed that the hypervisor changes are implied. We're > happy to change it. Would "x86/altp2m: Added > xc_altp2m_set_mem_access_multi() and hypervisor support" be better? > That's fine. I sent my previous mail because I thought hypervisor maintainer missed this patch. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On 10/20/2017 07:15 PM, Wei Liu wrote: > On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote: >> From: Razvan Cojocaru >> >> 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. >> >> Signed-off-by: Razvan Cojocaru >> Signed-off-by: Petre Pircalabu >> > > The title is a bit misleading -- this patch actually contains changes to > hypervisor as well. Sorry, I have assumed that the hypervisor changes are implied. We're happy to change it. Would "x86/altp2m: Added xc_altp2m_set_mem_access_multi() and hypervisor support" be better? Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
On Mon, Oct 16, 2017 at 08:07:41PM +0300, Petre Pircalabu wrote: > From: Razvan Cojocaru > > 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. > > Signed-off-by: Razvan Cojocaru > Signed-off-by: Petre Pircalabu > The title is a bit misleading -- this patch actually contains changes to hypervisor as well. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v7] x86/altp2m: Added xc_altp2m_set_mem_access_multi()
From: Razvan Cojocaru 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. Signed-off-by: Razvan Cojocaru Signed-off-by: Petre Pircalabu --- Changed since v2: * Added support for compat arguments translation Changed since v3: * Replaced __copy_to_guest with __copy_field_to_guest * Removed the un-needed parentheses. * Fixed xlat.lst ordering * Added comment to patch description explaining why the functionality was added as an HVMOP. * Guard using XEN_GENERATING_COMPAT_HEADERS the hvmmem_type_t definition. This will prevent suplicate definitions to be generated for the compat equivalent. * Added comment describing the manual translation of xen_hvm_altp2m_op_t generic fields from compat_hvm_altp2m_op_t. Changed since v4: * Changed the mask parameter to 0x3F. * Split long lines. * Added "improperly named HVMMEM_(*)" to the comment explaining the XEN_GENERATING_COMPAT_HEADERS guard. * Removed typedef and XEN_GUEST_HANDLE for xen_hvm_altp2m_set_mem_access_multi. * Copied the "opaque" field to guest in compat_altp2m_op. * Added build time test to check if the size of xen_hvm_altp2m_set_mem_access_multi at least equal to the size of compat_hvm_altp2m_set_mem_access_multi. Changed since v5: * Changed the domid parameter type to uint32_t to match 5b42c82f. * Added comment to explain why the 0x3F value was chosen. * Fixed switch indentation in compat_altp2m_op. * Changed the condition used to check if the opaque field has to be copied to the guest. * Added CHECK_hvm_altp2m_op and CHECK_hvm_altp2m_set_mem_access_multi. Changed since v6: * Removed trailing semicolon from the definitions of CHECK_hvm_altp2m_op and CHECK_hvm_altp2m_set_mem_access_multi. * Removed BUILD_BUG_ON check. * Added comment describing the reason for manually defining the CHECK_ macros. * Added ASSERT_UNREACHABLE as the default switch label action in compat_altp2m_op. * Added ASSERT(rc == __HYPERVISOR_hvm_op) to make sure the return code was actually sey by hypercall_create_continuation. --- tools/libxc/include/xenctrl.h | 3 + tools/libxc/xc_altp2m.c | 40 xen/arch/x86/hvm/hvm.c | 138 +++- xen/include/Makefile| 1 + xen/include/public/hvm/hvm_op.h | 36 +-- xen/include/xlat.lst| 1 + 6 files changed, 213 insertions(+), 6 deletions(-) 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); /** * Mem paging operations. diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c index 07fcd18..e170fe3 100644 --- a/tools/libxc/xc_altp2m.c +++ b/tools/libxc/xc_altp2m.c @@ -213,3 +213,43 @@ int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid, return rc; } +int xc_altp2m_set_mem_access_multi(xc_interface *xch, uint32_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) ) +{ +