Re: [PATCH 6/6] x86/boot: Expose MSR_ARCH_CAPS data in guest max policies

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.05.2023 16:42, Andrew Cooper wrote:
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -408,6 +408,25 @@ static void __init calculate_host_policy(void)
>      p->platform_info.cpuid_faulting = cpu_has_cpuid_faulting;
>  }
>  
> +static void __init guest_common_max_feature_adjustments(uint32_t *fs)
> +{
> +    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
> +    {
> +        /*
> +         * MSR_ARCH_CAPS is just feature data, and we can offer it to guests
> +         * unconditionally, although limit it to Intel systems as it is 
> highly
> +         * uarch-specific.
> +         *
> +         * In particular, the RSBA and RRSBA bits mean "you might migrate to 
> a
> +         * system where RSB underflow uses alternative predictors (a.k.a
> +         * Retpoline not safe)", so these need to be visible to a guest in 
> all
> +         * cases, even when it's only some other server in the pool which
> +         * suffers the identified behaviour.
> +         */
> +        __set_bit(X86_FEATURE_ARCH_CAPS, fs);
> +    }
> +}

The comment reads as if it wasn't applying to "max" only, but rather to
"default". Reading this I'm therefore now (and perhaps even more so in
the future, when coming across it) wondering whether it's misplaced, or
and hence whether the commented code is also misplaced and/or wrong.

Further is even just non-default exposure of all the various bits okay
to other than Dom0? IOW is there indeed no further adjustment necessary
to guest_rdmsr()?

> @@ -828,7 +845,10 @@ void __init init_dom0_cpuid_policy(struct domain *d)
>       * domain policy logic gains a better understanding of MSRs.
>       */
>      if ( is_hardware_domain(d) && cpu_has_arch_caps )
> +    {
>          p->feat.arch_caps = true;
> +        p->arch_caps.raw = host_cpu_policy.arch_caps.raw;
> +    }

Doesn't this expose all the bits, irrespective of their exposure
annotations in the public header? I.e. even more than just the two
bits that become 'A' in patch 4, but weren't ...

> @@ -858,20 +878,6 @@ void __init init_dom0_cpuid_policy(struct domain *d)
>          p->platform_info.cpuid_faulting = false;
>  
>      recalculate_cpuid_policy(d);
> -
> -    if ( is_hardware_domain(d) && cpu_has_arch_caps )
> -    {
> -        uint64_t val;
> -
> -        rdmsrl(MSR_ARCH_CAPABILITIES, val);
> -
> -        p->arch_caps.raw = val &
> -            (ARCH_CAPS_RDCL_NO | ARCH_CAPS_IBRS_ALL | ARCH_CAPS_RSBA |
> -             ARCH_CAPS_SSB_NO | ARCH_CAPS_MDS_NO | 
> ARCH_CAPS_IF_PSCHANGE_MC_NO |
> -             ARCH_CAPS_TAA_NO | ARCH_CAPS_SBDR_SSDP_NO | ARCH_CAPS_FBSDP_NO |
> -             ARCH_CAPS_PSDP_NO | ARCH_CAPS_FB_CLEAR | ARCH_CAPS_RRSBA |
> -             ARCH_CAPS_BHI_NO | ARCH_CAPS_PBRSB_NO);
> -    }

... included here?

Jan