Re: [PATCH v9 04/10] x86/cpu-policy: re-arrange no-VMX logic

[Jan Beulich <jbeulich@suse.com>]

On 07.04.2026 23:58, Andrew Cooper wrote:
> On 24/11/2025 2:59 pm, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu-policy.c
>> +++ b/xen/arch/x86/cpu-policy.c
>> @@ -812,19 +812,20 @@ static void __init calculate_hvm_max_pol
>>          if ( !cpu_has_vmx_xsaves )
>>              __clear_bit(X86_FEATURE_XSAVES, fs);
>>      }
>> +    else
>> +    {
>> +        /*
>> +         * Xen doesn't use PKS, so the guest support for it has opted to not use
>> +         * the VMCS load/save controls for efficiency reasons.  This depends on
>> +         * the exact vmentry/exit behaviour, so don't expose PKS in other
>> +         * situations until someone has cross-checked the behaviour for safety.
>> +         */
>> +        __clear_bit(X86_FEATURE_PKS, fs);
>> +    }
>>  
>>      if ( !cpu_has_vmx_msrlist )
>>          __clear_bit(X86_FEATURE_MSRLIST, fs);
>>  
>> -    /*
>> -     * Xen doesn't use PKS, so the guest support for it has opted to not use
>> -     * the VMCS load/save controls for efficiency reasons.  This depends on
>> -     * the exact vmentry/exit behaviour, so don't expose PKS in other
>> -     * situations until someone has cross-checked the behaviour for safety.
>> -     */
>> -    if ( !cpu_has_vmx )
>> -        __clear_bit(X86_FEATURE_PKS, fs);
>> -
>>      /* 
>>       * Make adjustments to possible (nested) virtualization features exposed
>>       * to the guest
>>
> 
> These clauses aren't logically doing the same thing.  So while the
> compiler can merge them, I don't think it's a good idea to do so at a
> source level.
> 
> The "if ( vmx ) " block we can just see the end of is for features which
> need to cross-check extra VMX capabilities.  Each of RDTSCP, INVPCID and
> XSAVES are #UD unless explicitly enabled.  MPX is the odd-one out,
> checking the load/save capabilities.
> 
> I suspect this list is incomplete.  These cross-checks shouldn't fail on
> real hardware, where the VMX capabilities should match the native
> features, but nested virt is rife with enumeration bugs here.
> 
> The second "if ( !vmx )" clause is different.  This is really "I wired
> up PKS based on how Intel works, and if AMD ever gains it it will
> definitely need context switching changes to work".  This is in lieu of
> having dedicated Intel/AMD annotations for features.

Right, and this is what I'm using the new "else" body for later in the
series.

> The MSRLIST addition from the prior patch is arguably misplaced, except
> it's trying to cover both of the aspects.

Why is it misplaced? Where else would you want it to go? The VMX aspect,
as you say, is included in cpu_has_vmx_msrlist, so it doesn't need
separate checking for VMX.

> AMD are making no obvious moves to add PKS, and I expect MSRLIST is even
> lower down their priority list.
> 
> Overall, we need checks here for every guest-visible feature which:
> * has VMCS/VMCB controls which are enumerated separately
> * needs new context switching considerations

Which in this series are USER-MSR and MSR-IMM. Hence this prereq change,
to simplify the additions there.

> Maybe the "if ( !vmx )" shouldn't really be written this way.  I'm open
> to suggestions, but making it an else block isn't a solution.

Well in this situation I guess it's you to make suggestions. I have made
mine by way of this submission.

Jan