| Message ID | 626789888.9820937.1592523621821.JavaMail.zimbra@cert.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement support for external IPT monitoring | expand |
On Fri, Jun 19, 2020 at 01:40:21AM +0200, Michał Leszczyński wrote: > Check if Intel Processor Trace feature is supported by current > processor. Define hvm_ipt_supported function. > > Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl> > --- We usually keep a shirt list of the changes between versions, so it's easier for the reviewers to know what changed. As an example: https://lore.kernel.org/xen-devel/20200613184132.11880-1-julien@xen.org/ > xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++ > xen/include/asm-x86/cpufeature.h | 1 + > xen/include/asm-x86/hvm/hvm.h | 9 +++++++++ > xen/include/asm-x86/hvm/vmx/vmcs.h | 1 + > xen/include/public/arch-x86/cpufeatureset.h | 1 + > 5 files changed, 16 insertions(+) > > diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c > index ca94c2bedc..8466ccb912 100644 > --- a/xen/arch/x86/hvm/vmx/vmcs.c > +++ b/xen/arch/x86/hvm/vmx/vmcs.c > @@ -315,6 +315,10 @@ static int vmx_init_vmcs_config(void) > if ( opt_ept_pml ) > opt |= SECONDARY_EXEC_ENABLE_PML; > > + /* Check whether IPT is supported in VMX operation */ > + hvm_funcs.pt_supported = cpu_has_ipt && > + ( _vmx_misc_cap & VMX_MISC_PT_SUPPORTED ); By the placement of this chunk you are tying IPT support to the secondary exec availability, but I don't think that's required? Ie: You should move the read of misc_cap to the top-level of the function and perform the VMX_MISC_PT_SUPPORTED check there also. Note that space inside parentheses is only required for conditions of 'if', 'for' and those kind of statements, here it's not required, so this should be: hvm_funcs.pt_supported = cpu_has_ipt && (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); I also think this should look like: if ( !smp_processor_id() ) hvm_funcs.pt_supported = cpu_has_ipt && (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); else if ( hvm_funcs.pt_supported && !(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED) ) { printk("VMX: IPT capabilities fatally differ between CPU%u and CPU0\n", smp_processor_id()); return -EINVAL; } So that you can detect mismatches between CPUs. Thanks, Roger.
----- 19 cze 2020 o 15:44, Roger Pau Monné roger.pau@citrix.com napisał(a): > On Fri, Jun 19, 2020 at 01:40:21AM +0200, Michał Leszczyński wrote: >> Check if Intel Processor Trace feature is supported by current >> processor. Define hvm_ipt_supported function. >> >> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl> >> --- > > We usually keep a shirt list of the changes between versions, so it's > easier for the reviewers to know what changed. As an example: > > https://lore.kernel.org/xen-devel/20200613184132.11880-1-julien@xen.org/ > There is a change list in the cover letter. Should I also add changelog for each individual patch? >> xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++ >> xen/include/asm-x86/cpufeature.h | 1 + >> xen/include/asm-x86/hvm/hvm.h | 9 +++++++++ >> xen/include/asm-x86/hvm/vmx/vmcs.h | 1 + >> xen/include/public/arch-x86/cpufeatureset.h | 1 + >> 5 files changed, 16 insertions(+) >> >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >> index ca94c2bedc..8466ccb912 100644 >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -315,6 +315,10 @@ static int vmx_init_vmcs_config(void) >> if ( opt_ept_pml ) >> opt |= SECONDARY_EXEC_ENABLE_PML; >> >> + /* Check whether IPT is supported in VMX operation */ >> + hvm_funcs.pt_supported = cpu_has_ipt && >> + ( _vmx_misc_cap & VMX_MISC_PT_SUPPORTED ); > > By the placement of this chunk you are tying IPT support to the > secondary exec availability, but I don't think that's required? > > Ie: You should move the read of misc_cap to the top-level of the > function and perform the VMX_MISC_PT_SUPPORTED check there also. > > Note that space inside parentheses is only required for conditions of > 'if', 'for' and those kind of statements, here it's not required, so > this should be: > > hvm_funcs.pt_supported = cpu_has_ipt && > (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); > > I also think this should look like: > > if ( !smp_processor_id() ) > hvm_funcs.pt_supported = cpu_has_ipt && > (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); > else if ( hvm_funcs.pt_supported && > !(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED) ) > { > printk("VMX: IPT capabilities fatally differ between CPU%u and CPU0\n", > smp_processor_id()); > return -EINVAL; > } > > > So that you can detect mismatches between CPUs. I will fix this. > > Thanks, Roger.
On Fri, Jun 19, 2020 at 04:22:28PM +0200, Michał Leszczyński wrote: > ----- 19 cze 2020 o 15:44, Roger Pau Monné roger.pau@citrix.com napisał(a): > > > On Fri, Jun 19, 2020 at 01:40:21AM +0200, Michał Leszczyński wrote: > >> Check if Intel Processor Trace feature is supported by current > >> processor. Define hvm_ipt_supported function. > >> > >> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl> > >> --- > > > > We usually keep a shirt list of the changes between versions, so it's > > easier for the reviewers to know what changed. As an example: > > > > https://lore.kernel.org/xen-devel/20200613184132.11880-1-julien@xen.org/ > > > > There is a change list in the cover letter. Should I also add changelog for > each individual patch? Oh, sorry, completely missed that. IMO yes, it's easier for reviewers because we then have the diff and the changelog at the same place. Changelogs in cover letters are also fine, but I would tend to only add big architectural changes there. Roger.
----- 19 cze 2020 o 15:44, Roger Pau Monné roger.pau@citrix.com napisał(a): > On Fri, Jun 19, 2020 at 01:40:21AM +0200, Michał Leszczyński wrote: >> Check if Intel Processor Trace feature is supported by current >> processor. Define hvm_ipt_supported function. >> >> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl> >> --- > > We usually keep a shirt list of the changes between versions, so it's > easier for the reviewers to know what changed. As an example: > > https://lore.kernel.org/xen-devel/20200613184132.11880-1-julien@xen.org/ > >> xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++ >> xen/include/asm-x86/cpufeature.h | 1 + >> xen/include/asm-x86/hvm/hvm.h | 9 +++++++++ >> xen/include/asm-x86/hvm/vmx/vmcs.h | 1 + >> xen/include/public/arch-x86/cpufeatureset.h | 1 + >> 5 files changed, 16 insertions(+) >> >> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >> index ca94c2bedc..8466ccb912 100644 >> --- a/xen/arch/x86/hvm/vmx/vmcs.c >> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >> @@ -315,6 +315,10 @@ static int vmx_init_vmcs_config(void) >> if ( opt_ept_pml ) >> opt |= SECONDARY_EXEC_ENABLE_PML; >> >> + /* Check whether IPT is supported in VMX operation */ >> + hvm_funcs.pt_supported = cpu_has_ipt && >> + ( _vmx_misc_cap & VMX_MISC_PT_SUPPORTED ); > > By the placement of this chunk you are tying IPT support to the > secondary exec availability, but I don't think that's required? > > Ie: You should move the read of misc_cap to the top-level of the > function and perform the VMX_MISC_PT_SUPPORTED check there also. > > Note that space inside parentheses is only required for conditions of > 'if', 'for' and those kind of statements, here it's not required, so > this should be: > > hvm_funcs.pt_supported = cpu_has_ipt && > (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); > > I also think this should look like: > > if ( !smp_processor_id() ) > hvm_funcs.pt_supported = cpu_has_ipt && > (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); > else if ( hvm_funcs.pt_supported && > !(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED) ) > { > printk("VMX: IPT capabilities fatally differ between CPU%u and CPU0\n", > smp_processor_id()); > return -EINVAL; > } > > > So that you can detect mismatches between CPUs. I'm afraid this snippet doesn't work. All CPUs read hvm_funcs.pt_supported as 0 even when it was set to 1 for CPU=0. I'm not sure if this is some multithreading issue or there is a separate hvm_funcs for each CPU? ml > > Thanks, Roger.
On 22.06.2020 04:49, Michał Leszczyński wrote: > ----- 19 cze 2020 o 15:44, Roger Pau Monné roger.pau@citrix.com napisał(a): > >> On Fri, Jun 19, 2020 at 01:40:21AM +0200, Michał Leszczyński wrote: >>> Check if Intel Processor Trace feature is supported by current >>> processor. Define hvm_ipt_supported function. >>> >>> Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl> >>> --- >> >> We usually keep a shirt list of the changes between versions, so it's >> easier for the reviewers to know what changed. As an example: >> >> https://lore.kernel.org/xen-devel/20200613184132.11880-1-julien@xen.org/ >> >>> xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++ >>> xen/include/asm-x86/cpufeature.h | 1 + >>> xen/include/asm-x86/hvm/hvm.h | 9 +++++++++ >>> xen/include/asm-x86/hvm/vmx/vmcs.h | 1 + >>> xen/include/public/arch-x86/cpufeatureset.h | 1 + >>> 5 files changed, 16 insertions(+) >>> >>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>> index ca94c2bedc..8466ccb912 100644 >>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>> @@ -315,6 +315,10 @@ static int vmx_init_vmcs_config(void) >>> if ( opt_ept_pml ) >>> opt |= SECONDARY_EXEC_ENABLE_PML; >>> >>> + /* Check whether IPT is supported in VMX operation */ >>> + hvm_funcs.pt_supported = cpu_has_ipt && >>> + ( _vmx_misc_cap & VMX_MISC_PT_SUPPORTED ); >> >> By the placement of this chunk you are tying IPT support to the >> secondary exec availability, but I don't think that's required? >> >> Ie: You should move the read of misc_cap to the top-level of the >> function and perform the VMX_MISC_PT_SUPPORTED check there also. >> >> Note that space inside parentheses is only required for conditions of >> 'if', 'for' and those kind of statements, here it's not required, so >> this should be: >> >> hvm_funcs.pt_supported = cpu_has_ipt && >> (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); >> >> I also think this should look like: >> >> if ( !smp_processor_id() ) >> hvm_funcs.pt_supported = cpu_has_ipt && >> (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); >> else if ( hvm_funcs.pt_supported && >> !(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED) ) >> { >> printk("VMX: IPT capabilities fatally differ between CPU%u and CPU0\n", >> smp_processor_id()); >> return -EINVAL; >> } >> >> >> So that you can detect mismatches between CPUs. > > > I'm afraid this snippet doesn't work. All CPUs read hvm_funcs.pt_supported as 0 even when it was set to 1 for CPU=0. I'm not sure if this is some multithreading issue or there is a separate hvm_funcs for each CPU? hvm_funcs will be set from start_vmx()'s return value, i.e. vmx_function_table. Therefore at least prior to start_vmx() completing you need to fiddle with vmx_function_table, not hvm_funcs. And in the code here, where for APs you only read the value, you could easily use the former uniformly, I think. Jan
On 19.06.2020 01:40, Michał Leszczyński wrote: > @@ -630,6 +633,12 @@ static inline bool hvm_altp2m_supported(void) > return hvm_funcs.altp2m_supported; > } > > +/* returns true if hardware supports Intel Processor Trace */ > +static inline bool hvm_pt_supported(void) > +{ > + return hvm_funcs.pt_supported; > +} This is vendor-independent code, hence I'd like to see "Intel" dropped from the comment. > --- a/xen/include/asm-x86/hvm/vmx/vmcs.h > +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h > @@ -285,6 +285,7 @@ extern u64 vmx_ept_vpid_cap; > > #define VMX_MISC_CR3_TARGET 0x01ff0000 > #define VMX_MISC_VMWRITE_ALL 0x20000000 > +#define VMX_MISC_PT_SUPPORTED 0x00004000 Move this up by two lines, so the values continue to be numerically sorted? Jan
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c index ca94c2bedc..8466ccb912 100644 --- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -315,6 +315,10 @@ static int vmx_init_vmcs_config(void) if ( opt_ept_pml ) opt |= SECONDARY_EXEC_ENABLE_PML; + /* Check whether IPT is supported in VMX operation */ + hvm_funcs.pt_supported = cpu_has_ipt && + ( _vmx_misc_cap & VMX_MISC_PT_SUPPORTED ); + /* * "APIC Register Virtualization" and "Virtual Interrupt Delivery" * can be set only when "use TPR shadow" is set diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h index f790d5c1f8..8d7955dd87 100644 --- a/xen/include/asm-x86/cpufeature.h +++ b/xen/include/asm-x86/cpufeature.h @@ -104,6 +104,7 @@ #define cpu_has_clwb boot_cpu_has(X86_FEATURE_CLWB) #define cpu_has_avx512er boot_cpu_has(X86_FEATURE_AVX512ER) #define cpu_has_avx512cd boot_cpu_has(X86_FEATURE_AVX512CD) +#define cpu_has_ipt boot_cpu_has(X86_FEATURE_IPT) #define cpu_has_sha boot_cpu_has(X86_FEATURE_SHA) #define cpu_has_avx512bw boot_cpu_has(X86_FEATURE_AVX512BW) #define cpu_has_avx512vl boot_cpu_has(X86_FEATURE_AVX512VL) diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index 1eb377dd82..8c0d0ece67 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -96,6 +96,9 @@ struct hvm_function_table { /* Necessary hardware support for alternate p2m's? */ bool altp2m_supported; + /* Hardware support for processor tracing? */ + bool pt_supported; + /* Hardware virtual interrupt delivery enable? */ bool virtual_intr_delivery_enabled; @@ -630,6 +633,12 @@ static inline bool hvm_altp2m_supported(void) return hvm_funcs.altp2m_supported; } +/* returns true if hardware supports Intel Processor Trace */ +static inline bool hvm_pt_supported(void) +{ + return hvm_funcs.pt_supported; +} + /* updates the current hardware p2m */ static inline void altp2m_vcpu_update_p2m(struct vcpu *v) { diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h index 906810592f..4c81093aba 100644 --- a/xen/include/asm-x86/hvm/vmx/vmcs.h +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h @@ -285,6 +285,7 @@ extern u64 vmx_ept_vpid_cap; #define VMX_MISC_CR3_TARGET 0x01ff0000 #define VMX_MISC_VMWRITE_ALL 0x20000000 +#define VMX_MISC_PT_SUPPORTED 0x00004000 #define VMX_TSC_MULTIPLIER_MAX 0xffffffffffffffffULL diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h index 5ca35d9d97..0d3f15f628 100644 --- a/xen/include/public/arch-x86/cpufeatureset.h +++ b/xen/include/public/arch-x86/cpufeatureset.h @@ -217,6 +217,7 @@ XEN_CPUFEATURE(SMAP, 5*32+20) /*S Supervisor Mode Access Prevention */ XEN_CPUFEATURE(AVX512_IFMA, 5*32+21) /*A AVX-512 Integer Fused Multiply Add */ XEN_CPUFEATURE(CLFLUSHOPT, 5*32+23) /*A CLFLUSHOPT instruction */ XEN_CPUFEATURE(CLWB, 5*32+24) /*A CLWB instruction */ +XEN_CPUFEATURE(IPT, 5*32+25) /* Intel Processor Trace */ XEN_CPUFEATURE(AVX512PF, 5*32+26) /*A AVX-512 Prefetch Instructions */ XEN_CPUFEATURE(AVX512ER, 5*32+27) /*A AVX-512 Exponent & Reciprocal Instrs */ XEN_CPUFEATURE(AVX512CD, 5*32+28) /*A AVX-512 Conflict Detection Instrs */
Check if Intel Processor Trace feature is supported by current processor. Define hvm_ipt_supported function. Signed-off-by: Michal Leszczynski <michal.leszczynski@cert.pl> --- xen/arch/x86/hvm/vmx/vmcs.c | 4 ++++ xen/include/asm-x86/cpufeature.h | 1 + xen/include/asm-x86/hvm/hvm.h | 9 +++++++++ xen/include/asm-x86/hvm/vmx/vmcs.h | 1 + xen/include/public/arch-x86/cpufeatureset.h | 1 + 5 files changed, 16 insertions(+)