Re: [Xen-devel] [PATCH 1/6] x86/AMD Split init_amd() into per-uarch helpers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.12.18 at 13:39, <andrew.cooper3@xxxxxxxxxx> wrote:
> This reduces the complexity of init_amd(), and collects related
> workarounds together.
> 
> It also offers us the opportunity to stop performing workarounds when
> virtualised - doing so is wasteful, as it all involves poking MSRs which
> no hypervisor will let us touch in practice.

No _current_ hypervisor - perhaps. But a really good one might
emulate at least some of them.

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -534,22 +534,165 @@ static void early_init_amd(struct cpuinfo_x86 *c)
>       ctxt_switch_levelling(NULL);
>  }
>  
> +static void init_amd_k8(struct cpuinfo_x86 *c)
> +{
> +    uint64_t val;
> +
> +    /*
> +     * Skip errata workarounds if we are virtualised.  We won't have
> +     * sufficient control of hardware to do anything useful.
> +     */
> +    if ( !cpu_has_hypervisor )
> +        return;

Note how you then also skip ...

> +    /*
> +     * Disable TLB flush filter by setting HWCR.FFDIS bit 6
> +     *
> +     * Erratum 63 for SH-B3 steppings
> +     * Erratum 122 for all steppings (F+ have it disabled by default)
> +     */
> +    rdmsrl(MSR_K7_HWCR, val);
> +    if ( !(val & (1u << 6)) )
> +        wrmsrl(MSR_K7_HWCR, val | (1u << 6));
> +
> +    /*
> +     * Some BIOSes incorrectly force LAHF_LM, but only revisions D and later
> +     * actually support it.
> +     *
> +     * AMD Erratum #110, docId: 25759.
> +     */
> +    if ( c->x86_model < 0x14 && cpu_has(c, X86_FEATURE_LAHF_LM) )
> +    {
> +        unsigned int l, h;
> +
> +        __clear_bit(X86_FEATURE_LAHF_LM, c->x86_capability);

... this. If we expect lower level hypervisors to have fixed this already, we
should imo panic() here (and perhaps in further places) if we still find it 
wrong
(or what we consider wrong).

> +static void init_amd_k10(struct cpuinfo_x86 *c)
> +{
> +    uint64_t val;
> +
> +    /* Pointless to use MWAIT on Family10 as it does not deep sleep. */
> +    __clear_bit(X86_FEATURE_MONITOR, c->x86_capability);

See how you have this ahead of the virtualization check here?

> +static void init_amd_bulldozer(struct cpuinfo_x86 *c) /* Fam 15h */
> +{
> +    uint64_t val;
> +
> +    /*
> +     * Skip errata workarounds if we are virtualised.  We won't have
> +     * sufficient control of hardware to do anything useful.
> +     */
> +    if ( !cpu_has_hypervisor )
> +        return;
> +
> +    /* re-enable TopologyExtensions if switched off by BIOS */
> +    if ( c->x86_model >= 0x10 && c->x86_model <= 0x1f &&
> +         !cpu_has(c, X86_FEATURE_TOPOEXT) &&
> +         !rdmsr_safe(MSR_K8_EXT_FEATURE_MASK, val) )
> +    {
> +        __set_bit(X86_FEATURE_TOPOEXT, c->x86_capability);
> +        wrmsr_safe(MSR_K8_EXT_FEATURE_MASK, val | (1ull << 54));
> +    }

This is one of the cases where I'd expect a good hypervisor to permit control.
Independent of this I think now would be a good opportunity to __set_bit()
only if WRMSR actually managed to flip the bit.

> +    /*
> @@ -694,73 +798,9 @@ static void init_amd(struct cpuinfo_x86 *c)
>                      "*** Pass \"allow_unsafe\" if you're trusting"
>                      " all your (PV) guest kernels. ***\n");
>  
> -     if (c->x86 == 0x16 && c->x86_model <= 0xf) {
> -             if (c == &boot_cpu_data) {
> -                     l = pci_conf_read32(0, 0, 0x18, 0x3, 0x58);
> -                     h = pci_conf_read32(0, 0, 0x18, 0x3, 0x5c);
> -                     if ((l & 0x1f) | (h & 0x1))
> -                             printk(KERN_WARNING
> -                                    "Applying workaround for erratum 792: 
> %s%s%s\n",
> -                                    (l & 0x1f) ? "clearing D18F3x58[4:0]" : 
> "",
> -                                    ((l & 0x1f) && (h & 0x1)) ? " and " : "",
> -                                    (h & 0x1) ? "clearing D18F3x5C[0]" : "");
> -
> -                     if (l & 0x1f)
> -                             pci_conf_write32(0, 0, 0x18, 0x3, 0x58,
> -                                              l & ~0x1f);
> -
> -                     if (h & 0x1)
> -                             pci_conf_write32(0, 0, 0x18, 0x3, 0x5c,
> -                                              h & ~0x1);
> -             }
> -
> -             rdmsrl(MSR_AMD64_LS_CFG, value);
> -             if (!(value & (1 << 15))) {
> -                     static bool_t warned;
> -
> -                     if (c == &boot_cpu_data || opt_cpu_info ||
> -                         !test_and_set_bool(warned))
> -                             printk(KERN_WARNING
> -                                    "CPU%u: Applying workaround for erratum 
> 793\n",
> -                                    smp_processor_id());
> -                     wrmsrl(MSR_AMD64_LS_CFG, value | (1 << 15));
> -             }
> -     } else if (c->x86 == 0x12) {
> -             rdmsrl(MSR_AMD64_DE_CFG, value);
> -             if (!(value & (1U << 31))) {
> -                     static bool warned;
> -
> -                     if (c == &boot_cpu_data || opt_cpu_info ||
> -                         !test_and_set_bool(warned))
> -                             printk(KERN_WARNING
> -                                    "CPU%u: Applying workaround for erratum 
> 665\n",
> -                                    smp_processor_id());

While I agree with your consistency argument, messages like this one are meant 
to
hint at an outdated BIOS, which I think is helpful and hence I'm not convinced 
should
be dropped despite similar messages missing elsewhere.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel