On 22.06.2020 04:49, Michał Leszczyński wrote: > ----- 19 cze 2020 o 15:44, Roger Pau Monné roger....@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.leszczyn...@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-jul...@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
