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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx/.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> 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