Re: [PATCH 2/3] x86: Add support for CpuidUserDis

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.05.2023 19:57, Alejandro Vallejo wrote:
> Includes a refactor to move vendor-specific probes to vendor-specific
> files.

I wonder whether the refactoring parts wouldn't better be split off.

> @@ -363,6 +375,21 @@ static void __init noinline amd_init_levelling(void)
>               ctxt_switch_masking = amd_ctxt_switch_masking;
>  }
>  
> +void amd_set_cpuid_user_dis(bool enable)
> +{
> +     const uint64_t msr_addr = MSR_K8_HWCR;

Nit: No MSR index is ever a 64-bit quantity. I'd recommend using MSR_K8_HWCR
directly in the two accesses below anyway, but otherwise the variable wants
to be "const unsigned int".

> +     const uint64_t bit = K8_HWCR_CPUID_USER_DIS;
> +     uint64_t val;
> +
> +     rdmsrl(msr_addr, val);
> +
> +     if (!!(val & bit) == enable)
> +             return;
> +
> +     val ^= bit;
> +     wrmsrl(msr_addr, val);
> +}
>[...]
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -226,8 +226,17 @@ static void cf_check intel_ctxt_switch_masking(const 
> struct vcpu *next)
>   */
>  static void __init noinline intel_init_levelling(void)
>  {
> -     if (probe_cpuid_faulting())
> +     /* Intel Fam0f is old enough that probing for CPUID faulting support

Nit: Style (/* on its own line).

> +      * introduces spurious #GP(0) when the appropriate MSRs are read,
> +      * so skip it altogether. In the case where Xen is virtualized these
> +      * MSRs may be emulated though, so we allow it in that case.
> +      */
> +     if ((cpu_has_hypervisor || boot_cpu_data.x86 !=0xf) &&

Nit: Style (blanks around binary operators). I'd also suggest swapping both
sides of the ||, to have the commonly true case first.

Jan