Re: [Xen-devel] [PATCH RFC 27/31] xen/x86: Rework Intel masking/faulting setup

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.12.15 at 22:24, <andrew.cooper3@xxxxxxxxxx> wrote:
> +     if (msr_basic)
> +             __probe_mask_msr(&msr_basic, LCAP_1cd, &cpumask_defaults._1cd);
> +
> +     if (msr_ext)
> +             __probe_mask_msr(&msr_ext, LCAP_e1cd, &cpumask_defaults.e1cd);
> +
> +     if (msr_xsave)
> +             __probe_mask_msr(&msr_xsave, LCAP_Da1, &cpumask_defaults.Da1);
> +
> +     /*
> +      * Don't bother warning about a mismatch if virtualised.  These MSRs
> +      * are not architectural and almost never virtualised.
> +      */
> +     if ((expected_levelling_cap == levelling_caps) ||
> +         cpu_has_hypervisor)
> +             return;
> +
> +     printk(XENLOG_WARNING "Mismatch between expected (%#x"
> +            ") and real (%#x) levelling caps: missing %#x\n",
> +            expected_levelling_cap, levelling_caps,
> +            (expected_levelling_cap ^ levelling_caps) & levelling_caps);
> +     printk(XENLOG_WARNING "Fam %#x, model %#x expected (%#x/%#x/%#x), "
> +            "got (%#x/%#x/%#x)\n", c->x86, c->x86_model,
> +            exp_msr_basic, exp_msr_ext, exp_msr_xsave,
> +            msr_basic, msr_ext, msr_xsave);

I may not have noticed the same on the AMD patch, but printing
zeros as "missing" MSR indexes seems strange to me. Why not
print the missing MSRs with their textual names, easing cross
referencing with the FlexMigration document?

> +/*
> + * opt_cpuid_mask_ecx/edx: cpuid.1[ecx, edx] feature mask.
> + * For example, E8400[Intel Core 2 Duo Processor series] ecx = 0x0008E3FD,
> + * edx = 0xBFEBFBFF when executing CPUID.EAX = 1 normally. If you want to
> + * 'rev down' to E8400, you can set these values in these Xen boot 
> parameters.
> + */
> +static void __init noinline intel_init_levelling(void)
> +{
> +     if ( !probe_intel_cpuid_faulting() )
> +             probe_masking_msrs();
> +
> +     if ( msr_basic )
> +     {
> +             cpumask_defaults._1cd =
> +                     ((u64)opt_cpuid_mask_edx << 32) | opt_cpuid_mask_ecx;
> +
> +             if ( !~(opt_cpuid_mask_ecx & opt_cpuid_mask_edx) )
>                       printk("Writing CPUID feature mask ecx:edx -> 
> %08x:%08x\n",
>                              opt_cpuid_mask_ecx, opt_cpuid_mask_edx);

Are these messages, without adjustment to their wording, not
going to be confusing? After all the intention is to not just write
a single, never modified value. E.g. better "Defaulting ..."?

> @@ -183,22 +237,13 @@ static void early_init_intel(struct cpuinfo_x86 *c)
>           (boot_cpu_data.x86_mask == 3 || boot_cpu_data.x86_mask == 4))
>               paddr_bits = 36;
>  
> -     if (c == &boot_cpu_data && c->x86 == 6) {
> -             if (probe_intel_cpuid_faulting())
> -                     __set_bit(X86_FEATURE_CPUID_FAULTING,
> -                               c->x86_capability);
> -     } else if (boot_cpu_has(X86_FEATURE_CPUID_FAULTING)) {
> -             BUG_ON(!probe_intel_cpuid_faulting());
> -             __set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);
> -     }
> +     if (c == &boot_cpu_data)
> +             intel_init_levelling();
> +
> +     if (test_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability))
> +            __set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability);

So you intentionally delete the validation of CPUID faulting being
available on APs? Also - indentation.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel