| Message ID | 20190607105449.28167-1-aisaila@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] x86/altp2m: Add a new hypercall to get the active altp2m index | expand |
>>> On 07.06.19 at 12:55, <aisaila@bitdefender.com> wrote: > @@ -4735,6 +4736,28 @@ static int do_altp2m_op( > _gfn(a.u.change_gfn.old_gfn), > _gfn(a.u.change_gfn.new_gfn)); > break; > + > + case HVMOP_altp2m_get_p2m_idx: > + { > + struct vcpu *v; > + > + if ( !altp2m_active(d) ) > + { > + rc = -EOPNOTSUPP; > + break; > + } > + > + if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL ) > + { > + rc = -EINVAL; > + break; > + } The order of return values was the other way around before, but I suppose this doesn't matter much? > + a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v); > + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; Just as remark for the future (I didn't pay attention before, and the difference isn't overly meaningful for operations that don't occur frequently) - __copy_field_to_guest() would be less overhead here. Oh, right - this is once again not possible because of arg (still) being a handle of void. Jan
Ping, Any thoughts on this matter are appreciated. Thanks, Alex On 07.06.2019 13:55, Alexandru Stefan ISAILA wrote: > The patch adds a new lib xc function (xc_altp2m_get_vcpu_p2m_idx) that > uses a new hvmop (HVMOP_altp2m_get_p2m_idx) to get the active altp2m > index from a given vcpu. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> > > --- > Changes since V2: > - Update comment and title > - Remove redundant max_vcpu check. > --- > tools/libxc/include/xenctrl.h | 2 ++ > tools/libxc/xc_altp2m.c | 25 +++++++++++++++++++++++++ > xen/arch/x86/hvm/hvm.c | 23 +++++++++++++++++++++++ > xen/include/public/hvm/hvm_op.h | 8 ++++++++ > 4 files changed, 58 insertions(+) > > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h > index 538007a6dc..87526af4b4 100644 > --- a/tools/libxc/include/xenctrl.h > +++ b/tools/libxc/include/xenctrl.h > @@ -1942,6 +1942,8 @@ int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid, > int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid, > uint16_t view_id, xen_pfn_t old_gfn, > xen_pfn_t new_gfn); > +int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid, > + uint32_t vcpuid, uint16_t *p2midx); > > /** > * Mem paging operations. > diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c > index a86520c232..09dad0355e 100644 > --- a/tools/libxc/xc_altp2m.c > +++ b/tools/libxc/xc_altp2m.c > @@ -352,3 +352,28 @@ int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid, > xc_hypercall_buffer_free(handle, arg); > return rc; > } > + > +int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid, > + uint32_t vcpuid, uint16_t *altp2m_idx) > +{ > + 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_get_p2m_idx; > + arg->domain = domid; > + arg->u.get_vcpu_p2m_idx.vcpu_id = vcpuid; > + > + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, > + HYPERCALL_BUFFER_AS_ARG(arg)); > + if ( !rc ) > + *altp2m_idx = arg->u.get_vcpu_p2m_idx.altp2m_idx; > + > + xc_hypercall_buffer_free(handle, arg); > + return rc; > +} > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 029eea3b85..4ee7e6ce47 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4500,6 +4500,7 @@ static int do_altp2m_op( > case HVMOP_altp2m_set_mem_access_multi: > case HVMOP_altp2m_get_mem_access: > case HVMOP_altp2m_change_gfn: > + case HVMOP_altp2m_get_p2m_idx: > break; > > default: > @@ -4735,6 +4736,28 @@ static int do_altp2m_op( > _gfn(a.u.change_gfn.old_gfn), > _gfn(a.u.change_gfn.new_gfn)); > break; > + > + case HVMOP_altp2m_get_p2m_idx: > + { > + struct vcpu *v; > + > + if ( !altp2m_active(d) ) > + { > + rc = -EOPNOTSUPP; > + break; > + } > + > + if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL ) > + { > + rc = -EINVAL; > + break; > + } > + > + a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v); > + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > + break; > + } > + > default: > ASSERT_UNREACHABLE(); > } > diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h > index c6cd12f596..353f8034d9 100644 > --- a/xen/include/public/hvm/hvm_op.h > +++ b/xen/include/public/hvm/hvm_op.h > @@ -304,6 +304,11 @@ struct xen_hvm_altp2m_change_gfn { > typedef struct xen_hvm_altp2m_change_gfn xen_hvm_altp2m_change_gfn_t; > DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_change_gfn_t); > > +struct xen_hvm_altp2m_get_vcpu_p2m_idx { > + uint32_t vcpu_id; > + uint16_t altp2m_idx; > +}; > + > struct xen_hvm_altp2m_op { > uint32_t version; /* HVMOP_ALTP2M_INTERFACE_VERSION */ > uint32_t cmd; > @@ -332,6 +337,8 @@ struct xen_hvm_altp2m_op { > #define HVMOP_altp2m_get_mem_access 12 > /* Disable altp2m event notifications for a given VCPU */ > #define HVMOP_altp2m_vcpu_disable_notify 13 > +/* Get the active vcpu p2m index */ > +#define HVMOP_altp2m_get_p2m_idx 14 > domid_t domain; > uint16_t pad1; > uint32_t pad2; > @@ -347,6 +354,7 @@ struct xen_hvm_altp2m_op { > struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi; > struct xen_hvm_altp2m_suppress_ve suppress_ve; > struct xen_hvm_altp2m_vcpu_disable_notify disable_notify; > + struct xen_hvm_altp2m_get_vcpu_p2m_idx get_vcpu_p2m_idx; > uint8_t pad[64]; > } u; > }; >
Ping, Any reviews on this patch are appreciated. Regards, Alex On 07.06.2019 14:50, Jan Beulich wrote: >>>> On 07.06.19 at 12:55, <aisaila@bitdefender.com> wrote: >> @@ -4735,6 +4736,28 @@ static int do_altp2m_op( >> _gfn(a.u.change_gfn.old_gfn), >> _gfn(a.u.change_gfn.new_gfn)); >> break; >> + >> + case HVMOP_altp2m_get_p2m_idx: >> + { >> + struct vcpu *v; >> + >> + if ( !altp2m_active(d) ) >> + { >> + rc = -EOPNOTSUPP; >> + break; >> + } >> + >> + if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL ) >> + { >> + rc = -EINVAL; >> + break; >> + } > > The order of return values was the other way around before, but > I suppose this doesn't matter much? > >> + a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v); >> + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > > Just as remark for the future (I didn't pay attention before, > and the difference isn't overly meaningful for operations that > don't occur frequently) - __copy_field_to_guest() would be > less overhead here. Oh, right - this is once again not possible > because of arg (still) being a handle of void. > > Jan > > > > ________________________ > This email was scanned by Bitdefender >
On 22.07.2019 14:39, Alexandru Stefan ISAILA wrote: > Ping, > > Any reviews on this patch are appreciated. FAOD I think I've provided all the feedback I have. The patch doesn't seem to have a tool stack maintainer ack yet, and I think I had made clear previously that I'm willing to look at changes to do_altp2m_op(), but that I'm not willing to ack new additions to this interface as long as it's (optionally or not) exposed to guests. It effectively being part of altp2m, strictly speaking it shouldn't be a general x86 maintainer ack that's needed here anyway. As a hint - it certainly would help if the function was moved suitably. Jan > On 07.06.2019 14:50, Jan Beulich wrote: >>>>> On 07.06.19 at 12:55, <aisaila@bitdefender.com> wrote: >>> @@ -4735,6 +4736,28 @@ static int do_altp2m_op( >>> _gfn(a.u.change_gfn.old_gfn), >>> _gfn(a.u.change_gfn.new_gfn)); >>> break; >>> + >>> + case HVMOP_altp2m_get_p2m_idx: >>> + { >>> + struct vcpu *v; >>> + >>> + if ( !altp2m_active(d) ) >>> + { >>> + rc = -EOPNOTSUPP; >>> + break; >>> + } >>> + >>> + if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL ) >>> + { >>> + rc = -EINVAL; >>> + break; >>> + } >> >> The order of return values was the other way around before, but >> I suppose this doesn't matter much? >> >>> + a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v); >>> + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; >> >> Just as remark for the future (I didn't pay attention before, >> and the difference isn't overly meaningful for operations that >> don't occur frequently) - __copy_field_to_guest() would be >> less overhead here. Oh, right - this is once again not possible >> because of arg (still) being a handle of void. >> >> Jan >> >> >> >> ________________________ >> This email was scanned by Bitdefender >>
On 6/7/19 11:55 AM, Alexandru Stefan ISAILA wrote: > The patch adds a new lib xc function (xc_altp2m_get_vcpu_p2m_idx) that > uses a new hvmop (HVMOP_altp2m_get_p2m_idx) to get the active altp2m > index from a given vcpu. > > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> This looks good to me. Sorry it took so long to get to it: Reviewed-by: George Dunlap <george.dunlap@citrix.com>
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h index 538007a6dc..87526af4b4 100644 --- a/tools/libxc/include/xenctrl.h +++ b/tools/libxc/include/xenctrl.h @@ -1942,6 +1942,8 @@ int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid, int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid, uint16_t view_id, xen_pfn_t old_gfn, xen_pfn_t new_gfn); +int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid, + uint32_t vcpuid, uint16_t *p2midx); /** * Mem paging operations. diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c index a86520c232..09dad0355e 100644 --- a/tools/libxc/xc_altp2m.c +++ b/tools/libxc/xc_altp2m.c @@ -352,3 +352,28 @@ int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid, xc_hypercall_buffer_free(handle, arg); return rc; } + +int xc_altp2m_get_vcpu_p2m_idx(xc_interface *handle, uint32_t domid, + uint32_t vcpuid, uint16_t *altp2m_idx) +{ + 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_get_p2m_idx; + arg->domain = domid; + arg->u.get_vcpu_p2m_idx.vcpu_id = vcpuid; + + rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m, + HYPERCALL_BUFFER_AS_ARG(arg)); + if ( !rc ) + *altp2m_idx = arg->u.get_vcpu_p2m_idx.altp2m_idx; + + xc_hypercall_buffer_free(handle, arg); + return rc; +} diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 029eea3b85..4ee7e6ce47 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -4500,6 +4500,7 @@ static int do_altp2m_op( case HVMOP_altp2m_set_mem_access_multi: case HVMOP_altp2m_get_mem_access: case HVMOP_altp2m_change_gfn: + case HVMOP_altp2m_get_p2m_idx: break; default: @@ -4735,6 +4736,28 @@ static int do_altp2m_op( _gfn(a.u.change_gfn.old_gfn), _gfn(a.u.change_gfn.new_gfn)); break; + + case HVMOP_altp2m_get_p2m_idx: + { + struct vcpu *v; + + if ( !altp2m_active(d) ) + { + rc = -EOPNOTSUPP; + break; + } + + if ( (v = domain_vcpu(d, a.u.get_vcpu_p2m_idx.vcpu_id)) == NULL ) + { + rc = -EINVAL; + break; + } + + a.u.get_vcpu_p2m_idx.altp2m_idx = altp2m_vcpu_idx(v); + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; + break; + } + default: ASSERT_UNREACHABLE(); } diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h index c6cd12f596..353f8034d9 100644 --- a/xen/include/public/hvm/hvm_op.h +++ b/xen/include/public/hvm/hvm_op.h @@ -304,6 +304,11 @@ struct xen_hvm_altp2m_change_gfn { typedef struct xen_hvm_altp2m_change_gfn xen_hvm_altp2m_change_gfn_t; DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_change_gfn_t); +struct xen_hvm_altp2m_get_vcpu_p2m_idx { + uint32_t vcpu_id; + uint16_t altp2m_idx; +}; + struct xen_hvm_altp2m_op { uint32_t version; /* HVMOP_ALTP2M_INTERFACE_VERSION */ uint32_t cmd; @@ -332,6 +337,8 @@ struct xen_hvm_altp2m_op { #define HVMOP_altp2m_get_mem_access 12 /* Disable altp2m event notifications for a given VCPU */ #define HVMOP_altp2m_vcpu_disable_notify 13 +/* Get the active vcpu p2m index */ +#define HVMOP_altp2m_get_p2m_idx 14 domid_t domain; uint16_t pad1; uint32_t pad2; @@ -347,6 +354,7 @@ struct xen_hvm_altp2m_op { struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi; struct xen_hvm_altp2m_suppress_ve suppress_ve; struct xen_hvm_altp2m_vcpu_disable_notify disable_notify; + struct xen_hvm_altp2m_get_vcpu_p2m_idx get_vcpu_p2m_idx; uint8_t pad[64]; } u; };
The patch adds a new lib xc function (xc_altp2m_get_vcpu_p2m_idx) that uses a new hvmop (HVMOP_altp2m_get_p2m_idx) to get the active altp2m index from a given vcpu. Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com> --- Changes since V2: - Update comment and title - Remove redundant max_vcpu check. --- tools/libxc/include/xenctrl.h | 2 ++ tools/libxc/xc_altp2m.c | 25 +++++++++++++++++++++++++ xen/arch/x86/hvm/hvm.c | 23 +++++++++++++++++++++++ xen/include/public/hvm/hvm_op.h | 8 ++++++++ 4 files changed, 58 insertions(+)