On 11/5/19 12:43 PM, Alexandru Stefan ISAILA wrote: > By default the sve bits are not set. > This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(), > to set a range of sve bits. > The core function, p2m_set_suppress_ve_multi(), does not brake in case > of a error and it is doing a best effort for setting the bits in the > given range. A check for continuation is made in order to have > preemption on big ranges. > > Signed-off-by: Alexandru Isaila <[email protected]> > --- > tools/libxc/include/xenctrl.h | 3 ++ > tools/libxc/xc_altp2m.c | 25 ++++++++++++++ > xen/arch/x86/hvm/hvm.c | 28 +++++++++++++-- > xen/arch/x86/mm/p2m.c | 61 +++++++++++++++++++++++++++++++++ > xen/include/public/hvm/hvm_op.h | 4 ++- > xen/include/xen/mem_access.h | 3 ++ > 6 files changed, 121 insertions(+), 3 deletions(-) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index f4431687b3..21b644f459 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -1923,6 +1923,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, > uint32_t domid, > uint16_t view_id); > int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid, > uint16_t view_id, xen_pfn_t gfn, bool sve); > +int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid, > + uint16_t view_id, xen_pfn_t start_gfn, > + uint32_t nr, bool sve); > int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_t domid, > uint16_t view_id, xen_pfn_t gfn, bool *sve); > int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid, > diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c > index 09dad0355e..6605d9abbe 100644 > --- a/tools/libxc/xc_altp2m.c > +++ b/tools/libxc/xc_altp2m.c > @@ -234,6 +234,31 @@ int xc_altp2m_set_suppress_ve(xc_interface *handle, > uint32_t domid, > return rc; > } > > +int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid, > + uint16_t view_id, xen_pfn_t start_gfn, > + uint32_t nr, bool sve) > +{ > + 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_suppress_ve_multi; > + arg->domain = domid; > + arg->u.suppress_ve.view = view_id; > + arg->u.suppress_ve.gfn = start_gfn; > + arg->u.suppress_ve.suppress_ve = sve; > + arg->u.suppress_ve.nr = nr; > + > + 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_set_mem_access(xc_interface *handle, uint32_t domid, > uint16_t view_id, xen_pfn_t gfn, > xenmem_access_t access) > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 06a7b40107..d3d9f8c30f 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4535,6 +4535,7 @@ static int do_altp2m_op( > case HVMOP_altp2m_destroy_p2m: > case HVMOP_altp2m_switch_p2m: > case HVMOP_altp2m_set_suppress_ve: > + case HVMOP_altp2m_set_suppress_ve_multi: > case HVMOP_altp2m_get_suppress_ve: > case HVMOP_altp2m_set_mem_access: > case HVMOP_altp2m_set_mem_access_multi: > @@ -4681,7 +4682,7 @@ static int do_altp2m_op( > break; > > case HVMOP_altp2m_set_suppress_ve: > - if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 ) > + if ( a.u.suppress_ve.pad1 ) > rc = -EINVAL; > else > { > @@ -4693,8 +4694,31 @@ static int do_altp2m_op( > } > break; > > + case HVMOP_altp2m_set_suppress_ve_multi: > + if ( a.u.suppress_ve.pad1 || !a.u.suppress_ve.nr ) > + rc = -EINVAL; > + else > + { > + rc = p2m_set_suppress_ve_multi(d, a.u.suppress_ve.gfn, > + a.u.suppress_ve.nr, > + a.u.suppress_ve.suppress_ve, > + a.u.suppress_ve.view); > + > + if ( rc > 0 ) > + { > + a.u.suppress_ve.gfn = rc; > + rc = -ERESTART; > + > + if ( __copy_field_to_guest(guest_handle_cast(arg, > + xen_hvm_altp2m_op_t), > + &a, u.suppress_ve.gfn) ) > + rc = -EFAULT; > + } > + } > + break; > + > case HVMOP_altp2m_get_suppress_ve: > - if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 ) > + if ( a.u.suppress_ve.pad1 ) > rc = -EINVAL; > else > { > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index e5e4349dea..b2e63e75ff 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -3054,6 +3054,67 @@ out: > return rc; > } > > +/* > + * Set/clear the #VE suppress bit for multiple pages. Only available on VMX. > + */ > +long p2m_set_suppress_ve_multi(struct domain *d, uint32_t start, uint32_t nr, > + bool suppress_ve, unsigned int altp2m_idx) > +{ > + struct p2m_domain *host_p2m = p2m_get_hostp2m(d); > + struct p2m_domain *ap2m = NULL; > + struct p2m_domain *p2m; > + long rc = 0; > + > + if ( altp2m_idx > 0 ) > + { > + if ( altp2m_idx >= MAX_ALTP2M || > + d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) ) > + return -EINVAL; > + > + p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx]; > + } > + else > + p2m = host_p2m; > + > + p2m_lock(host_p2m); > + > + if ( ap2m ) > + p2m_lock(ap2m); > + > + > + while ( start < nr ) > + { > + p2m_access_t a; > + p2m_type_t t; > + mfn_t mfn; > + > + rc = altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, &a, > AP2MGET_query); > + > + if ( rc ) > + a = p2m->default_access; > + > + rc = p2m->set_entry(p2m, _gfn(start), mfn, PAGE_ORDER_4K, t, a, > suppress_ve); > + > + /* Try best effort for setting the whole range. */ > + if ( rc ) > + continue; > + > + /* Check for continuation if it's not the last iteration. */ > + if ( nr > ++start && hypercall_preempt_check() ) > + { > + rc = start; > + break; > + }
What's the point of the "if ( rc ) continue;"? All it's doing is preventing the loop from being preempted at that point; but there doesn't seem to be a good reason for that. In fact, if an attacker could engineer a situation where large swaths could fail, it could use this to lock up the cpu for an unreasonable amount of time. Everything else looks OK to me. -George _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
