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
