| Message ID | 1507218137-29833-1-git-send-email-ppircalabu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote: > @@ -4451,6 +4453,7 @@ static int do_altp2m_op( > case HVMOP_altp2m_destroy_p2m: > case HVMOP_altp2m_switch_p2m: > case HVMOP_altp2m_set_mem_access: > + case HVMOP_altp2m_set_mem_access_multi: Was it agreed that this, just like others (many wrongly, I think) is supposed to be invokable by the affected domain itself? Its non- altp2m counterpart is a DM_PRIV operation. If the one here is meant to be different, I think the commit message should say why. > @@ -4568,6 +4571,30 @@ 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; > + } > + rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list, > + a.u.set_mem_access_multi.access_list, > + a.u.set_mem_access_multi.nr, > + a.u.set_mem_access_multi.opaque, > + MEMOP_CMD_MASK, > + a.u.set_mem_access_multi.view); > + if ( rc > 0 ) > + { > + a.u.set_mem_access_multi.opaque = rc; > + if ( __copy_to_guest(arg, &a, 1) ) __copy_field_to_guest() would suffice here afaict. > @@ -4586,6 +4613,53 @@ static int do_altp2m_op( > return rc; > } > > +static int compat_altp2m_op( > + XEN_GUEST_HANDLE_PARAM(void) arg) > +{ > + struct compat_hvm_altp2m_op a; > + union > + { > + XEN_GUEST_HANDLE_PARAM(void) hnd; > + struct xen_hvm_altp2m_op *altp2m_op; > + } nat; > + > + if ( !hvm_altp2m_supported() ) > + return -EOPNOTSUPP; > + > + if ( copy_from_guest(&a, arg, 1) ) > + return -EFAULT; > + > + if ( a.pad1 || a.pad2 || > + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ) > + return -EINVAL; Why doesn't suffice what do_altp2m_op() does? > + set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); > + > + switch ( a.cmd ) > + { > + case HVMOP_altp2m_set_mem_access_multi: > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \ > + guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list) > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \ > + guest_from_compat_handle((_d_)->access_list, (_s_)->access_list) > + XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op->u.set_mem_access_multi, > + &a.u.set_mem_access_multi); > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list > + break; > + default: > + return do_altp2m_op(arg); > + } > + > + nat.altp2m_op->version = a.version; > + nat.altp2m_op->cmd = a.cmd; > + nat.altp2m_op->domain = a.domain; > + nat.altp2m_op->pad1 = a.pad1; > + nat.altp2m_op->pad2 = a.pad2; Why do you do this by hand, rather than using XLAT_*() macros which at the same time check that the field sizes match? > @@ -4733,7 +4807,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) > break; > > case HVMOP_altp2m: > - rc = do_altp2m_op(arg); > + rc = ( current->hcall_compat ) ? compat_altp2m_op(arg) : do_altp2m_op(arg); Pointless parentheses and spaces. Plus, to be honest, I'm not really happy about this ad hoc compat handling, but at the same time I can't suggest a truly better alternative. > --- a/xen/include/xlat.lst > +++ b/xen/include/xlat.lst > @@ -73,6 +73,7 @@ > ? vcpu_hvm_context hvm/hvm_vcpu.h > ? vcpu_hvm_x86_32 hvm/hvm_vcpu.h > ? vcpu_hvm_x86_64 hvm/hvm_vcpu.h > +! hvm_altp2m_set_mem_access_multi hvm/hvm_op.h Please insert alphabetically, sorted by filename (and then structure name). > --- a/xen/tools/compat-build-header.py > +++ b/xen/tools/compat-build-header.py > @@ -16,6 +16,7 @@ pats = [ > [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ], > [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3" ], > [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ], > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ], What is this needed for? I can't find any instance of HVMMEM_* elsewhere in the patch. As you can see, so far there are only pretty generic tokens being replaced here. Jan
On 10/06/2017 06:34 PM, Jan Beulich wrote: >>>> On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote: >> @@ -4451,6 +4453,7 @@ static int do_altp2m_op( >> case HVMOP_altp2m_destroy_p2m: >> case HVMOP_altp2m_switch_p2m: >> case HVMOP_altp2m_set_mem_access: >> + case HVMOP_altp2m_set_mem_access_multi: > > Was it agreed that this, just like others (many wrongly, I think) is > supposed to be invokable by the affected domain itself? Its non- > altp2m counterpart is a DM_PRIV operation. If the one here is > meant to be different, I think the commit message should say why. In the absence of an answer from the designers of altp2m, we've chosen to remain consistent with HVMOP_altp2m_set_mem_access - since that is allowed to be invoked by the domain itself, this operation is also allowed to do that. Back in March, I've sent a DOMCTL version: https://patchwork.kernel.org/patch/9633615/ and a HVMOP version (minus the compat part): https://patchwork.kernel.org/patch/9612799/ It has been discussed, and an authoritative answer on the design of this was sought out, but despite several kind reminders during this time, it never came. At this point, the least modification to the initial design appears to be to keep the new operation as a HVMOP. This is an important optimization, and the waiting period for objections has surely been reasonable. Thanks, Razvan
>>> On 06.10.17 at 18:07, <rcojocaru@bitdefender.com> wrote: > On 10/06/2017 06:34 PM, Jan Beulich wrote: >>>>> On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote: >>> @@ -4451,6 +4453,7 @@ static int do_altp2m_op( >>> case HVMOP_altp2m_destroy_p2m: >>> case HVMOP_altp2m_switch_p2m: >>> case HVMOP_altp2m_set_mem_access: >>> + case HVMOP_altp2m_set_mem_access_multi: >> >> Was it agreed that this, just like others (many wrongly, I think) is >> supposed to be invokable by the affected domain itself? Its non- >> altp2m counterpart is a DM_PRIV operation. If the one here is >> meant to be different, I think the commit message should say why. > > In the absence of an answer from the designers of altp2m, we've chosen > to remain consistent with HVMOP_altp2m_set_mem_access - since that is > allowed to be invoked by the domain itself, this operation is also > allowed to do that. > > Back in March, I've sent a DOMCTL version: > > https://patchwork.kernel.org/patch/9633615/ > > and a HVMOP version (minus the compat part): > > https://patchwork.kernel.org/patch/9612799/ > > It has been discussed, and an authoritative answer on the design of this > was sought out, but despite several kind reminders during this time, it > never came. At this point, the least modification to the initial design > appears to be to keep the new operation as a HVMOP. This is an important > optimization, and the waiting period for objections has surely been > reasonable. Okay, this is (sort of) fine, but especially when there was no feedback the decision taken (and its reason) should be recorded in the commit message. As stated above as well as earlier, I strongly think that the altp2m permissions are too lax right now (and hence the patch here widens the problem, but I can agree that making it match set_mem_access is not unreasonable). Jan
On 09.10.2017 10:23, Jan Beulich wrote: >>>> On 06.10.17 at 18:07, <rcojocaru@bitdefender.com> wrote: >> On 10/06/2017 06:34 PM, Jan Beulich wrote: >>>>>> On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote: >>>> @@ -4451,6 +4453,7 @@ static int do_altp2m_op( >>>> case HVMOP_altp2m_destroy_p2m: >>>> case HVMOP_altp2m_switch_p2m: >>>> case HVMOP_altp2m_set_mem_access: >>>> + case HVMOP_altp2m_set_mem_access_multi: >>> >>> Was it agreed that this, just like others (many wrongly, I think) is >>> supposed to be invokable by the affected domain itself? Its non- >>> altp2m counterpart is a DM_PRIV operation. If the one here is >>> meant to be different, I think the commit message should say why. >> >> In the absence of an answer from the designers of altp2m, we've chosen >> to remain consistent with HVMOP_altp2m_set_mem_access - since that is >> allowed to be invoked by the domain itself, this operation is also >> allowed to do that. >> >> Back in March, I've sent a DOMCTL version: >> >> https://patchwork.kernel.org/patch/9633615/ >> >> and a HVMOP version (minus the compat part): >> >> https://patchwork.kernel.org/patch/9612799/ >> >> It has been discussed, and an authoritative answer on the design of this >> was sought out, but despite several kind reminders during this time, it >> never came. At this point, the least modification to the initial design >> appears to be to keep the new operation as a HVMOP. This is an important >> optimization, and the waiting period for objections has surely been >> reasonable. > > Okay, this is (sort of) fine, but especially when there was no > feedback the decision taken (and its reason) should be recorded > in the commit message. As stated above as well as earlier, I > strongly think that the altp2m permissions are too lax right now > (and hence the patch here widens the problem, but I can agree > that making it match set_mem_access is not unreasonable). Of course. We'll update the commit message. Thanks for the review! Razvan
On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote: > > > > > > > > > > > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote: > > @@ -4451,6 +4453,7 @@ static int do_altp2m_op( > > case HVMOP_altp2m_destroy_p2m: > > case HVMOP_altp2m_switch_p2m: > > case HVMOP_altp2m_set_mem_access: > > + case HVMOP_altp2m_set_mem_access_multi: > Was it agreed that this, just like others (many wrongly, I think) is > supposed to be invokable by the affected domain itself? Its non- > altp2m counterpart is a DM_PRIV operation. If the one here is > meant to be different, I think the commit message should say why. Will be corrected in the new patch iteration. > > > > > @@ -4568,6 +4571,30 @@ 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; > > + } > > + rc = p2m_set_mem_access_multi(d, > > a.u.set_mem_access_multi.pfn_list, > > + a.u.set_mem_access_multi.acc > > ess_list, > > + a.u.set_mem_access_multi.nr, > > + a.u.set_mem_access_multi.opa > > que, > > + MEMOP_CMD_MASK, > > + a.u.set_mem_access_multi.vie > > w); > > + if ( rc > 0 ) > > + { > > + a.u.set_mem_access_multi.opaque = rc; > > + if ( __copy_to_guest(arg, &a, 1) ) > __copy_field_to_guest() would suffice here afaict. Will be corrected in the new patch iteration. > > > > > @@ -4586,6 +4613,53 @@ static int do_altp2m_op( > > return rc; > > } > > > > +static int compat_altp2m_op( > > + XEN_GUEST_HANDLE_PARAM(void) arg) > > +{ > > + struct compat_hvm_altp2m_op a; > > + union > > + { > > + XEN_GUEST_HANDLE_PARAM(void) hnd; > > + struct xen_hvm_altp2m_op *altp2m_op; > > + } nat; > > + > > + if ( !hvm_altp2m_supported() ) > > + return -EOPNOTSUPP; > > + > > + if ( copy_from_guest(&a, arg, 1) ) > > + return -EFAULT; > > + > > + if ( a.pad1 || a.pad2 || > > + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ) > > + return -EINVAL; > Why doesn't suffice what do_altp2m_op() does? The sanity checks are the same as for do_altp2m_op but it wanted to check as early as possible for invalid arguments. > > > > > + set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); > > + > > + switch ( a.cmd ) > > + { > > + case HVMOP_altp2m_set_mem_access_multi: > > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, > > _s_); \ > > + guest_from_compat_handle((_d_)->pfn_list, (_s_)- > > >pfn_list) > > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, > > _s_); \ > > + guest_from_compat_handle((_d_)->access_list, (_s_)- > > >access_list) > > + XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op- > > >u.set_mem_access_multi, > > + &a.u.set_mem_access_multi); > > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list > > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list > > + break; > > + default: > > + return do_altp2m_op(arg); > > + } > > + > > + nat.altp2m_op->version = a.version; > > + nat.altp2m_op->cmd = a.cmd; > > + nat.altp2m_op->domain = a.domain; > > + nat.altp2m_op->pad1 = a.pad1; > > + nat.altp2m_op->pad2 = a.pad2; > Why do you do this by hand, rather than using XLAT_*() macros > which at the same time check that the field sizes match? Actually, there is a problem with the XLAT_hvm_altp2m_op macro generation. The current definition of struct xen_hvm_altp2m_op uses "structs" as member of the union. In this case the enum values for switch(u) are not generated. #define XLAT_hvm_altp2m_op(_d_, _s_) do { \ (_d_)->version = (_s_)->version; \ (_d_)->cmd = (_s_)->cmd; \ (_d_)->domain = (_s_)->dWill be corrected in the new patch iteration.omain; \ (_d_)->pad1 = (_s_)->pad1; \ (_d_)->pad2 = (_s_)->pad2; \ switch (u) { \ case XLAT_hvm_altp2m_op_u_domain_state: \ XLAT_hvm_altp2m_domain_state(&(_d_)->u.domain_state, &(_s_)- >u.domain_state); \ break; \ case XLAT_hvm_alxen_hvm_altp2m_set_mem_access_multi_ttp2m_op_u_enable_notify : \ XLAT_hvm_altp2m_vcpu_enable_notify(&(_d_)->u.enable_notify, &(_s_)->u.enable_notifyxen_hvm_altp2m_set_mem_access_multi_t); \ break; \ case XLAT_hvm_altp2m_op_u_view: \ XLAT_hvm_altp2m_view(&(_d_)->u.view, &(_s_)->u.view); \ break; \Will be corrected in the new patch iteration. case XLAT_hvm_altp2m_op_u_set_mem_access: \ XLAT_hvm_altp2m_set_mem_access(&(_d_)->u.set_mem_access, &(_s_)->u.set_mem_access); \ break; \ case XLAT_hvm_altp2m_op_u_change_gfn: \ XLAT_hvm_altp2m_change_gfn(&(_d_)->u.change_gfn, &(_s_)- >u.change_gfn); \ break; \ case XLAT_hvm_altp2m_op_u_set_mem_access_multi: \ XLAT_hvm_altp2m_set_mem_access_multi(&(_d_)- >u.set_mem_access_multi, &(_s_)->u.set_mem_access_multi); \ break; \ case XLAT_hvm_altp2m_op_u_pad: \ (_d_)->u.pad = (_s_)->u.pad; \ break; \ } \ } while (0) If the "structs" are replaced with the corresponding typedefs in the definition of xen_hvm_altp2m_op (e.g. xen_hvm_altp2m_set_mem_access_multi_t instead of struct xen_hvm_altp2m_domain_state), the enum values are generated correctly but the XLAT_hvm_altp2m_set_mem_access_multi macro is replaced with a simple assignment, thus breaking the build ( compat_hvm_altp2m_set_mem_access_multi_t to xen_hvm_altp2m_set_mem_access_multi_t assignment) enum XLAT_hvm_altp2m_op_u { XLAT_hvm_altp2m_op_u_domain_state, XLAT_hvm_altp2m_op_u_enable_notify, XLAT_hvm_altp2m_op_u_view, XLAT_hvm_altp2m_op_u_set_mem_access, XLAT_hvm_altp2m_op_u_change_gfn, XLAT_hvm_altp2m_op_u_set_mem_access_multi, XLAT_hvm_altp2m_op_u_pad, }; #define XLAT_hvm_altp2m_op(_d_, _s_) do { \ (_d_)->version = (_s_)->version; \ (_d_)->cmd = (_s_)->cmd; \ (_d_)->domain = (_s_)->domain; \ (_d_)->pad1 = (_s_)->pad1; \ (_d_)->pad2 = (_s_)->pad2; \ switch (u) { \ case XLAT_hvm_altp2m_op_u_domain_state: \ (_d_)->u.domain_state = (_s_)->u.domain_state; \ break; \ case XLAT_hvm_altp2m_op_u_enable_notify: \ (_d_)->u.enable_notify = (_s_)->u.enable_notify; \ break; \ case XLAT_hvm_altp2m_op_u_view: \ (_d_)->u.view = (_s_)->u.view; \ break; \ case XLAT_hvm_altp2m_op_u_set_mem_access: \ (_d_)->u.set_mem_access = (_s_)->u.set_mem_access; \ break; \ case XLAT_hvm_altp2m_op_u_change_gfn: \ (_d_)->u.change_gfn = (_s_)->u.change_gfn; \ break; \ case XLAT_hvm_altp2m_op_u_set_mem_access_multi: \ (_d_)->u.set_mem_access_multi = (_s_)->u.set_mem_access_multi; \ break; \ case XLAT_hvm_altp2m_op_u_pad: \ (_d_)->u.pad = (_s_)->u.pad; \ break; \ } \ } while (0) At this stage the easiest approach was to set the values by hand. > > > > > @@ -4733,7 +4807,7 @@ long do_hvm_op(unsigned long op, > > XEN_GUEST_HANDLE_PARAM(void) arg) > > break; > > > > case HVMOP_altp2m: > > - rc = do_altp2m_op(arg); > > + rc = ( current->hcall_compat ) ? compat_altp2m_op(arg) : > > do_altp2m_op(arg); > Pointless parentheses and spaces. Plus, to be honest, I'm not really > happy about this ad hoc compat handling, but at the same time I > can't suggest a truly better alternative. > Removed the spaces. The alternative (e.g. changing hvm_hypercall_table to use COMPAT_CALL(hvm_op) and defining a compat_hvm_op function in ch only altp2m is handled differently) is uglier in my opinion because only HVMOP_altp2m_set_mem_access_multi requires COMPAT argument translation. > > > > --- a/xen/include/xlat.lst > > +++ b/xen/include/xlat.lst > > @@ -73,6 +73,7 @@ > > ? vcpu_hvm_context hvm/hvm_vcpu.h > > ? vcpu_hvm_x86_32 hvm/hvm_vcpu.h > > ? vcpu_hvm_x86_64 hvm/hvm_vcpu.h > > +! hvm_altp2m_set_mem_access_multi hvm/hvm_op.h > Please insert alphabetically, sorted by filename (and then structure > name). Will be corrected in the new patch iteration. > > > > > --- a/xen/tools/compat-build-header.py > > +++ b/xen/tools/compat-build-header.py > > @@ -16,6 +16,7 @@ pats = [ > > [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ], > > [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3" > > ], > > [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ], > > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ], > What is this needed for? I can't find any instance of HVMMEM_* > elsewhere in the patch. As you can see, so far there are only > pretty generic tokens being replaced here. > Without this, both hvmmem_type_t and hvmmem_type_compat_t enums will define the same values (e.g. HVMMEM_ram_rw,) resulting in a compile error. By adding this translation we will have a COMPAT_HVMMEM value for each HVMMEM_ value defined in public/hvm/hvm_op.h) (e.g. HVMMEM_ram_rw -> COMPAT_HVMMEM_ram_rw) Many thanks for your support, Petre > Jan > > ________________________ > This email was scanned by Bitdefender
>>> On 09.10.17 at 12:10, <ppircalabu@bitdefender.com> wrote: > On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote: >> > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote: >> > + switch ( a.cmd ) >> > + { >> > + case HVMOP_altp2m_set_mem_access_multi: >> > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, >> > _s_); \ >> > + guest_from_compat_handle((_d_)->pfn_list, (_s_)- >> > >pfn_list) >> > +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, >> > _s_); \ >> > + guest_from_compat_handle((_d_)->access_list, (_s_)- >> > >access_list) >> > + XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op- >> > >u.set_mem_access_multi, >> > + &a.u.set_mem_access_multi); >> > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list >> > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list >> > + break; >> > + default: >> > + return do_altp2m_op(arg); >> > + } >> > + >> > + nat.altp2m_op->version = a.version; >> > + nat.altp2m_op->cmd = a.cmd; >> > + nat.altp2m_op->domain = a.domain; >> > + nat.altp2m_op->pad1 = a.pad1; >> > + nat.altp2m_op->pad2 = a.pad2; >> Why do you do this by hand, rather than using XLAT_*() macros >> which at the same time check that the field sizes match? > Actually, there is a problem with the XLAT_hvm_altp2m_op macro > generation. > The current definition of struct xen_hvm_altp2m_op uses "structs" as > member of the union. In this case the enum values for switch(u) are not > generated. >[...] > If the "structs" are replaced with the corresponding typedefs in the > definition of xen_hvm_altp2m_op > (e.g. xen_hvm_altp2m_set_mem_access_multi_t instead of struct > xen_hvm_altp2m_domain_state), the enum values are generated correctly > but the XLAT_hvm_altp2m_set_mem_access_multi macro is replaced with a > simple assignment, thus breaking the build ( > compat_hvm_altp2m_set_mem_access_multi_t > to xen_hvm_altp2m_set_mem_access_multi_t assignment) >[...] > At this stage the easiest approach was to set the values by hand. Okay, but then please add a comment to say so (after all it should really be the script that gets adjusted in order to correctly deal with the cases) and add the missing size checks (and whatever else verification the macros may do). >> > --- a/xen/tools/compat-build-header.py >> > +++ b/xen/tools/compat-build-header.py >> > @@ -16,6 +16,7 @@ pats = [ >> > [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ], >> > [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3" >> > ], >> > [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ], >> > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ], >> What is this needed for? I can't find any instance of HVMMEM_* >> elsewhere in the patch. As you can see, so far there are only >> pretty generic tokens being replaced here. >> > Without this, both hvmmem_type_t and hvmmem_type_compat_t enums will > define the same values (e.g. HVMMEM_ram_rw,) resulting in a compile > error. By adding this translation we will have a COMPAT_HVMMEM value > for each HVMMEM_ value defined in public/hvm/hvm_op.h) (e.g. > HVMMEM_ram_rw -> COMPAT_HVMMEM_ram_rw) That's ugly. I realize that we shouldn't even attempt to translate enumerations (or to be fully precise, there shouldn't be any enumerations in the public interface in the first place), as the enumerator values ought to remain consistent between native and compat uses. Hence we could either convert the enum to a set of #define-s, or we would need a mechanism to exclude parts of a header from the compat conversion. In the end the problem here is because of the enumerators, other than xenmem_access_t's, aren't properly prefixed with XEN or XEN_ (or else the script would already handle them fine afaict). So another variant of addressing this would be to deprecate (but not remove) the current names, introducing properly named ones for __XEN_INTERFACE_VERSION__ >= 0x00040a00. Jan
On Lu, 2017-10-09 at 04:36 -0600, Jan Beulich wrote: > > > > > > > > > > > > > On 09.10.17 at 12:10, <ppircalabu@bitdefender.com> wrote: > > On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote: > > > > + switch ( a.cmd ) > > > > + { > > > > + case HVMOP_altp2m_set_mem_access_multi: > > > > +#define > > > > XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, > > > > _s_); \ > > > > + guest_from_compat_handle((_d_)->pfn_list, (_s_)- > > > > > > > > > > pfn_list) > > > > +#define > > > > XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, > > > > _s_); \ > > > > + guest_from_compat_handle((_d_)->access_list, > > > > (_s_)- > > > > > > > > > > access_list) > > > > + XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_o > > > > p- > > > > > > > > > > u.set_mem_access_multi, > > > > + &a.u.set_mem_access_multi); > > > > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list > > > > +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list > > > > + break; > > > > + default: > > > > + return do_altp2m_op(arg); > > > > + } > > > > + > > > > + nat.altp2m_op->version = a.version; > > > > + nat.altp2m_op->cmd = a.cmd; > > > > + nat.altp2m_op->domain = a.domain; > > > > + nat.altp2m_op->pad1 = a.pad1; > > > > + nat.altp2m_op->pad2 = a.pad2; > > > Why do you do this by hand, rather than using XLAT_*() macros > > > which at the same time check that the field sizes match? > > Actually, there is a problem with the XLAT_hvm_altp2m_op macro > > generation. > > The current definition of struct xen_hvm_altp2m_op uses "structs" > > as > > member of the union. In this case the enum values for switch(u) are > > not > > generated. > > [...] > > If the "structs" are replaced with the corresponding typedefs in > > the > > definition of xen_hvm_altp2m_op > > (e.g. xen_hvm_altp2m_set_mem_access_multi_t instead of struct > > xen_hvm_altp2m_domain_state), the enum values are generated > > correctly > > but the XLAT_hvm_altp2m_set_mem_access_multi macro is replaced with > > a > > simple assignment, thus breaking the build ( > > compat_hvm_altp2m_set_mem_access_multi_t > > to xen_hvm_altp2m_set_mem_access_multi_t assignment) > > [...] > > At this stage the easiest approach was to set the values by hand. > Okay, but then please add a comment to say so (after all it should > really be the script that gets adjusted in order to correctly deal > with the cases) and add the missing size checks (and whatever > else verification the macros may do). Will fix in in the new patch iteration. > > > > > > > > > > > > > > --- a/xen/tools/compat-build-header.py > > > > +++ b/xen/tools/compat-build-header.py > > > > @@ -16,6 +16,7 @@ pats = [ > > > > [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ], > > > > [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", > > > > r"\1compat_\2_t\3" > > > > ], > > > > [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ], > > > > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ], > > > What is this needed for? I can't find any instance of HVMMEM_* > > > elsewhere in the patch. As you can see, so far there are only > > > pretty generic tokens being replaced here. > > > > > Without this, both hvmmem_type_t and hvmmem_type_compat_t enums > > will > > define the same values (e.g. HVMMEM_ram_rw,) resulting in a > > compile > > error. By adding this translation we will have a COMPAT_HVMMEM > > value > > for each HVMMEM_ value defined in public/hvm/hvm_op.h) (e.g. > > HVMMEM_ram_rw -> COMPAT_HVMMEM_ram_rw) > That's ugly. I realize that we shouldn't even attempt to translate > enumerations (or to be fully precise, there shouldn't be any > enumerations in the public interface in the first place), as the > enumerator values ought to remain consistent between native > and compat uses. Hence we could either convert the enum to a > set of #define-s, or we would need a mechanism to exclude > parts of a header from the compat conversion. > > In the end the problem here is because of the enumerators, > other than xenmem_access_t's, aren't properly prefixed with > XEN or XEN_ (or else the script would already handle them fine > afaict). So another variant of addressing this would be to > deprecate (but not remove) the current names, introducing > properly named ones for __XEN_INTERFACE_VERSION__ >= > 0x00040a00. > Unfortunately the enum is referenced also in other functions (e.g. xendevicemodel_set_mem_type) so replacing it with #defines would be more difficult. When generating the compat headers,-DXEN_GENERATING_COMPAT_HEADERS is defined (xen/include/Makefile). I can guard hvmmem_type_t with an #ifndef XEN_GENERATING_COMPAT_HEADERS so the enum is not processed by the compat-build-header.py script. (in my opinion this is the minimum- impact approach) Do you agree with this? Many thanks, Petre > Jan > > ________________________ > This email was scanned by Bitdefender
>>> On 09.10.17 at 13:19, <ppircalabu@bitdefender.com> wrote: > On Lu, 2017-10-09 at 04:36 -0600, Jan Beulich wrote: >> > > > On 09.10.17 at 12:10, <ppircalabu@bitdefender.com> wrote: >> > On Vi, 2017-10-06 at 09:34 -0600, Jan Beulich wrote: >> > > > > > On 05.10.17 at 17:42, <ppircalabu@bitdefender.com> wrote: >> > > > --- a/xen/tools/compat-build-header.py >> > > > +++ b/xen/tools/compat-build-header.py >> > > > @@ -16,6 +16,7 @@ pats = [ >> > > > [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ], >> > > > [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", >> > > > r"\1compat_\2_t\3" >> > > > ], >> > > > [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ], >> > > > + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ], >> > > What is this needed for? I can't find any instance of HVMMEM_* >> > > elsewhere in the patch. As you can see, so far there are only >> > > pretty generic tokens being replaced here. >> > > >> > Without this, both hvmmem_type_t and hvmmem_type_compat_t enums >> > will >> > define the same values (e.g. HVMMEM_ram_rw,) resulting in a >> > compile >> > error. By adding this translation we will have a COMPAT_HVMMEM >> > value >> > for each HVMMEM_ value defined in public/hvm/hvm_op.h) (e.g. >> > HVMMEM_ram_rw -> COMPAT_HVMMEM_ram_rw) >> That's ugly. I realize that we shouldn't even attempt to translate >> enumerations (or to be fully precise, there shouldn't be any >> enumerations in the public interface in the first place), as the >> enumerator values ought to remain consistent between native >> and compat uses. Hence we could either convert the enum to a >> set of #define-s, or we would need a mechanism to exclude >> parts of a header from the compat conversion. >> >> In the end the problem here is because of the enumerators, >> other than xenmem_access_t's, aren't properly prefixed with >> XEN or XEN_ (or else the script would already handle them fine >> afaict). So another variant of addressing this would be to >> deprecate (but not remove) the current names, introducing >> properly named ones for __XEN_INTERFACE_VERSION__ >= >> 0x00040a00. >> > Unfortunately the enum is referenced also in other functions > (e.g. xendevicemodel_set_mem_type) so replacing it with #defines would > be more difficult. > When generating the compat headers,-DXEN_GENERATING_COMPAT_HEADERS is > defined (xen/include/Makefile). I can guard hvmmem_type_t with an > #ifndef XEN_GENERATING_COMPAT_HEADERS so the enum is not processed by > the compat-build-header.py script. (in my opinion this is the minimum- > impact approach) > Do you agree with this? I'm not entirely happy about that approach, but it'll do for the moment. Jan
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 3bcab3c..4e2ce64 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1971,6 +1971,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); 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 205b4cb..1f4358c 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -73,6 +73,8 @@ #include <public/arch-x86/cpuid.h> #include <asm/cpuid.h> +#include <compat/hvm/hvm_op.h> + bool_t __read_mostly hvm_enabled; #ifdef DBG_LEVEL_0 @@ -4451,6 +4453,7 @@ static int do_altp2m_op( case HVMOP_altp2m_destroy_p2m: case HVMOP_altp2m_switch_p2m: case HVMOP_altp2m_set_mem_access: + case HVMOP_altp2m_set_mem_access_multi: case HVMOP_altp2m_change_gfn: break; default: @@ -4568,6 +4571,30 @@ 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; + } + rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list, + a.u.set_mem_access_multi.access_list, + a.u.set_mem_access_multi.nr, + a.u.set_mem_access_multi.opaque, + MEMOP_CMD_MASK, + a.u.set_mem_access_multi.view); + if ( rc > 0 ) + { + a.u.set_mem_access_multi.opaque = rc; + if ( __copy_to_guest(arg, &a, 1) ) + rc = -EFAULT; + else + rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh", + HVMOP_altp2m, arg); + } + break; + case HVMOP_altp2m_change_gfn: if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 ) rc = -EINVAL; @@ -4586,6 +4613,53 @@ static int do_altp2m_op( return rc; } +static int compat_altp2m_op( + XEN_GUEST_HANDLE_PARAM(void) arg) +{ + struct compat_hvm_altp2m_op a; + union + { + XEN_GUEST_HANDLE_PARAM(void) hnd; + struct xen_hvm_altp2m_op *altp2m_op; + } nat; + + if ( !hvm_altp2m_supported() ) + return -EOPNOTSUPP; + + if ( copy_from_guest(&a, arg, 1) ) + return -EFAULT; + + if ( a.pad1 || a.pad2 || + (a.version != HVMOP_ALTP2M_INTERFACE_VERSION) ) + return -EINVAL; + + set_xen_guest_handle(nat.hnd, COMPAT_ARG_XLAT_VIRT_BASE); + + switch ( a.cmd ) + { + case HVMOP_altp2m_set_mem_access_multi: +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list(_d_, _s_); \ + guest_from_compat_handle((_d_)->pfn_list, (_s_)->pfn_list) +#define XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list(_d_, _s_); \ + guest_from_compat_handle((_d_)->access_list, (_s_)->access_list) + XLAT_hvm_altp2m_set_mem_access_multi(&nat.altp2m_op->u.set_mem_access_multi, + &a.u.set_mem_access_multi); +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_pfn_list +#undef XLAT_hvm_altp2m_set_mem_access_multi_HNDL_access_list + break; + default: + return do_altp2m_op(arg); + } + + nat.altp2m_op->version = a.version; + nat.altp2m_op->cmd = a.cmd; + nat.altp2m_op->domain = a.domain; + nat.altp2m_op->pad1 = a.pad1; + nat.altp2m_op->pad2 = a.pad2; + + return do_altp2m_op(nat.hnd); +} + static int hvmop_get_mem_type( XEN_GUEST_HANDLE_PARAM(xen_hvm_get_mem_type_t) arg) { @@ -4733,7 +4807,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) break; case HVMOP_altp2m: - rc = do_altp2m_op(arg); + rc = ( current->hcall_compat ) ? compat_altp2m_op(arg) : do_altp2m_op(arg); break; default: diff --git a/xen/include/Makefile b/xen/include/Makefile index c90fdee..814b0a8 100644 --- a/xen/include/Makefile +++ b/xen/include/Makefile @@ -28,6 +28,7 @@ headers-$(CONFIG_X86) += compat/arch-x86/xen.h headers-$(CONFIG_X86) += compat/arch-x86/xen-$(compat-arch-y).h headers-$(CONFIG_X86) += compat/hvm/hvm_vcpu.h headers-$(CONFIG_X86) += compat/hvm/dm_op.h +headers-$(CONFIG_X86) += compat/hvm/hvm_op.h headers-y += compat/arch-$(compat-arch-y).h compat/pmu.h compat/xlat.h headers-$(CONFIG_FLASK) += compat/xsm/flask_op.h diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index 0bdafdf..bedce71 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -237,6 +237,23 @@ struct xen_hvm_altp2m_set_mem_access { 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_set_mem_access_multi { + /* view */ + uint16_t view; + uint16_t pad; + /* Number of pages */ + uint32_t nr; + /* Used for continuation purposes */ + uint64_t opaque; + /* List of pfns to set access for */ + XEN_GUEST_HANDLE(const_uint64) pfn_list; + /* Corresponding list of access settings for pfn_list */ + XEN_GUEST_HANDLE(const_uint8) access_list; +}; +typedef struct xen_hvm_altp2m_set_mem_access_multi + xen_hvm_altp2m_set_mem_access_multi_t; +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t); + struct xen_hvm_altp2m_change_gfn { /* view */ uint16_t view; @@ -268,15 +285,18 @@ struct xen_hvm_altp2m_op { #define HVMOP_altp2m_set_mem_access 7 /* Change a p2m entry to have a different gfn->mfn mapping */ #define HVMOP_altp2m_change_gfn 8 +/* Set access for an array of pages */ +#define HVMOP_altp2m_set_mem_access_multi 9 domid_t domain; uint16_t pad1; uint32_t pad2; union { - 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; + 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; + struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi; uint8_t pad[64]; } u; }; diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst index 0f17000..750d199 100644 --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -73,6 +73,7 @@ ? vcpu_hvm_context hvm/hvm_vcpu.h ? vcpu_hvm_x86_32 hvm/hvm_vcpu.h ? vcpu_hvm_x86_64 hvm/hvm_vcpu.h +! hvm_altp2m_set_mem_access_multi hvm/hvm_op.h ? kexec_exec kexec.h ! kexec_image kexec.h ! kexec_range kexec.h diff --git a/xen/tools/compat-build-header.py b/xen/tools/compat-build-header.py index 32421b6..12b7a6c 100755 --- a/xen/tools/compat-build-header.py +++ b/xen/tools/compat-build-header.py @@ -16,6 +16,7 @@ pats = [ [ r"(8|16|32|64)_compat_t([^\w]|$)", r"\1_t\2" ], [ r"(^|[^\w])xen_?(\w*)_compat_t([^\w]|$$)", r"\1compat_\2_t\3" ], [ r"(^|[^\w])XEN_?", r"\1COMPAT_" ], + [ r"(^|[^\w])HVMMEM_?", r"\1COMPAT_HVMMEM_" ], [ r"(^|[^\w])Xen_?", r"\1Compat_" ], [ r"(^|[^\w])long([^\w]|$$)", r"\1int\2" ] ];