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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/11/2025 2:59 pm, Jan Beulich wrote:
> Move the PKS check into an "else" for the corresponding "if()", such
> that further adjustments (like for USER_MSR) can easily be put there as
> well.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v5: Re-base.
> v4: New.
>
> --- 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.

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


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

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.

~Andrew