x86/amd: Fixes for levelling setup

[PATCH 1/3] x86/amd: Fold another DE_CFG edit into amd_init_de_cfg()

Posted by Andrew Cooper 2 months, 1 week ago

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.

Fixes: d0c75dc4c028 ("x86/amd: Fix race editing DE_CFG")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

I apparently couldn't count how many examples we had editing DE_CFG...
---
 xen/arch/x86/cpu/amd.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 47c109f89804..4dc9157836ad 100644
--- 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));
-		}
 	}
 
 	/* AMD CPUs do not support SYSENTER outside of legacy mode. */
-- 
2.39.5

Re: [PATCH 1/3] x86/amd: Fold another DE_CFG edit into amd_init_de_cfg()

Posted by Jan Beulich 2 months ago

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.

> --- 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?

And I assume it is deliberate that the adjustment no longer is done when we're
running virtualized ourselves?

Both imo want making explicit in the description.

Jan

Re: [PATCH 1/3] x86/amd: Fold another DE_CFG edit into amd_init_de_cfg()

Posted by Andrew Cooper 2 weeks, 5 days ago

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 <andrew.cooper3@citrix.com>

Re: [PATCH 1/3] x86/amd: Fold another DE_CFG edit into amd_init_de_cfg()

Posted by Jan Beulich 2 weeks, 5 days ago

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 <andrew.cooper3@citrix.com>

Thanks, this reads good to me:
Acked-by: Jan Beulich <jbeulich@suse.com>

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