Re: [PATCH 3/3] x86/amd: Fix race editing DE_CFG

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2025 14:22, Andrew Cooper wrote:
> @@ -1075,6 +966,112 @@ static void cf_check fam17_disable_c6(void *arg)
>       wrmsrl(MSR_AMD_CSTATE_CFG, val & mask);
>  }
>  
> +static bool zenbleed_use_chickenbit(void)
> +{
> +    unsigned int curr_rev;
> +    uint8_t fixed_rev;
> +
> +    /*
> +     * If we're virtualised, we can't do family/model checks safely, and
> +     * we likely wouldn't have access to DE_CFG even if we could see a
> +     * microcode revision.
> +     *
> +     * A hypervisor may hide AVX as a stopgap mitigation.  We're not in a
> +     * position to care either way.  An admin doesn't want to be disabling
> +     * AVX as a mitigation on any build of Xen with this logic present.
> +     */
> +    if ( cpu_has_hypervisor || boot_cpu_data.family != 0x17 )
> +        return false;
> +
> +    curr_rev = this_cpu(cpu_sig).rev;
> +    switch ( curr_rev >> 8 )
> +    {
> +    case 0x083010: fixed_rev = 0x7a; break;
> +    case 0x086001: fixed_rev = 0x0b; break;
> +    case 0x086081: fixed_rev = 0x05; break;
> +    case 0x087010: fixed_rev = 0x32; break;
> +    case 0x08a000: fixed_rev = 0x08; break;
> +    default:
> +        /*
> +         * With the Fam17h check above, most parts getting here are Zen1.
> +         * They're not affected.  Assume Zen2 ones making it here are 
> affected
> +         * regardless of microcode version.
> +         */
> +        return is_zen2_uarch();
> +    }
> +
> +    return (uint8_t)curr_rev >= fixed_rev;
> +}
> +
> +void amd_init_de_cfg(const struct cpuinfo_x86 *c)
> +{
> +    uint64_t val, new = 0;
> +
> +    /* The MSR doesn't exist on Fam 0xf/0x11. */
> +    if ( c->family != 0xf && c->family != 0x11 )
> +        return;

Comment and code don't match. Did you mean

    if ( c->family == 0xf || c->family == 0x11 )
        return;

(along the lines of what you have in amd_init_lfence_dispatch())?

> +    /*
> +     * On Zen3 (Fam 0x19) and later CPUs, LFENCE is unconditionally dispatch
> +     * serialising, and is enumerated in CPUID.  Hypervisors may also
> +     * enumerate it when the setting is in place and MSR_AMD64_DE_CFG isn't
> +     * available.
> +     */
> +    if ( !test_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability) )
> +        new |= AMD64_DE_CFG_LFENCE_SERIALISE;
> +
> +    /*
> +     * If vulnerable to Zenbleed and not mitigated in microcode, use the
> +     * bigger hammer.
> +     */
> +    if ( zenbleed_use_chickenbit() )
> +        new |= (1 << 9);
> +
> +    if ( !new )
> +        return;
> +
> +    if ( rdmsr_safe(MSR_AMD64_DE_CFG, &val) ||
> +         (val & new) == new )
> +        return;
> +
> +    /*
> +     * DE_CFG is a Core-scoped MSR, and this write is racy.  However, both
> +     * threads calculate the new value from state which expected to be
> +     * consistent across CPUs and unrelated to the old value, so the result
> +     * should be consistent.
> +     */
> +    wrmsr_safe(MSR_AMD64_DE_CFG, val | new);

Either of the bits may be the cause of #GP. In that case we wouldn't set the
other bit, even if it may be possible to set it.

> +}
> +
> +void __init amd_init_lfence_dispatch(void)
> +{
> +    struct cpuinfo_x86 *c = &boot_cpu_data;
> +    uint64_t val;
> +
> +    if ( test_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability) )
> +        /* LFENCE is forced dispatch serialising and we can't control it. */
> +        return;
> +
> +    if ( c->family == 0xf || c->family == 0x11 )
> +        /* MSR doesn't exist.  LFENCE is dispatch serialising on this 
> hardare. */
> +        goto set;
> +
> +    if ( rdmsr_safe(MSR_AMD64_DE_CFG, &val) )
> +        /* Unable to read.  Assume the safer default. */
> +        goto clear;
> +
> +    if ( val & AMD64_DE_CFG_LFENCE_SERIALISE )
> +        /* Already dispatch serialising. */
> +        goto set;

Why "already", when this is now after we did the adjustment?

> + clear:
> +    setup_clear_cpu_cap(X86_FEATURE_LFENCE_DISPATCH);
> +    return;
> +
> + set:
> +    setup_force_cpu_cap(X86_FEATURE_LFENCE_DISPATCH);
> +}
> +
>  static void amd_check_bp_cfg(void)
>  {
>       uint64_t val, new = 0;
> @@ -1118,6 +1115,11 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
>       u32 l, h;
>       uint64_t value;
>  
> +        amd_init_de_cfg(c);
> +
> +        if (c == &boot_cpu_data)
> +            amd_init_lfence_dispatch(); /* Needs amd_init_de_cfg() */
> +
>       /* Disable TLB flush filter by setting HWCR.FFDIS on K8
>        * bit 6 of msr C001_0015
>        *

Nit: I don't think we want to mix indentation style within a function.

> --- a/xen/arch/x86/cpu/hygon.c
> +++ b/xen/arch/x86/cpu/hygon.c
> @@ -31,7 +31,11 @@ static void cf_check init_hygon(struct cpuinfo_x86 *c)
>  {
>       unsigned long long value;
>  
> -     amd_init_lfence(c);
> +        amd_init_de_cfg(c);
> +
> +        if (c == &boot_cpu_data)
> +            amd_init_lfence_dispatch(); /* Needs amd_init_de_cfg() */
> +
>       amd_init_ssbd(c);
>  
>       /* Probe for NSCB on Zen2 CPUs when not virtualised */

Same here then.

Jan