----- 2 lip 2020 o 10:34, Jan Beulich jbeul...@suse.com napisał(a): > On 02.07.2020 10:10, Roger Pau Monné wrote: >> On Wed, Jul 01, 2020 at 10:42:55PM +0100, Andrew Cooper wrote: >>> On 30/06/2020 13:33, Michał Leszczyński wrote: >>>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>>> index ca94c2bedc..b73d824357 100644 >>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>> @@ -291,6 +291,12 @@ static int vmx_init_vmcs_config(void) >>>> _vmx_cpu_based_exec_control &= >>>> ~(CPU_BASED_CR8_LOAD_EXITING | CPU_BASED_CR8_STORE_EXITING); >>>> >>>> + rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); >>>> + >>>> + /* Check whether IPT is supported in VMX operation. */ >>>> + vmtrace_supported = cpu_has_ipt && >>>> + (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); >>> >>> There is a subtle corner case here. vmx_init_vmcs_config() is called on >>> all CPUs, and is supposed to level things down safely if we find any >>> asymmetry. >>> >>> If instead you go with something like this: >>> >>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c >>> index b73d824357..6960109183 100644 >>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>> @@ -294,8 +294,8 @@ static int vmx_init_vmcs_config(void) >>> rdmsrl(MSR_IA32_VMX_MISC, _vmx_misc_cap); >>> >>> /* Check whether IPT is supported in VMX operation. */ >>> - vmtrace_supported = cpu_has_ipt && >>> - (_vmx_misc_cap & VMX_MISC_PT_SUPPORTED); >>> + if ( !(_vmx_misc_cap & VMX_MISC_PT_SUPPORTED) ) >>> + vmtrace_supported = false; >> >> This is also used during hotplug, so I'm not sure it's safe to turn >> vmtrace_supported off during runtime, where VMs might be already using >> it. IMO it would be easier to just set it on the BSP, and then refuse >> to bring up any AP that doesn't have the feature. > > +1 > > IOW I also don't think that "vmx_init_vmcs_config() ... is supposed to > level things down safely". Instead I think the expectation is for > CPU onlining to fail if a CPU lacks features compared to the BSP. As > can be implied from what Roger says, doing like what you suggest may > be fine during boot, but past that only at times where we know there's > no user of a certain feature, and where discarding the feature flag > won't lead to other inconsistencies (which may very well mean "never"). > > Jan
Ok, I will modify it in a way Roger suggested for the previous patch version. CPU onlining will fail if there is an inconsistency. Best regards, Michał Leszczyński CERT Polska
