| Message ID | a9899858dba4a7e22a0256cff734399bff348adb.1594150543.git.michal.leszczynski@cert.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement support for external IPT monitoring | expand |
On Tue, Jul 07, 2020 at 09:39:48PM +0200, Michał Leszczyński wrote: > From: Michal Leszczynski <michal.leszczynski@cert.pl> > > Implement domctl to manage the runtime state of > processor trace feature. > > Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl> > --- > xen/arch/x86/domctl.c | 50 +++++++++++++++++++++++++++++++++++++ > xen/include/public/domctl.h | 28 +++++++++++++++++++++ > 2 files changed, 78 insertions(+) > > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c > index 6f2c69788d..6132499db4 100644 > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -322,6 +322,50 @@ void arch_get_domain_info(const struct domain *d, > info->arch_config.emulation_flags = d->arch.emulation_flags; > } > > +static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op, > + XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > +{ > + int rc; > + struct vcpu *v; > + > + if ( op->pad1 || op->pad2 ) > + return -EINVAL; > + > + if ( !vmtrace_supported ) > + return -EOPNOTSUPP; > + > + if ( !is_hvm_domain(d) ) > + return -EOPNOTSUPP; You can join both if into a single one if they both return -EOPNOTSUPP. > + > + if ( op->vcpu >= d->max_vcpus ) > + return -EINVAL; You can remove this check and just use the return value of domain_vcpu instead, if it's NULL the vCPU ID is wrong. > + > + v = domain_vcpu(d, op->vcpu); > + rc = 0; No need to init rc to 0, as it would be unconditionally initialized in the switch below due to the default case. > + > + switch ( op->cmd ) > + { > + case XEN_DOMCTL_vmtrace_pt_enable: > + case XEN_DOMCTL_vmtrace_pt_disable: > + vcpu_pause(v); > + rc = vmtrace_control_pt(v, op->cmd == XEN_DOMCTL_vmtrace_pt_enable); > + vcpu_unpause(v); > + break; > + > + case XEN_DOMCTL_vmtrace_pt_get_offset: > + rc = vmtrace_get_pt_offset(v, &op->offset, &op->size); In order to get consistent values here the vCPU should be paused, or else you just get stale values from the last vmexit? Maybe that's fine for your use case? > + > + if ( !rc && d->is_dying ) > + rc = ENODATA; This check here seems weird, if this is really required shouldn't it be done before attempting to fetch the data? > + break; > + > + default: > + rc = -EOPNOTSUPP; > + } > + > + return rc; > +} > + > #define MAX_IOPORTS 0x10000 > > long arch_do_domctl( > @@ -337,6 +381,12 @@ long arch_do_domctl( > switch ( domctl->cmd ) > { > > + case XEN_DOMCTL_vmtrace_op: > + ret = do_vmtrace_op(d, &domctl->u.vmtrace_op, u_domctl); > + if ( !ret ) > + copyback = true; > + break; Nit: new additions would usually got at the end of the switch. > + > case XEN_DOMCTL_shadow_op: > ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0); > if ( ret == -ERESTART ) > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 7681675a94..73c7ccbd16 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -1136,6 +1136,30 @@ struct xen_domctl_vuart_op { > */ > }; > > +/* XEN_DOMCTL_vmtrace_op: Perform VM tracing related operation */ > +#if defined(__XEN__) || defined(__XEN_TOOLS__) > + > +struct xen_domctl_vmtrace_op { > + /* IN variable */ > + uint32_t cmd; > +/* Enable/disable external vmtrace for given domain */ > +#define XEN_DOMCTL_vmtrace_pt_enable 1 > +#define XEN_DOMCTL_vmtrace_pt_disable 2 > +#define XEN_DOMCTL_vmtrace_pt_get_offset 3 > + domid_t domain; > + uint16_t pad1; > + uint32_t vcpu; > + uint16_t pad2; pad2 should be a uint32_t? Or else there's a padding field added by the compiler? (maybe I'm missing something, I haven't checked with pahole). > + > + /* OUT variable */ > + uint64_aligned_t size; > + uint64_aligned_t offset; > +}; > +typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t; > +DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t); > + > +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ > + > struct xen_domctl { > uint32_t cmd; > #define XEN_DOMCTL_createdomain 1 > @@ -1217,6 +1241,7 @@ struct xen_domctl { > #define XEN_DOMCTL_vuart_op 81 > #define XEN_DOMCTL_get_cpu_policy 82 > #define XEN_DOMCTL_set_cpu_policy 83 > +#define XEN_DOMCTL_vmtrace_op 84 > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > @@ -1277,6 +1302,9 @@ struct xen_domctl { > struct xen_domctl_monitor_op monitor_op; > struct xen_domctl_psr_alloc psr_alloc; > struct xen_domctl_vuart_op vuart_op; > +#if defined(__XEN__) || defined(__XEN_TOOLS__) > + struct xen_domctl_vmtrace_op vmtrace_op; > +#endif No need for the preprocessor conditionals here, the whole domctl.h is only to be used by the tools or Xen, and is already properly guarded (see the #error at the top of the file). Thanks.
On 07.07.2020 21:39, Michał Leszczyński wrote: > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -1136,6 +1136,30 @@ struct xen_domctl_vuart_op { > */ > }; > > +/* XEN_DOMCTL_vmtrace_op: Perform VM tracing related operation */ > +#if defined(__XEN__) || defined(__XEN_TOOLS__) > + > +struct xen_domctl_vmtrace_op { > + /* IN variable */ > + uint32_t cmd; > +/* Enable/disable external vmtrace for given domain */ > +#define XEN_DOMCTL_vmtrace_pt_enable 1 > +#define XEN_DOMCTL_vmtrace_pt_disable 2 > +#define XEN_DOMCTL_vmtrace_pt_get_offset 3 > + domid_t domain; > + uint16_t pad1; > + uint32_t vcpu; > + uint16_t pad2; > + > + /* OUT variable */ > + uint64_aligned_t size; > + uint64_aligned_t offset; > +}; > +typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t; > +DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t); > + > +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ Here and ... > @@ -1277,6 +1302,9 @@ struct xen_domctl { > struct xen_domctl_monitor_op monitor_op; > struct xen_domctl_psr_alloc psr_alloc; > struct xen_domctl_vuart_op vuart_op; > +#if defined(__XEN__) || defined(__XEN_TOOLS__) > + struct xen_domctl_vmtrace_op vmtrace_op; > +#endif ... here I'm struggling with the #ifdef-s - see the very top of the file. Jan
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 6f2c69788d..6132499db4 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -322,6 +322,50 @@ void arch_get_domain_info(const struct domain *d, info->arch_config.emulation_flags = d->arch.emulation_flags; } +static int do_vmtrace_op(struct domain *d, struct xen_domctl_vmtrace_op *op, + XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) +{ + int rc; + struct vcpu *v; + + if ( op->pad1 || op->pad2 ) + return -EINVAL; + + if ( !vmtrace_supported ) + return -EOPNOTSUPP; + + if ( !is_hvm_domain(d) ) + return -EOPNOTSUPP; + + if ( op->vcpu >= d->max_vcpus ) + return -EINVAL; + + v = domain_vcpu(d, op->vcpu); + rc = 0; + + switch ( op->cmd ) + { + case XEN_DOMCTL_vmtrace_pt_enable: + case XEN_DOMCTL_vmtrace_pt_disable: + vcpu_pause(v); + rc = vmtrace_control_pt(v, op->cmd == XEN_DOMCTL_vmtrace_pt_enable); + vcpu_unpause(v); + break; + + case XEN_DOMCTL_vmtrace_pt_get_offset: + rc = vmtrace_get_pt_offset(v, &op->offset, &op->size); + + if ( !rc && d->is_dying ) + rc = ENODATA; + break; + + default: + rc = -EOPNOTSUPP; + } + + return rc; +} + #define MAX_IOPORTS 0x10000 long arch_do_domctl( @@ -337,6 +381,12 @@ long arch_do_domctl( switch ( domctl->cmd ) { + case XEN_DOMCTL_vmtrace_op: + ret = do_vmtrace_op(d, &domctl->u.vmtrace_op, u_domctl); + if ( !ret ) + copyback = true; + break; + case XEN_DOMCTL_shadow_op: ret = paging_domctl(d, &domctl->u.shadow_op, u_domctl, 0); if ( ret == -ERESTART ) diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 7681675a94..73c7ccbd16 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -1136,6 +1136,30 @@ struct xen_domctl_vuart_op { */ }; +/* XEN_DOMCTL_vmtrace_op: Perform VM tracing related operation */ +#if defined(__XEN__) || defined(__XEN_TOOLS__) + +struct xen_domctl_vmtrace_op { + /* IN variable */ + uint32_t cmd; +/* Enable/disable external vmtrace for given domain */ +#define XEN_DOMCTL_vmtrace_pt_enable 1 +#define XEN_DOMCTL_vmtrace_pt_disable 2 +#define XEN_DOMCTL_vmtrace_pt_get_offset 3 + domid_t domain; + uint16_t pad1; + uint32_t vcpu; + uint16_t pad2; + + /* OUT variable */ + uint64_aligned_t size; + uint64_aligned_t offset; +}; +typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t; +DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t); + +#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ + struct xen_domctl { uint32_t cmd; #define XEN_DOMCTL_createdomain 1 @@ -1217,6 +1241,7 @@ struct xen_domctl { #define XEN_DOMCTL_vuart_op 81 #define XEN_DOMCTL_get_cpu_policy 82 #define XEN_DOMCTL_set_cpu_policy 83 +#define XEN_DOMCTL_vmtrace_op 84 #define XEN_DOMCTL_gdbsx_guestmemio 1000 #define XEN_DOMCTL_gdbsx_pausevcpu 1001 #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 @@ -1277,6 +1302,9 @@ struct xen_domctl { struct xen_domctl_monitor_op monitor_op; struct xen_domctl_psr_alloc psr_alloc; struct xen_domctl_vuart_op vuart_op; +#if defined(__XEN__) || defined(__XEN_TOOLS__) + struct xen_domctl_vmtrace_op vmtrace_op; +#endif uint8_t pad[128]; } u; };