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: Wed, 22 Nov 2023 09:33:22 +0100	[thread overview]
Message-ID: <0c1720be-e0f6-46c2-a99f-97cf1bc747d0@suse.com> (raw)
In-Reply-To: <ZV26VGxA6ZZS7LAZ@macbook.local>

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