| Message ID | 1490003427-11501-1-git-send-email-rcojocaru@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20/03/17 09:50, Razvan Cojocaru wrote: > xc_altp2m_set_mem_access() and xc_set_mem_access() end up doing the same thing > in the hypervisor, but the former is a HVMOP and the latter a DOMCTL. Since > nobody is currently using, or has stated intent to use, this functionality > specifically as an HVMOP, this patch removes the HVMOP while adding an extra > parameter to the more flexible DOMCTL variant, in which the altp2m view can be > transmitted (0 for the default view, or when altp2m is disabled). > The xen-access test has been updated in the process. > > Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> I am sorry to be awkward here, but as this patch stands, it definitely breaks the original usecase altp2m was introduced for. Therefore, I don't it is appropriate to take in this form. The problem is that the current permissions are too coarse-grained. Intel's use case needs all hypercalls usable by the guest, as the agent is entirely in-guest. (It also occurs to me that scenario might be useful to develop within.) Bitdefenders use case needs vmfunc usable by the guest, but all alteration of view permissions must be restricted to a more privileged entity. Tamas' usecase is (IIRC) entirely behind the back of the guest. As a result, the two choices needed are "can a guest configure/use vmfunc", and "can a guest alter its own view permissions". These should cater to all usecases, as far as I understand. ~Andrew
On 03/20/2017 04:20 PM, Andrew Cooper wrote: > On 20/03/17 09:50, Razvan Cojocaru wrote: >> xc_altp2m_set_mem_access() and xc_set_mem_access() end up doing the same thing >> in the hypervisor, but the former is a HVMOP and the latter a DOMCTL. Since >> nobody is currently using, or has stated intent to use, this functionality >> specifically as an HVMOP, this patch removes the HVMOP while adding an extra >> parameter to the more flexible DOMCTL variant, in which the altp2m view can be >> transmitted (0 for the default view, or when altp2m is disabled). >> The xen-access test has been updated in the process. >> >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > > I am sorry to be awkward here, but as this patch stands, it definitely > breaks the original usecase altp2m was introduced for. Therefore, I > don't it is appropriate to take in this form. > > The problem is that the current permissions are too coarse-grained. > > Intel's use case needs all hypercalls usable by the guest, as the agent > is entirely in-guest. (It also occurs to me that scenario might be > useful to develop within.) > > Bitdefenders use case needs vmfunc usable by the guest, but all > alteration of view permissions must be restricted to a more privileged > entity. > > Tamas' usecase is (IIRC) entirely behind the back of the guest. > > > As a result, the two choices needed are "can a guest configure/use > vmfunc", and "can a guest alter its own view permissions". > > These should cater to all usecases, as far as I understand. Alright, but in that case I'm not sure how to proceed with this. Would returning to the previous form of the patch (adding another HVMOP + solving the compat issues) work? Or are you proposing an altogether different mechanism, alongside which this merge would work? As for Intel's use case, we've unfortunately had no reply from Ravi Sahita (CCd on the discussion even before V1 was sent). If somebody knows other email addresses we should use, please CC them. Thanks, Razvan
On 03/20/2017 04:20 PM, Andrew Cooper wrote: > On 20/03/17 09:50, Razvan Cojocaru wrote: >> xc_altp2m_set_mem_access() and xc_set_mem_access() end up doing the same thing >> in the hypervisor, but the former is a HVMOP and the latter a DOMCTL. Since >> nobody is currently using, or has stated intent to use, this functionality >> specifically as an HVMOP, this patch removes the HVMOP while adding an extra >> parameter to the more flexible DOMCTL variant, in which the altp2m view can be >> transmitted (0 for the default view, or when altp2m is disabled). >> The xen-access test has been updated in the process. >> >> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > > I am sorry to be awkward here, but as this patch stands, it definitely > breaks the original usecase altp2m was introduced for. Therefore, I > don't it is appropriate to take in this form. > > The problem is that the current permissions are too coarse-grained. > > Intel's use case needs all hypercalls usable by the guest, as the agent > is entirely in-guest. (It also occurs to me that scenario might be > useful to develop within.) Actually upon reading this again: https://lists.xenproject.org/archives/html/xen-devel/2015-06/msg01319.html it doesn't look like Intel's use case is for entirely in-guest agents (although granted that's a possiblity): "The altp2m capability allows for para-virtualized guest software agent within or across domains to be able to enforce memory introspection policies in an efficient manner. Altp2m also allows para-virtualized guest agent components to be isolated within an HVM (in terms of guest physical memory) for secure VM introspection as well as various other security and privacy usages that require efficient memory isolation." I could be misreading this, but "para-virtualized guest agent components" sound more like a different domain than a typical in-HVM-guest application. Thanks, Razvan
>>> On 20.03.17 at 10:50, <rcojocaru@bitdefender.com> wrote: > --- a/xen/include/public/memory.h > +++ b/xen/include/public/memory.h > @@ -444,6 +444,8 @@ struct xen_mem_access_op { > /* xenmem_access_t */ > uint8_t access; > domid_t domid; > + uint16_t view_id; > + uint16_t pad[3]; Irrespective of Andrew's valid general objection, the change above wouldn't be valid either: How would you guarantee compatibility with old callers? Other than in e.g. domctl/sysctl there's no interface version here which can be bumped, so simply adding fields to a structure and re-using an existing sub-op won't do. Jan
On 03/20/2017 06:07 PM, Jan Beulich wrote: >>>> On 20.03.17 at 10:50, <rcojocaru@bitdefender.com> wrote: >> --- a/xen/include/public/memory.h >> +++ b/xen/include/public/memory.h >> @@ -444,6 +444,8 @@ struct xen_mem_access_op { >> /* xenmem_access_t */ >> uint8_t access; >> domid_t domid; >> + uint16_t view_id; >> + uint16_t pad[3]; > > Irrespective of Andrew's valid general objection, the change above > wouldn't be valid either: How would you guarantee compatibility > with old callers? Other than in e.g. domctl/sysctl there's no > interface version here which can be bumped, so simply adding > fields to a structure and re-using an existing sub-op won't do. I wouldn't - I thought simply bumping the DOMCTL version macro would be enough, but obviously I could just add other DOMCTLs and return an error for the old ones. In any case, back when I've added xc_set_mem_access_multi() I've also modified struct xen_mem_access_op in the same manner: http://xenbits.xenproject.org/gitweb/?p=xen.git;a=commitdiff;h=1ef5056bd6274ecbe065387b6cf45657d6d700cd Thanks, Razvan
On 03/20/2017 06:14 PM, Razvan Cojocaru wrote: > On 03/20/2017 06:07 PM, Jan Beulich wrote: >>>>> On 20.03.17 at 10:50, <rcojocaru@bitdefender.com> wrote: >>> --- a/xen/include/public/memory.h >>> +++ b/xen/include/public/memory.h >>> @@ -444,6 +444,8 @@ struct xen_mem_access_op { >>> /* xenmem_access_t */ >>> uint8_t access; >>> domid_t domid; >>> + uint16_t view_id; >>> + uint16_t pad[3]; >> >> Irrespective of Andrew's valid general objection, the change above >> wouldn't be valid either: How would you guarantee compatibility >> with old callers? Other than in e.g. domctl/sysctl there's no >> interface version here which can be bumped, so simply adding >> fields to a structure and re-using an existing sub-op won't do. > > I wouldn't - I thought simply bumping the DOMCTL version macro would be > enough, but obviously I could just add other DOMCTLs and return an error > for the old ones. > > In any case, back when I've added xc_set_mem_access_multi() I've also > modified struct xen_mem_access_op in the same manner: > > http://xenbits.xenproject.org/gitweb/?p=xen.git;a=commitdiff;h=1ef5056bd6274ecbe065387b6cf45657d6d700cd Oh, nevermind, I think you're referring to the fact that I had back then added members to the end of the structure, and so the old layout had remained compatible. Point taken. Thanks, Razvan
On Mon, Mar 20, 2017 at 8:20 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 20/03/17 09:50, Razvan Cojocaru wrote: > > xc_altp2m_set_mem_access() and xc_set_mem_access() end up doing the same > thing > > in the hypervisor, but the former is a HVMOP and the latter a DOMCTL. > Since > > nobody is currently using, or has stated intent to use, this > functionality > > specifically as an HVMOP, this patch removes the HVMOP while adding an > extra > > parameter to the more flexible DOMCTL variant, in which the altp2m view > can be > > transmitted (0 for the default view, or when altp2m is disabled). > > The xen-access test has been updated in the process. > > > > Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> > > I am sorry to be awkward here, but as this patch stands, it definitely > breaks the original usecase altp2m was introduced for. Therefore, I > don't it is appropriate to take in this form. > > The problem is that the current permissions are too coarse-grained. > > Intel's use case needs all hypercalls usable by the guest, as the agent > is entirely in-guest. (It also occurs to me that scenario might be > useful to develop within.) > > Bitdefenders use case needs vmfunc usable by the guest, but all > alteration of view permissions must be restricted to a more privileged > entity. > > Tamas' usecase is (IIRC) entirely behind the back of the guest. > > > As a result, the two choices needed are "can a guest configure/use > vmfunc", and "can a guest alter its own view permissions". > > These should cater to all usecases, as far as I understand. > So I have an outstanding patch that addressed this issue, at least partially: https://patchwork.kernel.org/patch/9284861. It was part of a larger series that didn't yet make it to upstream but it doesn't depend on anything in that series so we could take it out. The goal with that patch was to be able to indicate how the default altp2m should behave, ie. mixed internal-external use, or purely external. If there is a need for a third option that would specify that only the #VE call should be enabled from within the guest, that could probably be added. Tamas
>>> On 20.03.17 at 17:16, <rcojocaru@bitdefender.com> wrote: > On 03/20/2017 06:14 PM, Razvan Cojocaru wrote: >> On 03/20/2017 06:07 PM, Jan Beulich wrote: >>>>>> On 20.03.17 at 10:50, <rcojocaru@bitdefender.com> wrote: >>>> --- a/xen/include/public/memory.h >>>> +++ b/xen/include/public/memory.h >>>> @@ -444,6 +444,8 @@ struct xen_mem_access_op { >>>> /* xenmem_access_t */ >>>> uint8_t access; >>>> domid_t domid; >>>> + uint16_t view_id; >>>> + uint16_t pad[3]; >>> >>> Irrespective of Andrew's valid general objection, the change above >>> wouldn't be valid either: How would you guarantee compatibility >>> with old callers? Other than in e.g. domctl/sysctl there's no >>> interface version here which can be bumped, so simply adding >>> fields to a structure and re-using an existing sub-op won't do. >> >> I wouldn't - I thought simply bumping the DOMCTL version macro would be >> enough, but obviously I could just add other DOMCTLs and return an error >> for the old ones. I miss the connection to domctl here - this is a mem-op, isn't it? >> In any case, back when I've added xc_set_mem_access_multi() I've also >> modified struct xen_mem_access_op in the same manner: >> >> > http://xenbits.xenproject.org/gitweb/?p=xen.git;a=commitdiff;h=1ef5056bd6274e > cbe065387b6cf45657d6d700cd > > Oh, nevermind, I think you're referring to the fact that I had back then > added members to the end of the structure, and so the old layout had > remained compatible. Point taken. Not just that - there you've also introduced a new sub-op. Jan
On 03/20/2017 06:40 PM, Jan Beulich wrote: >>>> On 20.03.17 at 17:16, <rcojocaru@bitdefender.com> wrote: >> On 03/20/2017 06:14 PM, Razvan Cojocaru wrote: >>> On 03/20/2017 06:07 PM, Jan Beulich wrote: >>>>>>> On 20.03.17 at 10:50, <rcojocaru@bitdefender.com> wrote: >>>>> --- a/xen/include/public/memory.h >>>>> +++ b/xen/include/public/memory.h >>>>> @@ -444,6 +444,8 @@ struct xen_mem_access_op { >>>>> /* xenmem_access_t */ >>>>> uint8_t access; >>>>> domid_t domid; >>>>> + uint16_t view_id; >>>>> + uint16_t pad[3]; >>>> >>>> Irrespective of Andrew's valid general objection, the change above >>>> wouldn't be valid either: How would you guarantee compatibility >>>> with old callers? Other than in e.g. domctl/sysctl there's no >>>> interface version here which can be bumped, so simply adding >>>> fields to a structure and re-using an existing sub-op won't do. >>> >>> I wouldn't - I thought simply bumping the DOMCTL version macro would be >>> enough, but obviously I could just add other DOMCTLs and return an error >>> for the old ones. > > I miss the connection to domctl here - this is a mem-op, isn't it? Yes, sorry, I meant mem-ops. >>> In any case, back when I've added xc_set_mem_access_multi() I've also >>> modified struct xen_mem_access_op in the same manner: >>> >>> >> http://xenbits.xenproject.org/gitweb/?p=xen.git;a=commitdiff;h=1ef5056bd6274e >> cbe065387b6cf45657d6d700cd >> >> Oh, nevermind, I think you're referring to the fact that I had back then >> added members to the end of the structure, and so the old layout had >> remained compatible. Point taken. > > Not just that - there you've also introduced a new sub-op. Yes, but by modifying the structure for use with XENMEM_access_op_set_access_multi, it's now also changed for, e.g., XENMEM_access_op_set_access - since it's common to them. Other than the place where the new data has been added, I believe that the same critique would apply to the old patch, unless I'm misunderstanding something. Thanks, Razvan
>>> On 20.03.17 at 17:48, <rcojocaru@bitdefender.com> wrote: > On 03/20/2017 06:40 PM, Jan Beulich wrote: >>>>> On 20.03.17 at 17:16, <rcojocaru@bitdefender.com> wrote: >>> On 03/20/2017 06:14 PM, Razvan Cojocaru wrote: >>>> In any case, back when I've added xc_set_mem_access_multi() I've also >>>> modified struct xen_mem_access_op in the same manner: >>>> >>>> >>> http://xenbits.xenproject.org/gitweb/?p=xen.git;a=commitdiff;h=1ef5056bd6274e >>> cbe065387b6cf45657d6d700cd >>> >>> Oh, nevermind, I think you're referring to the fact that I had back then >>> added members to the end of the structure, and so the old layout had >>> remained compatible. Point taken. >> >> Not just that - there you've also introduced a new sub-op. > > Yes, but by modifying the structure for use with > XENMEM_access_op_set_access_multi, it's now also changed for, e.g., > XENMEM_access_op_set_access - since it's common to them. Other than the > place where the new data has been added, I believe that the same > critique would apply to the old patch, unless I'm misunderstanding > something. Indeed, strictly speaking that change wasn't really okay either, as someone passing the smaller structure near the end of a page might get -EFAULT back. So we're trying to do better this time ... Jan
On 03/20/2017 06:54 PM, Jan Beulich wrote: >>>> On 20.03.17 at 17:48, <rcojocaru@bitdefender.com> wrote: >> On 03/20/2017 06:40 PM, Jan Beulich wrote: >>>>>> On 20.03.17 at 17:16, <rcojocaru@bitdefender.com> wrote: >>>> On 03/20/2017 06:14 PM, Razvan Cojocaru wrote: >>>>> In any case, back when I've added xc_set_mem_access_multi() I've also >>>>> modified struct xen_mem_access_op in the same manner: >>>>> >>>>> >>>> http://xenbits.xenproject.org/gitweb/?p=xen.git;a=commitdiff;h=1ef5056bd6274e >>>> cbe065387b6cf45657d6d700cd >>>> >>>> Oh, nevermind, I think you're referring to the fact that I had back then >>>> added members to the end of the structure, and so the old layout had >>>> remained compatible. Point taken. >>> >>> Not just that - there you've also introduced a new sub-op. >> >> Yes, but by modifying the structure for use with >> XENMEM_access_op_set_access_multi, it's now also changed for, e.g., >> XENMEM_access_op_set_access - since it's common to them. Other than the >> place where the new data has been added, I believe that the same >> critique would apply to the old patch, unless I'm misunderstanding >> something. > > Indeed, strictly speaking that change wasn't really okay either, > as someone passing the smaller structure near the end of a page > might get -EFAULT back. So we're trying to do better this time ... Two question remain to be answered then: 1. How should this proceed? Andrew's comment, taken together with Tamas' last patch ("altp2m: Allow specifying external-only use-case") would seem to imply that the best way forward is to revert to the previous patch which duplicates the xc_set_mem_access_multi()'s mem-op with xc_altp2m_set_mem_access_multi()'s HVMOP (with the compat code to be worked out). Is there a consensus on this? 2. What is the best way to avoid incompatibilities such as the one mentioned? For example, in this case, should I have deprecated XENMEM_access_op_set_access and XENMEM_access_op_set_access_multi and added XENMEM_access_op_set_access2 and XENMEM_access_op_set_access2? Or do you have something different in mind? Thanks, Razvan
On Mon, Mar 20, 2017 at 2:20 PM, Razvan Cojocaru <rcojocaru@bitdefender.com> wrote: > On 03/20/2017 06:54 PM, Jan Beulich wrote: > >>>> On 20.03.17 at 17:48, <rcojocaru@bitdefender.com> wrote: > >> On 03/20/2017 06:40 PM, Jan Beulich wrote: > >>>>>> On 20.03.17 at 17:16, <rcojocaru@bitdefender.com> wrote: > >>>> On 03/20/2017 06:14 PM, Razvan Cojocaru wrote: > >>>>> In any case, back when I've added xc_set_mem_access_multi() I've also > >>>>> modified struct xen_mem_access_op in the same manner: > >>>>> > >>>>> > >>>> http://xenbits.xenproject.org/gitweb/?p=xen.git;a= > commitdiff;h=1ef5056bd6274e > >>>> cbe065387b6cf45657d6d700cd > >>>> > >>>> Oh, nevermind, I think you're referring to the fact that I had back > then > >>>> added members to the end of the structure, and so the old layout had > >>>> remained compatible. Point taken. > >>> > >>> Not just that - there you've also introduced a new sub-op. > >> > >> Yes, but by modifying the structure for use with > >> XENMEM_access_op_set_access_multi, it's now also changed for, e.g., > >> XENMEM_access_op_set_access - since it's common to them. Other than the > >> place where the new data has been added, I believe that the same > >> critique would apply to the old patch, unless I'm misunderstanding > >> something. > > > > Indeed, strictly speaking that change wasn't really okay either, > > as someone passing the smaller structure near the end of a page > > might get -EFAULT back. So we're trying to do better this time ... > > Two question remain to be answered then: > > 1. How should this proceed? Andrew's comment, taken together with Tamas' > last patch ("altp2m: Allow specifying external-only use-case") would > seem to imply that the best way forward is to revert to the previous > patch which duplicates the xc_set_mem_access_multi()'s mem-op with > xc_altp2m_set_mem_access_multi()'s HVMOP (with the compat code to be > worked out). Is there a consensus on this? > > 2. What is the best way to avoid incompatibilities such as the one > mentioned? For example, in this case, should I have deprecated > XENMEM_access_op_set_access and XENMEM_access_op_set_access_multi and > added XENMEM_access_op_set_access2 and XENMEM_access_op_set_access2? Or > do you have something different in mind? > IMHO deprecating the old memops shouldn't be needed. The code on the hypervisor side all lands in the same spot, so keeping both memops (and the hvmop) in place should add no extra work from a maintenance perspective. Tamas
>>> On 20.03.17 at 21:20, <rcojocaru@bitdefender.com> wrote: > On 03/20/2017 06:54 PM, Jan Beulich wrote: >>>>> On 20.03.17 at 17:48, <rcojocaru@bitdefender.com> wrote: >>> On 03/20/2017 06:40 PM, Jan Beulich wrote: >>>>>>> On 20.03.17 at 17:16, <rcojocaru@bitdefender.com> wrote: >>>>> On 03/20/2017 06:14 PM, Razvan Cojocaru wrote: >>>>>> In any case, back when I've added xc_set_mem_access_multi() I've also >>>>>> modified struct xen_mem_access_op in the same manner: >>>>>> >>>>>> >>>>> > http://xenbits.xenproject.org/gitweb/?p=xen.git;a=commitdiff;h=1ef5056bd6274e > >>>>> cbe065387b6cf45657d6d700cd >>>>> >>>>> Oh, nevermind, I think you're referring to the fact that I had back then >>>>> added members to the end of the structure, and so the old layout had >>>>> remained compatible. Point taken. >>>> >>>> Not just that - there you've also introduced a new sub-op. >>> >>> Yes, but by modifying the structure for use with >>> XENMEM_access_op_set_access_multi, it's now also changed for, e.g., >>> XENMEM_access_op_set_access - since it's common to them. Other than the >>> place where the new data has been added, I believe that the same >>> critique would apply to the old patch, unless I'm misunderstanding >>> something. >> >> Indeed, strictly speaking that change wasn't really okay either, >> as someone passing the smaller structure near the end of a page >> might get -EFAULT back. So we're trying to do better this time ... > > Two question remain to be answered then: > > 1. How should this proceed? Andrew's comment, taken together with Tamas' > last patch ("altp2m: Allow specifying external-only use-case") would > seem to imply that the best way forward is to revert to the previous > patch which duplicates the xc_set_mem_access_multi()'s mem-op with > xc_altp2m_set_mem_access_multi()'s HVMOP (with the compat code to be > worked out). Is there a consensus on this? To reach consensus we first need to get a firm understanding of the requirements of all interested parties. With Ravi not responding, I've added Paul in the hopes that he may be able to track down who in Intel is currently able to represent their altp2m intentions. > 2. What is the best way to avoid incompatibilities such as the one > mentioned? For example, in this case, should I have deprecated > XENMEM_access_op_set_access and XENMEM_access_op_set_access_multi and > added XENMEM_access_op_set_access2 and XENMEM_access_op_set_access2? Or > do you have something different in mind? Indeed it looks like there would be no way around introducing a new sub-op. We should then take the opportunity and make it extensible from the beginning, unless we all agree to it not being a problem to possibly have to introduce further new sub- ops down the road. Whether such introduction implies deprecation of the old ones is a separate decision. Jan
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 2d97d36..10d80b3 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1923,9 +1923,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); @@ -1956,9 +1953,8 @@ int xc_mem_paging_load(xc_interface *xch, domid_t domain_id, * Allowed types are XENMEM_access_default, XENMEM_access_n, any combination of * XENMEM_access_ + (rwx), and XENMEM_access_rx2rw */ -int xc_set_mem_access(xc_interface *xch, domid_t domain_id, - xenmem_access_t access, uint64_t first_pfn, - uint32_t nr); +int xc_set_mem_access(xc_interface *xch, domid_t domain_id, uint16_t view_id, + xenmem_access_t access, uint64_t first_pfn, uint32_t nr); /* * Set an array of pages to their respective access in the access array. @@ -1966,7 +1962,7 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id, * The same allowed access types as for xc_set_mem_access() apply. */ int xc_set_mem_access_multi(xc_interface *xch, domid_t domain_id, - uint8_t *access, uint64_t *pages, + uint16_t view_id, uint8_t *access, uint64_t *pages, uint32_t nr); /* 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 9536635..4242527 100644 --- a/tools/libxc/xc_mem_access.c +++ b/tools/libxc/xc_mem_access.c @@ -25,17 +25,19 @@ int xc_set_mem_access(xc_interface *xch, domid_t domain_id, + uint16_t view_id, xenmem_access_t access, uint64_t first_pfn, uint32_t nr) { 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, + .view_id = view_id }; return do_memory_op(xch, XENMEM_access_op, &mao, sizeof(mao)); @@ -43,6 +45,7 @@ int xc_set_mem_access(xc_interface *xch, int xc_set_mem_access_multi(xc_interface *xch, domid_t domain_id, + uint16_t view_id, uint8_t *access, uint64_t *pages, uint32_t nr) @@ -59,6 +62,7 @@ int xc_set_mem_access_multi(xc_interface *xch, .access = XENMEM_access_default + 1, /* Invalid value */ .pfn = ~0UL, /* Invalid GFN */ .nr = nr, + .view_id = view_id, }; if ( xc_hypercall_bounce_pre(xch, pages) || diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c index 9d4f957..bd348ef 100644 --- a/tools/tests/xen-access/xen-access.c +++ b/tools/tests/xen-access/xen-access.c @@ -475,9 +475,6 @@ int main(int argc, char *argv[]) /* With altp2m we just create a new, restricted view of the memory */ if ( memaccess && altp2m ) { - xen_pfn_t gfn = 0; - unsigned long perm_set = 0; - rc = xc_altp2m_set_domain_state( xch, domain_id, 1 ); if ( rc < 0 ) { @@ -495,15 +492,9 @@ 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_set_mem_access(xch, domain_id, altp2m_view_id, default_access, + 0, xenaccess->max_gpfn); + DPRINTF("done!"); rc = xc_altp2m_switch_to_view( xch, domain_id, altp2m_view_id ); if ( rc < 0 ) @@ -523,14 +514,14 @@ int main(int argc, char *argv[]) if ( memaccess && !altp2m ) { /* Set the default access type and convert all pages to it */ - rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0); + rc = xc_set_mem_access(xch, domain_id, 0, default_access, ~0ull, 0); if ( rc < 0 ) { ERROR("Error %d setting default mem access type\n", rc); goto exit; } - rc = xc_set_mem_access(xch, domain_id, default_access, START_PFN, + rc = xc_set_mem_access(xch, domain_id, 0, default_access, START_PFN, (xenaccess->max_gpfn - START_PFN) ); if ( rc < 0 ) @@ -606,8 +597,8 @@ int main(int argc, char *argv[]) rc = xc_altp2m_set_domain_state(xch, domain_id, 0); rc = xc_monitor_singlestep(xch, domain_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, + rc = xc_set_mem_access(xch, domain_id, 0, XENMEM_access_rwx, ~0ull, 0); + rc = xc_set_mem_access(xch, domain_id, 0, XENMEM_access_rwx, START_PFN, (xenaccess->max_gpfn - START_PFN) ); } @@ -685,7 +676,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, + rc = xc_set_mem_access(xch, domain_id, 0, after_first_access, req.u.mem_access.gfn, 1); if (rc < 0) { diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 0282986..863ac74 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4343,7 +4343,6 @@ static int do_altp2m_op( case HVMOP_altp2m_create_p2m: case HVMOP_altp2m_destroy_p2m: case HVMOP_altp2m_switch_p2m: - case HVMOP_altp2m_set_mem_access: case HVMOP_altp2m_change_gfn: break; default: @@ -4451,15 +4450,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_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0, - a.u.set_mem_access.hvmmem_access, - a.u.set_mem_access.view); - break; - case HVMOP_altp2m_change_gfn: if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) rc = -EINVAL; diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c index 19f63bb..35afb1c 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, 0); + MEMOP_CMD_MASK, mao.access, mao.view_id); if ( rc > 0 ) { ASSERT(!(rc & MEMOP_CMD_MASK)); @@ -78,7 +78,7 @@ int mem_access_memop(unsigned long cmd, case XENMEM_access_op_set_access_multi: rc = p2m_set_mem_access_multi(d, mao.pfn_list, mao.access_list, mao.nr, - start_iter, MEMOP_CMD_MASK, 0); + start_iter, MEMOP_CMD_MASK, mao.view_id); if ( rc > 0 ) { ASSERT(!(rc & MEMOP_CMD_MASK)); diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index bc00ef0..cf6f24f 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -219,18 +219,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; @@ -258,7 +246,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 @@ -269,7 +257,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 6eee0c8..07ccc2a 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -444,6 +444,8 @@ struct xen_mem_access_op { /* xenmem_access_t */ uint8_t access; domid_t domid; + uint16_t view_id; + uint16_t pad[3]; /* * Number of pages for set op (or size of pfn_list for * XENMEM_access_op_set_access_multi)
xc_altp2m_set_mem_access() and xc_set_mem_access() end up doing the same thing in the hypervisor, but the former is a HVMOP and the latter a DOMCTL. Since nobody is currently using, or has stated intent to use, this functionality specifically as an HVMOP, this patch removes the HVMOP while adding an extra parameter to the more flexible DOMCTL variant, in which the altp2m view can be transmitted (0 for the default view, or when altp2m is disabled). The xen-access test has been updated in the process. Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com> --- Changes since V1: - Removed XEN_DOMCTL_INTERFACE_VERSION bump (as the "x86: remove PVHv1 code" already does this for this release). - Replaced uint16_t pad; with uint16_t pad[3]; as suggested by Tamas. --- tools/libxc/include/xenctrl.h | 10 +++------- tools/libxc/xc_altp2m.c | 25 ------------------------- tools/libxc/xc_mem_access.c | 14 +++++++++----- tools/tests/xen-access/xen-access.c | 25 ++++++++----------------- xen/arch/x86/hvm/hvm.c | 10 ---------- xen/common/mem_access.c | 4 ++-- xen/include/public/hvm/hvm_op.h | 15 +-------------- xen/include/public/memory.h | 2 ++ 8 files changed, 25 insertions(+), 80 deletions(-)