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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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)

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.

Thanks, Roger.