On 22.11.2023 09:22, Roger Pau Monné wrote: > On Tue, Nov 21, 2023 at 06:27:02PM +0100, Jan Beulich wrote: >> On 21.11.2023 17:24, Roger Pau Monné wrote: >>> On Thu, Nov 16, 2023 at 02:31:05PM +0100, Jan Beulich wrote: >>>> ... or we fail to enable the functionality on the BSP for other reasons. >>>> The only place where hardware announcing the feature is recorded is the >>>> raw CPU policy/featureset. >>>> >>>> Inspired by >>>> https://lore.kernel.org/all/[email protected]/. >>>> >>>> Signed-off-by: Jan Beulich <[email protected]> >>> >>> Acked-by: Roger Pau Monné <[email protected]> >> >> Thanks. >> >>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c >>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c >>>> @@ -2163,6 +2163,23 @@ int __init vmx_vmcs_init(void) >>>> >>>> if ( !ret ) >>>> register_keyhandler('v', vmcs_dump, "dump VT-x VMCSs", 1); >>>> + else >>>> + { >>>> + setup_clear_cpu_cap(X86_FEATURE_VMX); >>>> + >>>> + /* >>>> + * _vmx_vcpu_up() may have made it past feature identification. >>>> + * Make sure all dependent features are off as well. >>>> + */ >>>> + vmx_basic_msr = 0; >>>> + vmx_pin_based_exec_control = 0; >>>> + vmx_cpu_based_exec_control = 0; >>>> + vmx_secondary_exec_control = 0; >>>> + vmx_vmexit_control = 0; >>>> + vmx_vmentry_control = 0; >>>> + vmx_ept_vpid_cap = 0; >>>> + vmx_vmfunc = 0; >>> >>> Are there really any usages of those variables if VMX is disabled in >>> CPUID? >> >> I wanted to be on the safe side, as to me the question was "Are there really >> _no_ uses anywhere of those variables if VMX is disabled in CPUID?" And I >> couldn't easily convince myself of this being the case, seeing how all of >> vmcs.h's cpu_has_* are defined (and I'm pretty sure we have uses outside of >> arch/x86/hvm/vmx/). > > Wouldn't that have exploded already if initialization of _vmx_cpu_up() > failed? (regardless of whether the CPUID flag is cleared or not)
Quite likely, or in other words the clearing added here likely was missing before already. > My main concern is that it's very easy for the variables here getting > out of sync with the ones used by vmx_init_vmcs_config(). > > It might have been nice to place all those fields in an array that we > could just zero here without having to account for each individual > variable. Yeah, that might (have been) better. Indeed I already need to remember to correctly deal with vmx_tertiary_exec_control either here or in the patch introducing it. I guess I should make a follow-on patch converting to a struct and at the same time moving to __ro_after_init. Jan
