| Message ID | 1507570237-5420-1-git-send-email-ppircalabu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 09.10.17 at 19:30, <ppircalabu@bitdefender.com> wrote: > @@ -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, This not being a memop, re-using this value looks arbitrary. Unless it absolutely has to be this value, please either add a brief comment saying this just happens to fit your need or use a literal constant. > + a.u.set_mem_access_multi.view); > + if ( rc > 0 ) > + { > + a.u.set_mem_access_multi.opaque = rc; > + if ( __copy_field_to_guest(guest_handle_cast(arg, xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) ) Long line. Also please consider adding a prereq patch changing the function's parameter to a properly typed handle, which will then make unnecessary using the not obviously correct cast here. Other copying back of individual fields in this function could then also be switched to __copy_field_to_guest(). > @@ -4586,6 +4613,57 @@ 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); > + } > + > + /* Manually fill the common part of the xen_hvm_altp2m_op structure because Comment style. > + * the generated XLAT_hvm_altp2m_op macro doesn't correctly handle the > + * translation of all fields from compat_hvm_altp2m_op to xen_hvm_altp2m_op. > + */ > + 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; I specifically asked (and even more than once, I think) that you add the size (and whatever else) checks we would have here if this wasn't open coding an XLAT_* macro. > + return do_altp2m_op(nat.hnd); You appear to miss copying back opaque here, in case it was set in do_altp2m_op(). > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -83,6 +83,13 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_pci_link_route_t); > /* Flushes all VCPU TLBs: @arg must be NULL. */ > #define HVMOP_flush_tlbs 5 > > +/* > + * hvmmem_type_t should not be defined when generating the corresponding > + * compat header. This will ensure that the HVMMEM_(*) values are defined > + * only once. To help readers fully understand the situation, please consider making this "This will ensure that the improperly named HVMMEM_(*) values ..."; otherwise it gives the appearance of there being a bug in the translation scripts. > @@ -237,6 +246,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); Are typedef and handle actually needed anywhere? Otherwise please don't add them. Just like recently done for domctl and sysctl we should even consider cleaning up the others here. Jan
On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > > > > > > > > > > > > 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, > This not being a memop, re-using this value looks arbitrary. > Unless it absolutely has to be this value, please either add a brief > comment saying this just happens to fit your need or use a literal > constant. I will add a comment to clarify the reason for using MEMOP_CMD_MASK: e.g. /* MEMOP_CMD_MASK is used here to mirror the way p2m_set_mem_access_multi() is called for the XENMEM_access_op_set_access_multi case. */ > > > > > + a.u.set_mem_access_multi.vie > > w); > > + if ( rc > 0 ) > > + { > > + a.u.set_mem_access_multi.opaque = rc; > > + if ( __copy_field_to_guest(guest_handle_cast(arg, > > xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) ) > Long line. > > Also please consider adding a prereq patch changing the function's > parameter to a properly typed handle, which will then make > unnecessary using the not obviously correct cast here. Other > copying back of individual fields in this function could then also be > switched to __copy_field_to_guest(). Actually, changing the function's parameter to a typed handle could complicate things a little for compat_altp2m_op. It should be modified also to use a typed handle (compat_hvm_altp2m_op_t), which in case of commands other than HVMOP_altp2m_set_mem_access_multi should be casted to xen_hvm_altp2m_op_t before calling do_altp2m_op. If the problem was only related to the code clarity, I think the new implementation will be a little more difficult to follow. > > > > > @@ -4586,6 +4613,57 @@ 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_gueWill change in the next patch iteration. > st(&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); > > + } > > + > > + /* Manually fill the common part of the xen_hvm_altp2m_op > > structure because > Comment style. Will change in the next patch iteration. > > > > > + * the generated XLAT_hvm_altp2m_op macro doesn't correctly > > handle the > > + * translation of all fields from compat_hvm_altp2m_op to > > xen_hvm_altp2m_op. > > + */ > > + 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; > I specifically asked (and even more than once, I think) that you add > the size (and whatever else) checks we would have here if this wasn't > open coding an XLAT_* macro. I ran into a build error while trying to use generated CHECK_ macros. I will try to add a manual check here. > > > > > + return do_altp2m_op(nat.hnd); > You appear to miss copying back opaque here, in case it was set > in do_altp2m_op(). > > > > > --- a/xen/include/public/hvm/hvm_op.h > > +++ b/xen/include/public/hvm/hvm_op.h > > @@ -83,6 +83,13 @@ > > DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_pci_link_route_t); > > /* Flushes all VCPU TLBs: @arg must be NULL. */ > > #define HVMOP_flush_tlbs 5 > > > > +/* > > + * hvmmem_type_t should not be defined when generating the > > corresponding > > + * compat header. This will ensure that the HVMMEM_(*) values are > > defined > > + * only once. > To help readers fully understand the situation, please consider > making this "This will ensure that the improperly named > HVMMEM_(*) values ..."; otherwise it gives the appearance of > there being a bug in the translation scripts. Will change in the next patch iteration. > Will change in the next patch iteration. > > > > > > @@ -237,6 +246,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); > Are typedef and handle actually needed anywhere? Otherwise > please don't add them. Just like recently done for domctl and > sysctl we should even consider cleaning up the others here. Will change in the next patch iteration. > > > Jan > > Many thanks for your support, Petre > ________________________ > This email was scanned by Bitdefender
>>> On 10.10.17 at 16:13, <ppircalabu@bitdefender.com> wrote: > On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > >> > > >> > > > >> > > > 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, >> This not being a memop, re-using this value looks arbitrary. >> Unless it absolutely has to be this value, please either add a brief >> comment saying this just happens to fit your need or use a literal >> constant. > I will add a comment to clarify the reason for using MEMOP_CMD_MASK: > e.g. > /* MEMOP_CMD_MASK is used here to mirror the way > p2m_set_mem_access_multi() is called for the > XENMEM_access_op_set_access_multi case. */ But that's neither brief nor an actual reason (if at all it is a cosmetic one). A wider or more narrow mask would do, afaict. >> > + a.u.set_mem_access_multi.vie >> > w); >> > + if ( rc > 0 ) >> > + { >> > + a.u.set_mem_access_multi.opaque = rc; >> > + if ( __copy_field_to_guest(guest_handle_cast(arg, >> > xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) ) >> Long line. >> >> Also please consider adding a prereq patch changing the function's >> parameter to a properly typed handle, which will then make >> unnecessary using the not obviously correct cast here. Other >> copying back of individual fields in this function could then also be >> switched to __copy_field_to_guest(). > > Actually, changing the function's parameter to a typed handle could > complicate things a little for compat_altp2m_op. It should be modified > also to use a typed handle (compat_hvm_altp2m_op_t), which in case of > commands other than HVMOP_altp2m_set_mem_access_multi should be casted > to xen_hvm_altp2m_op_t before calling do_altp2m_op. > If the problem was only related to the code clarity, I think the new > implementation will be a little more difficult to follow. Hmm, likely true - it would help do_altp2m_op(), but would indeed look to harm the new ad hoc compat wrapper you add. >> > @@ -4586,6 +4613,57 @@ 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_gueWill change in the next patch iteration. >> > st(&a, arg, 1) ) This isn't the first time I see such a misplaced comment. Once again I can't make sense of it when being placed this way. Jan
On 10/10/2017 05:24 PM, Jan Beulich wrote: >>>> On 10.10.17 at 16:13, <ppircalabu@bitdefender.com> wrote: >> On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > >>>>> >>>>>> >>>>>> 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, >>> This not being a memop, re-using this value looks arbitrary. >>> Unless it absolutely has to be this value, please either add a brief >>> comment saying this just happens to fit your need or use a literal >>> constant. >> I will add a comment to clarify the reason for using MEMOP_CMD_MASK: >> e.g. >> /* MEMOP_CMD_MASK is used here to mirror the way >> p2m_set_mem_access_multi() is called for the >> XENMEM_access_op_set_access_multi case. */ > > But that's neither brief nor an actual reason (if at all it is a cosmetic > one). A wider or more narrow mask would do, afaict. Revisiting the p2m_set_mem_access_multi() code, the mask is used here: 447 /* Check for continuation if it's not the last iteration. */ 448 if ( nr > ++start && !(start & mask) && hypercall_preempt_check() ) 449 { 450 rc = start; 451 break; 452 } If I'm reading the code correctly, MEMOP_CMD_MASK happens to be 0000000000111111, which allows an interruption in the processing loop every 64th time we go through it. Now, it does indeed make more sense to see MEMOP_CMD_MASK being used with XENMEM_access_op_set_access_multi than it does with altp2m (where we're not dealing with memops). However, indeed almost any mask would do here (0x1f, for example, or 0x7f, or something else entirely). The reason I've initially picked MEMOP_CMD_MASK for this patch is that it had seemed like a reasonable default (currenly made use of with XENMEM_access_op_set_access_multi). I'm quite likely missing something, but I don't know what criteria we should use for picking a good value here, and how to express it. And if we go with the tried and true MEMOP_CMD_MASK (because it seems to work well), is it not appropriate to express that intent by using its name in the code? Should we just use it's value directly (i.e. 0x3f)? Thanks, Razvan
On Tue, 2017-10-10 at 06:28 -0600, Jan Beulich wrote: > > > > +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); > > Are typedef and handle actually needed anywhere? Otherwise > please don't add them. Just like recently done for domctl and > sysctl we should even consider cleaning up the others here. > > Jan All xen_hvm_altp2m_* structs have also defined the typedef and the handle. I can remove them for xen_hvm_altp2m_set_mem_access_multi but this way it will not be in sync with the other xen_hvm_altp2m_* definitions. Also, regarding the typedef, I have encountered a possible usage when trying to generate the XLAT macro for xen_hvm_altp2m_op. Using the existing way of declaring the structure (union of structs) the enum corresponding to the union members was not generated. Replacing struct with the corresponding typedef fixed the issue. The cause is the condition the ./xen/tools/get-fields.sh uses to generate the enum (if [ "${kind%%;*}" = union ] ). In our case the kind variable is something like "struct;struct;struct;..;union" and the condition is valid only if kind starts with "union", hence the enum is not generated. Unfortunately my understanding of this script is quite scarce, so I cannot propose viable a solution. Personally I would prefer to keep the definition in sync with the other xen_hvm_altp2m_* structs for now and hanlde this issue in a separate patch for applicable for all definitions. Best regards, Petre > > ________________________ > This email was scanned by Bitdefender
>>> On 10.10.17 at 20:08, <rcojocaru@bitdefender.com> wrote: > On 10/10/2017 05:24 PM, Jan Beulich wrote: >>>>> On 10.10.17 at 16:13, <ppircalabu@bitdefender.com> wrote: >>> On Ma, 2017-10-10 at 06:28 -0600, Jan Beulich wrote:> > >>>>>> >>>>>>> >>>>>>> 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, >>>> This not being a memop, re-using this value looks arbitrary. >>>> Unless it absolutely has to be this value, please either add a brief >>>> comment saying this just happens to fit your need or use a literal >>>> constant. >>> I will add a comment to clarify the reason for using MEMOP_CMD_MASK: >>> e.g. >>> /* MEMOP_CMD_MASK is used here to mirror the way >>> p2m_set_mem_access_multi() is called for the >>> XENMEM_access_op_set_access_multi case. */ >> >> But that's neither brief nor an actual reason (if at all it is a cosmetic >> one). A wider or more narrow mask would do, afaict. > > Revisiting the p2m_set_mem_access_multi() code, the mask is used here: > > 447 /* Check for continuation if it's not the last iteration. */ > 448 if ( nr > ++start && !(start & mask) && hypercall_preempt_check() ) > 449 { > 450 rc = start; > 451 break; > 452 } > > If I'm reading the code correctly, MEMOP_CMD_MASK happens to be > 0000000000111111, which allows an interruption in the processing loop > every 64th time we go through it. > > Now, it does indeed make more sense to see MEMOP_CMD_MASK being used > with XENMEM_access_op_set_access_multi than it does with altp2m (where > we're not dealing with memops). However, indeed almost any mask would do > here (0x1f, for example, or 0x7f, or something else entirely). > > The reason I've initially picked MEMOP_CMD_MASK for this patch is that > it had seemed like a reasonable default (currenly made use of with > XENMEM_access_op_set_access_multi). I'm quite likely missing something, > but I don't know what criteria we should use for picking a good value > here, and how to express it. And if we go with the tried and true > MEMOP_CMD_MASK (because it seems to work well), is it not appropriate to > express that intent by using its name in the code? My point is that MEMOP_CMD_MASK can't be changed, i.e. other than the value to be used here it is not arbitrary. > Should we just use it's value directly (i.e. 0x3f)? That's certainly an option. Jan
>>> On 10.10.17 at 23:00, <ppircalabu@bitdefender.com> wrote: > On Tue, 2017-10-10 at 06:28 -0600, Jan Beulich wrote: >> > > > +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); >> >> Are typedef and handle actually needed anywhere? Otherwise >> please don't add them. Just like recently done for domctl and >> sysctl we should even consider cleaning up the others here. >> > All xen_hvm_altp2m_* structs have also defined the typedef and the > handle. I can remove them for xen_hvm_altp2m_set_mem_access_multi but > this way it will not be in sync with the other xen_hvm_altp2m_* > definitions. Please, as frequently asked for elsewhere elsewhere, let's not spread badness once it was recognized. > Also, regarding the typedef, I have encountered a possible usage when > trying to generate the XLAT macro for xen_hvm_altp2m_op. Using the > existing way of declaring the structure (union of structs) the enum > corresponding to the union members was not generated. Replacing struct > with the corresponding typedef fixed the issue. Now that's a valid argument, if that way less customization is necessary elsewhere in your patch. But that still wouldn't require the handle to be declared. 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..9b5302a 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_field_to_guest(guest_handle_cast(arg, xen_hvm_altp2m_op_t), &a, u.set_mem_access_multi.opaque) ) + 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,57 @@ 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); + } + + /* Manually fill the common part of the xen_hvm_altp2m_op structure because + * the generated XLAT_hvm_altp2m_op macro doesn't correctly handle the + * translation of all fields from compat_hvm_altp2m_op to xen_hvm_altp2m_op. + */ + 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 +4811,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..c12c1af 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -83,6 +83,13 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_pci_link_route_t); /* Flushes all VCPU TLBs: @arg must be NULL. */ #define HVMOP_flush_tlbs 5 +/* + * hvmmem_type_t should not be defined when generating the corresponding + * compat header. This will ensure that the HVMMEM_(*) values are defined + * only once. + */ +#ifndef XEN_GENERATING_COMPAT_HEADERS + typedef enum { HVMMEM_ram_rw, /* Normal read/write guest RAM */ HVMMEM_ram_ro, /* Read-only; writes are discarded */ @@ -102,6 +109,8 @@ typedef enum { to HVMMEM_ram_rw. */ } hvmmem_type_t; +#endif /* XEN_GENERATING_COMPAT_HEADERS */ + /* Hint from PV drivers for pagetable destruction. */ #define HVMOP_pagetable_dying 9 struct xen_hvm_pagetable_dying { @@ -237,6 +246,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 +294,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..5010fcc 100644 --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -70,6 +70,7 @@ ? dm_op_set_pci_intx_level hvm/dm_op.h ? dm_op_set_pci_link_route hvm/dm_op.h ? dm_op_track_dirty_vram hvm/dm_op.h +! hvm_altp2m_set_mem_access_multi hvm/hvm_op.h ? vcpu_hvm_context hvm/hvm_vcpu.h ? vcpu_hvm_x86_32 hvm/hvm_vcpu.h ? vcpu_hvm_x86_64 hvm/hvm_vcpu.h