| Message ID | 20210121212718.2441-10-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement support for external IPT monitoring | expand |
On 21.01.2021 22:27, Andrew Cooper wrote: > From: Tamas K Lengyel <tamas.lengyel@intel.com> > > Implement vmtrace_reset_pt function. Properly set IPT > state for VM forks. > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> albeit it strikes me as somewhat odd that ... > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2408,6 +2408,16 @@ static int vmtrace_output_position(struct vcpu *v, uint64_t *pos) > return v->arch.hvm.vmx.ipt_active; > } > > +static int vmtrace_reset(struct vcpu *v) > +{ > + if ( !v->arch.hvm.vmx.ipt_active ) > + return -EINVAL; > + > + v->arch.msrs->rtit.output_offset = 0; > + v->arch.msrs->rtit.status = 0; > + return 0; > +} ... the function/hook return non-void, yet ... > --- a/xen/arch/x86/mm/mem_sharing.c > +++ b/xen/arch/x86/mm/mem_sharing.c > @@ -1632,6 +1632,8 @@ static int copy_vcpu_settings(struct domain *cd, const struct domain *d) > copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn); > } > > + hvm_vmtrace_reset(cd_vcpu); ... the only caller doesn't care. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, January 26, 2021 9:22 AM > To: Cooper, Andrew <andrew.cooper3@citrix.com> > Cc: Lengyel, Tamas <tamas.lengyel@intel.com>; Roger Pau Monné > <roger.pau@citrix.com>; Wei Liu <wl@xen.org>; Nakajima, Jun > <jun.nakajima@intel.com>; Tian, Kevin <kevin.tian@intel.com>; Michał > Leszczyński <michal.leszczynski@cert.pl>; Tamas K Lengyel > <tamas@tklengyel.com>; Xen-devel <xen-devel@lists.xenproject.org> > Subject: Re: [PATCH v7 09/10] xen/vmtrace: support for VM forks > > On 21.01.2021 22:27, Andrew Cooper wrote: > > From: Tamas K Lengyel <tamas.lengyel@intel.com> > > > > Implement vmtrace_reset_pt function. Properly set IPT state for VM > > forks. > > > > Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com> > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> albeit it strikes me as > somewhat odd that ... > > > --- a/xen/arch/x86/hvm/vmx/vmx.c > > +++ b/xen/arch/x86/hvm/vmx/vmx.c > > @@ -2408,6 +2408,16 @@ static int vmtrace_output_position(struct vcpu > *v, uint64_t *pos) > > return v->arch.hvm.vmx.ipt_active; } > > > > +static int vmtrace_reset(struct vcpu *v) { > > + if ( !v->arch.hvm.vmx.ipt_active ) > > + return -EINVAL; > > + > > + v->arch.msrs->rtit.output_offset = 0; > > + v->arch.msrs->rtit.status = 0; > > + return 0; > > +} > > ... the function/hook return non-void, yet ... > > > --- a/xen/arch/x86/mm/mem_sharing.c > > +++ b/xen/arch/x86/mm/mem_sharing.c > > @@ -1632,6 +1632,8 @@ static int copy_vcpu_settings(struct domain *cd, > const struct domain *d) > > copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn); > > } > > > > + hvm_vmtrace_reset(cd_vcpu); > > ... the only caller doesn't care. Noted. For now the function could just return void since if IPT is not enabled we don't care from a fork reset perspective. But perhaps in the future we want to return that information to the toolstack. Haven't decided yet whether that's important or not, for now things are OK as-is. Tamas
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index d4e7b50b8a..40ae398cf5 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2408,6 +2408,16 @@ static int vmtrace_output_position(struct vcpu *v, uint64_t *pos) return v->arch.hvm.vmx.ipt_active; } +static int vmtrace_reset(struct vcpu *v) +{ + if ( !v->arch.hvm.vmx.ipt_active ) + return -EINVAL; + + v->arch.msrs->rtit.output_offset = 0; + v->arch.msrs->rtit.status = 0; + return 0; +} + static struct hvm_function_table __initdata vmx_function_table = { .name = "VMX", .cpu_up_prepare = vmx_cpu_up_prepare, @@ -2467,6 +2477,7 @@ static struct hvm_function_table __initdata vmx_function_table = { .vmtrace_output_position = vmtrace_output_position, .vmtrace_set_option = vmtrace_set_option, .vmtrace_get_option = vmtrace_get_option, + .vmtrace_reset = vmtrace_reset, .tsc_scaling = { .max_ratio = VMX_TSC_MULTIPLIER_MAX, }, diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c index adaeab4612..fd07eb8d4d 100644 --- a/xen/arch/x86/mm/mem_sharing.c +++ b/xen/arch/x86/mm/mem_sharing.c @@ -1632,6 +1632,8 @@ static int copy_vcpu_settings(struct domain *cd, const struct domain *d) copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn); } + hvm_vmtrace_reset(cd_vcpu); + /* * TODO: to support VMs with PV interfaces copy additional * settings here, such as PV timers. @@ -1782,6 +1784,7 @@ static int fork(struct domain *cd, struct domain *d) cd->max_pages = d->max_pages; *cd->arch.cpuid = *d->arch.cpuid; *cd->arch.msr = *d->arch.msr; + cd->vmtrace_frames = d->vmtrace_frames; cd->parent = d; } diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 960ec03917..150746de66 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -219,6 +219,7 @@ struct hvm_function_table { int (*vmtrace_output_position)(struct vcpu *v, uint64_t *pos); int (*vmtrace_set_option)(struct vcpu *v, uint64_t key, uint64_t value); int (*vmtrace_get_option)(struct vcpu *v, uint64_t key, uint64_t *value); + int (*vmtrace_reset)(struct vcpu *v); /* * Parameters and callbacks for hardware-assisted TSC scaling, @@ -696,6 +697,14 @@ static inline int hvm_vmtrace_get_option( return -EOPNOTSUPP; } +static inline int hvm_vmtrace_reset(struct vcpu *v) +{ + if ( hvm_funcs.vmtrace_reset ) + return hvm_funcs.vmtrace_reset(v); + + return -EOPNOTSUPP; +} + /* * This must be defined as a macro instead of an inline function, * because it uses 'struct vcpu' and 'struct domain' which have