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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.11.2025 18:56, Andrew Cooper wrote:
> On 26/11/2025 4:55 pm, Andrew Cooper wrote:
>> On 26/11/2025 3:07 pm, Jan Beulich wrote:
>>> 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())?
>> Oh - that was a last minute refactor which I didn't do quite correctly. 
>> Yes, it should match 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.
>> This MSR does not #GP on real hardware.

I consider this unexpected / inconsistent, at least as long as some of the
bits would be documented as reserved. "Would be" because the particular
Fam17 and Fam19 PPRs I'm looking at don't even mention DE_CFG (or BP_CFG,
for that matter).

>> Also, both of these bits come from instructions AMD have provided,
>> saying "set $X in case $Y", which we have honoured as part of the
>> conditions for setting up new, which I consider to be a reasonable
>> guarantee that no #GP will ensue.

The AMD instructions are for particular models, aren't they? While that
may mean the bits are fine to blindly (try to) set on other models, pretty
likely this can't be extended to other families. (While
zenbleed_use_chickenbit() is family-specific, the LFENCE bit is tried
without regard to family.)

>> This wrmsr_safe() is covering the virt case, because older Xen and
>> Byhive used to disallow writes to it, and OpenBSD would explode as a
>> consequence.  Xen's fix was 4175fd3ccd17.
>>
>> I toyed with the idea of having a tristate de_cfg_writeable, but that
>> got very ugly very quickly
>>
>> The other option would be to ignore DE_CFG entirely under virt.  That's
>> what we do for BP_CFG already, and no hypervisor is going to really let
>> us have access to it, and it would downgrade to non-safe variants.
> 
> In fact, ignoring the virt case for DE_CFG makes this generally nicer.

And being consistent with what we do with BP_CFG looks desirable to me as
well.

Jan