Re: [PATCH 3/3] x86: Use CpuidUserDis if an AMD HVM guest toggles CPUID faulting

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.05.2023 19:57, Alejandro Vallejo wrote:
> This is in order to aid guests of AMD hardware that we have exposed
> CPUID faulting to. If they try to modify the Intel MSR that enables
> the feature, trigger levelling so AMD's version of it (CpuidUserDis)
> is used instead.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
>  xen/arch/x86/msr.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Don't you also need to update cpu-policy.c:calculate_host_policy()
for the guest to actually know it can use the functionality? Which
in turn would appear to require some form of adjustment to
lib/x86/policy.c:x86_cpu_policies_are_compatible().

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -431,6 +431,13 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t 
> val)
>      {
>          bool old_cpuid_faulting = msrs->misc_features_enables.cpuid_faulting;
>  
> +        /*
> +         * The boot CPU must support Intel's CPUID faulting _or_
> +         * AMD's CpuidUserDis.
> +         */
> +        bool can_fault_cpuid = cpu_has_cpuid_faulting ||
> +                               boot_cpu_has(X86_FEATURE_CPUID_USER_DIS);

I'd like to suggest that in such a comment it not be emphasized that
it's the boot CPU (alone) we check. In fact I'm not convinced any
comment is needed here at all. I'm further inclined to suggest to
omit this (single-use) variable altogether.

Jan