On 08/12/2025 9:17 am, Jan Beulich wrote:
> On 02.12.2025 11:57, Andrew Cooper wrote:
>> Fam12h processors aren't SMT-capable so there are no race condition worries
>> with this path. Nevertheless, group it together with the other DE_CFG
>> modifications.
> With this, ...
>
>> Fixes: d0c75dc4c028 ("x86/amd: Fix race editing DE_CFG")
> ... isn't this more like Amends:? Aiui this wouldn't need backporting.
This does want backporting.
d0c75dc4c028 explains how it's buggy to have multiple pieces of logic
writing to DE_CFG, and here's yet another one.
It's only a latent bug because Fam12 doesn't have CMT/SMT, and this
property is not discussed, meaning that the logic as-is comes across as
unsafe.
>
>> --- a/xen/arch/x86/cpu/amd.c
>> +++ b/xen/arch/x86/cpu/amd.c
>> @@ -920,6 +920,13 @@ void amd_init_de_cfg(const struct cpuinfo_x86 *c)
>> if ( zenbleed_use_chickenbit() )
>> new |= (1 << 9);
>>
>> + /*
>> + * Erratum #665, doc 44739. Integer divide instructions may cause
>> + * unpredictable behaviour.
>> + */
>> + if ( c->family == 0x12 )
>> + new |= 1U << 31;
>> +
>> /* Avoid reading DE_CFG if we don't intend to change anything. */
>> if ( !new )
>> return;
>> @@ -1201,15 +1208,6 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
>> 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))) {
>> - if (c == &boot_cpu_data || opt_cpu_info)
>> - printk_once(XENLOG_WARNING
>> - "CPU%u: Applying workaround for
>> erratum 665\n",
>> - smp_processor_id());
>> - wrmsrl(MSR_AMD64_DE_CFG, value | (1U << 31));
>> - }
>> }
> Are you deliberately getting rid of the log message?
Yes, it's basically line noise.
Most errata like this are not handled at all, and this is not overly
noteworthy. If it were at debug level then maybe, but certainly not at
warning.
Fam12h were rare in the first place and are museum pieces these days.
> And I assume it is deliberate that the adjustment no longer is done when we're
> running virtualized ourselves?
Of course. It's pointless, and a readback would show that the write had
had no effect.
>
> Both imo want making explicit in the description.
I've updated the commit message to:
x86/amd: Fold another DE_CFG edit into amd_init_de_cfg()
As amd_init_de_cfg() states, it's only safe for there to be one location
writing to DE_CFG. This happens to be a latent bug rather than a real one
because Fam12h CPUs aren't SMT-capable. Nevertheless, group it together
with
the other DE_CFG modifications.
This removes a printk() which is not noteworthy, and skips the adjustment
entirely under virt, where the attempted write wouldn't have stuck anyway.
Fixes: d0c75dc4c028 ("x86/amd: Fix race editing DE_CFG")
Signed-off-by: Andrew Cooper <[email protected]>
