Re: [PATCH 2/5] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	Kevin Tian <kevin.tian@intel.com>,
	Jun Nakajima <jun.nakajima@intel.com>
Subject: Re: [PATCH 2/5] x86/HVM: hide SVM/VMX when their enabling is prohibited by firmware
Date: Tue, 21 Nov 2023 18:27:02 +0100	[thread overview]
Message-ID: <d63dfc54-cf89-44b7-ab7e-983037892833@suse.com> (raw)
In-Reply-To: <ZVzZy-KYgdAsqRE3@macbook.local>

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/20230921114940.957141-1-pbonzini@redhat.com/.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

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/).

Jan