| Message ID | 20191105124332.4380-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V1,1/2] x86/altp2m: Add hypercall to set a range of sve bits | expand |
On Tue, Nov 5, 2019 at 5:43 AM Alexandru Stefan ISAILA <aisaila@bitdefender.com> 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 <aisaila@bitdefender.com> > --- > 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); I have to say I'm not a fan of stuffing the current gfn progress into rc, perhaps a separate pointer being passed in for storing that and returning -ERESTART would be cleaner. > + if ( rc > 0 ) > + { > + a.u.suppress_ve.gfn = rc; There had been discussion in the past whether its acceptable to overwrite fields that were passed in like this. This may not be the expected behavior. For the mem_sharing side at least we have introduced an "opaque" field in the structure to store that continuation value (see https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/memory.h;h=cfdda6e2a87ed30bed590034d8d717674abfbc79;hb=HEAD#l524). > + 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; > + } > + } > + > + if ( ap2m ) > + p2m_unlock(ap2m); > + > + p2m_unlock(host_p2m); > + > + return rc; > +} > + > int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve, > unsigned int altp2m_idx) > { > diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h > index 353f8034d9..bccfc45a07 100644 > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -42,7 +42,7 @@ struct xen_hvm_altp2m_suppress_ve { > uint16_t view; > uint8_t suppress_ve; /* Boolean type. */ > uint8_t pad1; > - uint32_t pad2; > + uint32_t nr; > uint64_t gfn; > }; > > @@ -339,6 +339,8 @@ struct xen_hvm_altp2m_op { > #define HVMOP_altp2m_vcpu_disable_notify 13 > /* Get the active vcpu p2m index */ > #define HVMOP_altp2m_get_p2m_idx 14 > +/* Set the "Supress #VE" bit for a range of pages */ > +#define HVMOP_altp2m_set_suppress_ve_multi 15 > domid_t domain; > uint16_t pad1; > uint32_t pad2; > diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h > index e4d24502e0..0c776fc7f3 100644 > --- a/xen/include/xen/mem_access.h > +++ b/xen/include/xen/mem_access.h > @@ -75,6 +75,9 @@ long p2m_set_mem_access_multi(struct domain *d, > int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, > unsigned int altp2m_idx); > > +long p2m_set_suppress_ve_multi(struct domain *d, uint32_t start, uint32_t nr, > + bool suppress_ve, unsigned int altp2m_idx); > + > int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve, > unsigned int altp2m_idx); > > -- > 2.17.1
On 05.11.2019 17:18, Tamas K Lengyel wrote: > On Tue, Nov 5, 2019 at 5:43 AM Alexandru Stefan ISAILA > <aisaila@bitdefender.com> 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 <aisaila@bitdefender.com> >> --- >> 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); > > I have to say I'm not a fan of stuffing the current gfn progress into > rc, perhaps a separate pointer being passed in for storing that and > returning -ERESTART would be cleaner. This sounds cleaner, I will have it changed in v2. > >> + if ( rc > 0 ) >> + { >> + a.u.suppress_ve.gfn = rc; > > There had been discussion in the past whether its acceptable to > overwrite fields that were passed in like this. This may not be the > expected behavior. For the mem_sharing side at least we have > introduced an "opaque" field in the structure to store that > continuation value (see > https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/include/public/memory.h;h=cfdda6e2a87ed30bed590034d8d717674abfbc79;hb=HEAD#l524). I wanted to keep the size of the struct. It it is ok to have a new uint64_t opaque here and then pad the rest of the structures then I will change this in v2. Thanks, Alex
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 <aisaila@bitdefender.com> > --- > 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
>> >> +/* >> + * 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. Yes, that could be an issue. It will go in v2 > > Everything else looks OK to me. > If the changes requested by Tamas are also ok with you then I will have them all go in v2. Alex
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; + } + } + + if ( ap2m ) + p2m_unlock(ap2m); + + p2m_unlock(host_p2m); + + return rc; +} + int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve, unsigned int altp2m_idx) { diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index 353f8034d9..bccfc45a07 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -42,7 +42,7 @@ struct xen_hvm_altp2m_suppress_ve { uint16_t view; uint8_t suppress_ve; /* Boolean type. */ uint8_t pad1; - uint32_t pad2; + uint32_t nr; uint64_t gfn; }; @@ -339,6 +339,8 @@ struct xen_hvm_altp2m_op { #define HVMOP_altp2m_vcpu_disable_notify 13 /* Get the active vcpu p2m index */ #define HVMOP_altp2m_get_p2m_idx 14 +/* Set the "Supress #VE" bit for a range of pages */ +#define HVMOP_altp2m_set_suppress_ve_multi 15 domid_t domain; uint16_t pad1; uint32_t pad2; diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h index e4d24502e0..0c776fc7f3 100644 --- a/xen/include/xen/mem_access.h +++ b/xen/include/xen/mem_access.h @@ -75,6 +75,9 @@ long p2m_set_mem_access_multi(struct domain *d, int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve, unsigned int altp2m_idx); +long p2m_set_suppress_ve_multi(struct domain *d, uint32_t start, uint32_t nr, + bool suppress_ve, unsigned int altp2m_idx); + int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve, unsigned int altp2m_idx);
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 <aisaila@bitdefender.com> --- 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(-)