On 21.01.2026 12:46, Andrew Cooper wrote:
> 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.
Hmm, this last part again leaves me with the impression that backport isn't
strictly needed. (We often don't when addressing only a latent issue.)
>>> --- 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]>
Thanks, this reads good to me:
Acked-by: Jan Beulich <[email protected]>
I'd like to understand the backporting (or not) aspect, though, in order to
properly know whether to pick this up once you put it in.
Jan
