[PATCH] x86/amd: Fix DE_CFG truncation in amd_check_zenbleed()

Andrew Cooper posted 1 patch 9 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230728181730.3065977-1-andrew.cooper3@citrix.com
xen/arch/x86/cpu/amd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] x86/amd: Fix DE_CFG truncation in amd_check_zenbleed()
Posted by Andrew Cooper 9 months, 2 weeks ago
This line:

	val &= ~chickenbit;

ends up truncating val to 32 bits, and turning off various errata workarounds
in Zen2 systems.

Fixes: f91c5ea97067 ("x86/amd: Mitigations for Zenbleed")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

The choice is between int or uint64_t.  This is one case where the insistence
on using unsigned int as a default data type is genuinely unsafe.
---
 xen/arch/x86/cpu/amd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 3ed06f670491..089038bf62c5 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -909,8 +909,9 @@ void __init detect_zen2_null_seg_behaviour(void)
 void amd_check_zenbleed(void)
 {
 	const struct cpu_signature *sig = &this_cpu(cpu_sig);
-	unsigned int good_rev, chickenbit = (1 << 9);
+	unsigned int good_rev;
 	uint64_t val, old_val;
+	int chickenbit = (1 << 9);
 
 	/*
 	 * If we're virtualised, we can't do family/model checks safely, and
-- 
2.30.2


Re: [PATCH] x86/amd: Fix DE_CFG truncation in amd_check_zenbleed()
Posted by Jan Beulich 9 months, 2 weeks ago
On 28.07.2023 20:17, Andrew Cooper wrote:
> This line:
> 
> 	val &= ~chickenbit;
> 
> ends up truncating val to 32 bits, and turning off various errata workarounds
> in Zen2 systems.
> 
> Fixes: f91c5ea97067 ("x86/amd: Mitigations for Zenbleed")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> The choice is between int or uint64_t.  This is one case where the insistence
> on using unsigned int as a default data type is genuinely unsafe.

It is not. The (unsigned) type should have been wide enough. From a Misra
perspective I'm pretty sure we would be better off using uint64_t. But in
the interest of getting this in without a lot of discussion I'll leave the
decision up to you; either way
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH] x86/amd: Fix DE_CFG truncation in amd_check_zenbleed()
Posted by Andrew Cooper 9 months, 2 weeks ago
On 31/07/2023 10:02 am, Jan Beulich wrote:
> On 28.07.2023 20:17, Andrew Cooper wrote:
>> This line:
>>
>> 	val &= ~chickenbit;
>>
>> ends up truncating val to 32 bits, and turning off various errata workarounds
>> in Zen2 systems.
>>
>> Fixes: f91c5ea97067 ("x86/amd: Mitigations for Zenbleed")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>>
>> The choice is between int or uint64_t.  This is one case where the insistence
>> on using unsigned int as a default data type is genuinely unsafe.
> It is not. The (unsigned) type should have been wide enough. From a Misra
> perspective I'm pretty sure we would be better off using uint64_t. But in
> the interest of getting this in without a lot of discussion I'll leave the
> decision up to you; either way
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Roger asked for uint64_t too so I'll go with that.  Thanks.

~Andrew