[PATCH 1/2] x86/microcode/AMD: Add a patch revision number union

[PATCH 1/2] x86/microcode/AMD: Add a patch revision number union

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

Add a structure which will be used to split the Zen generation of
microcode revision numbers into its corresponding elements. This will be
used to match microcode patches a lot easier and obviate the need for
a equivalence table.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Cc: John Allen <john.allen@amd.com>
---
 arch/x86/include/asm/microcode.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 695e569159c1..c1de0a6aefbc 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -47,6 +47,18 @@ struct microcode_intel {
 	unsigned int			bits[];
 };
 
+union zen_patch_rev {
+	struct {
+		__u32 rev	: 8,
+		      stepping	: 4,
+		      model	: 4,
+		      __resv    : 4,
+		      ext_model	: 4,
+		      ext_fam	: 8;
+	};
+	__u32 ucode_rev;
+};
+
 #define DEFAULT_UCODE_DATASIZE		(2000)
 #define MC_HEADER_SIZE			(sizeof(struct microcode_header_intel))
 #define MC_HEADER_TYPE_MICROCODE	1
-- 
2.43.0

[PATCH 2/2] x86/CPU/AMD: Improve the erratum 1386 workaround

[Borislav Petkov]

From: "Borislav Petkov (AMD)" <bp@alien8.de>

Disable XSAVES only on machines which haven't loaded the microcode
revision containing the erratum fix.

This will come in handy when running archaic OSes as guests. OSes whose
brilliant programmers thought that CPUID is overrated and one should not
query it but use features directly, ala shoot first, ask questions
later... but only if you're alive after the shooting.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
---
 arch/x86/kernel/cpu/amd.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 6d8677e80ddb..c02b07feff6e 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -15,6 +15,7 @@
 #include <asm/cpu.h>
 #include <asm/spec-ctrl.h>
 #include <asm/smp.h>
+#include <asm/microcode.h>
 #include <asm/numa.h>
 #include <asm/pci-direct.h>
 #include <asm/delay.h>
@@ -804,6 +805,16 @@ static void init_amd_bd(struct cpuinfo_x86 *c)
 
 static void fix_erratum_1386(struct cpuinfo_x86 *c)
 {
+	u8 fam, model, stpng, rev;
+	union zen_patch_rev p;
+
+	p.ucode_rev = c->microcode;
+
+	fam   = p.ext_fam + 0xf;
+	model = p.ext_model << 4 | p.model;
+	stpng = p.stepping;
+	rev   = p.rev;
+
 	/*
 	 * Work around Erratum 1386.  The XSAVES instruction malfunctions in
 	 * certain circumstances on Zen1/2 uarch, and not all parts have had
@@ -811,7 +822,24 @@ static void fix_erratum_1386(struct cpuinfo_x86 *c)
 	 *
 	 * Affected parts all have no supervisor XSAVE states, meaning that
 	 * the XSAVEC instruction (which works fine) is equivalent.
+	 *
+	 * Clear the feature flag only on microcode revisions which
+	 * don't have the fix.
 	 */
+	if (fam   == c->x86 &&
+	    model == c->x86_model &&
+	    stpng == c->x86_stepping) {
+		if (fam == 0x17) {
+			if (model == 0x1 && stpng == 0x2) {
+				if (rev >= 0x6e)
+					return;
+			} else if (model == 0x31 && stpng == 0x0) {
+				if (rev >= 0x52)
+					return;
+			}
+		}
+	}
+
 	clear_cpu_cap(c, X86_FEATURE_XSAVES);
 }
 
-- 
2.43.0

Re: [PATCH 2/2] x86/CPU/AMD: Improve the erratum 1386 workaround

[Maciej S. Szmigiero]

On 21.03.2024 13:05, Borislav Petkov wrote:
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> Disable XSAVES only on machines which haven't loaded the microcode
> revision containing the erratum fix.
> 
> This will come in handy when running archaic OSes as guests. OSes whose
> brilliant programmers thought that CPUID is overrated and one should not
> query it but use features directly, ala shoot first, ask questions
> later... but only if you're alive after the shooting.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> ---

Tested-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Thanks,
Maciej

[PATCH -v2] x86/CPU/AMD: Improve the erratum 1386 workaround

[Borislav Petkov]

On Thu, Mar 21, 2024 at 04:21:39PM +0100, Maciej S. Szmigiero wrote:
> On 21.03.2024 13:05, Borislav Petkov wrote:
> > From: "Borislav Petkov (AMD)" <bp@alien8.de>
> > 
> > Disable XSAVES only on machines which haven't loaded the microcode
> > revision containing the erratum fix.
> > 
> > This will come in handy when running archaic OSes as guests. OSes whose
> > brilliant programmers thought that CPUID is overrated and one should not
> > query it but use features directly, ala shoot first, ask questions
> > later... but only if you're alive after the shooting.
> > 
> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> > ---
> 
> Tested-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Yeah, thanks but still not good enough. Turns out we already have the
min microcode revision testing functionality so there's no need for me
to reinvent the wheel.

No harm no foul, tho, the stuff in those two previous patches I will use
in the microcode loader soon for simplifying the loading.

Here's one *final* variant, I promise! :-)

Sorry about that.

---
From: "Borislav Petkov (AMD)" <bp@alien8.de>
Date: Sun, 24 Mar 2024 20:51:35 +0100
Subject: [PATCH -v2] x86/CPU/AMD: Improve the erratum 1386 workaround

Disable XSAVES only on machines which haven't loaded the microcode
revision containing the erratum fix.

This will come in handy when running archaic OSes as guests. OSes whose
brilliant programmers thought that CPUID is overrated and one should not
query it but use features directly, ala shoot first, ask questions
later... but only if you're alive after the shooting.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
---
 arch/x86/include/asm/cpu_device_id.h |  8 ++++++++
 arch/x86/kernel/cpu/amd.c            | 12 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
index eb8fcede9e3b..bf4e065cf1e2 100644
--- a/arch/x86/include/asm/cpu_device_id.h
+++ b/arch/x86/include/asm/cpu_device_id.h
@@ -190,6 +190,14 @@ struct x86_cpu_desc {
 	.x86_microcode_rev	= (revision),			\
 }
 
+#define AMD_CPU_DESC(fam, model, stepping, revision) {		\
+	.x86_family		= (fam),			\
+	.x86_vendor		= X86_VENDOR_AMD,		\
+	.x86_model		= (model),			\
+	.x86_stepping		= (stepping),			\
+	.x86_microcode_rev	= (revision),			\
+}
+
 extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match);
 extern bool x86_cpu_has_min_microcode_rev(const struct x86_cpu_desc *table);
 
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 6d8677e80ddb..873f0fdc2ef8 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -13,6 +13,7 @@
 #include <asm/apic.h>
 #include <asm/cacheinfo.h>
 #include <asm/cpu.h>
+#include <asm/cpu_device_id.h>
 #include <asm/spec-ctrl.h>
 #include <asm/smp.h>
 #include <asm/numa.h>
@@ -802,6 +803,11 @@ static void init_amd_bd(struct cpuinfo_x86 *c)
 	clear_rdrand_cpuid_bit(c);
 }
 
+static const struct x86_cpu_desc erratum_1386_microcode[] = {
+	AMD_CPU_DESC(0x17,  0x1, 0x2, 0x0800126e),
+	AMD_CPU_DESC(0x17, 0x31, 0x0, 0x08301052),
+};
+
 static void fix_erratum_1386(struct cpuinfo_x86 *c)
 {
 	/*
@@ -811,7 +817,13 @@ static void fix_erratum_1386(struct cpuinfo_x86 *c)
 	 *
 	 * Affected parts all have no supervisor XSAVE states, meaning that
 	 * the XSAVEC instruction (which works fine) is equivalent.
+	 *
+	 * Clear the feature flag only on microcode revisions which
+	 * don't have the fix.
 	 */
+	if (x86_cpu_has_min_microcode_rev(erratum_1386_microcode))
+		return;
+
 	clear_cpu_cap(c, X86_FEATURE_XSAVES);
 }
 
-- 
2.43.0

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH -v2] x86/CPU/AMD: Improve the erratum 1386 workaround

[Maciej S. Szmigiero]

On 24.03.2024 21:05, Borislav Petkov wrote:
> On Thu, Mar 21, 2024 at 04:21:39PM +0100, Maciej S. Szmigiero wrote:
>> On 21.03.2024 13:05, Borislav Petkov wrote:
>>> From: "Borislav Petkov (AMD)" <bp@alien8.de>
>>>
>>> Disable XSAVES only on machines which haven't loaded the microcode
>>> revision containing the erratum fix.
>>>
>>> This will come in handy when running archaic OSes as guests. OSes whose
>>> brilliant programmers thought that CPUID is overrated and one should not
>>> query it but use features directly, ala shoot first, ask questions
>>> later... but only if you're alive after the shooting.
>>>
>>> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
>>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>> ---
>>
>> Tested-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> 
> Yeah, thanks but still not good enough. Turns out we already have the
> min microcode revision testing functionality so there's no need for me
> to reinvent the wheel.
> 
> No harm no foul, tho, the stuff in those two previous patches I will use
> in the microcode loader soon for simplifying the loading.
> 
> Here's one *final* variant, I promise! :-)
> 
> Sorry about that.

No problem, it happens :)

> ---
> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> Date: Sun, 24 Mar 2024 20:51:35 +0100
> Subject: [PATCH -v2] x86/CPU/AMD: Improve the erratum 1386 workaround
> 
> Disable XSAVES only on machines which haven't loaded the microcode
> revision containing the erratum fix.
> 
> This will come in handy when running archaic OSes as guests. OSes whose
> brilliant programmers thought that CPUID is overrated and one should not
> query it but use features directly, ala shoot first, ask questions
> later... but only if you're alive after the shooting.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> ---

I have tested the updated patch now, including negative test by
increasing the required microcode version by 10, and can confirm that
this version works properly too.

Thanks,
Maciej

Re: [PATCH -v2] x86/CPU/AMD: Improve the erratum 1386 workaround

[Borislav Petkov]

On Mon, Mar 25, 2024 at 01:45:58PM +0100, Maciej S. Szmigiero wrote:
> I have tested the updated patch now, including negative test by
> increasing the required microcode version by 10, and can confirm that
> this version works properly too.

Thanks, lemme queue it.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[tip: x86/cpu] x86/CPU/AMD: Improve the erratum 1386 workaround

[tip-bot2 for Borislav Petkov (AMD)]

The following commit has been merged into the x86/cpu branch of tip:

Commit-ID:     29ba89f1895285f06c333546882e0c5ae9a6df23
Gitweb:        https://git.kernel.org/tip/29ba89f1895285f06c333546882e0c5ae9a6df23
Author:        Borislav Petkov (AMD) <bp@alien8.de>
AuthorDate:    Sun, 24 Mar 2024 20:51:35 +01:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Mon, 25 Mar 2024 14:05:24 +01:00

x86/CPU/AMD: Improve the erratum 1386 workaround

Disable XSAVES only on machines which haven't loaded the microcode
revision containing the erratum fix.

This will come in handy when running archaic OSes as guests. OSes whose
brilliant programmers thought that CPUID is overrated and one should not
query it but use features directly, ala shoot first, ask questions
later... but only if you're alive after the shooting.

Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Tested-by: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Link: https://lore.kernel.org/r/20240324200525.GBZgCHhYFsBj12PrKv@fat_crate.local
---
 arch/x86/include/asm/cpu_device_id.h |  8 ++++++++
 arch/x86/kernel/cpu/amd.c            | 12 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
index eb8fced..bf4e065 100644
--- a/arch/x86/include/asm/cpu_device_id.h
+++ b/arch/x86/include/asm/cpu_device_id.h
@@ -190,6 +190,14 @@ struct x86_cpu_desc {
 	.x86_microcode_rev	= (revision),			\
 }
 
+#define AMD_CPU_DESC(fam, model, stepping, revision) {		\
+	.x86_family		= (fam),			\
+	.x86_vendor		= X86_VENDOR_AMD,		\
+	.x86_model		= (model),			\
+	.x86_stepping		= (stepping),			\
+	.x86_microcode_rev	= (revision),			\
+}
+
 extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match);
 extern bool x86_cpu_has_min_microcode_rev(const struct x86_cpu_desc *table);
 
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 6d8677e..873f0fd 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -13,6 +13,7 @@
 #include <asm/apic.h>
 #include <asm/cacheinfo.h>
 #include <asm/cpu.h>
+#include <asm/cpu_device_id.h>
 #include <asm/spec-ctrl.h>
 #include <asm/smp.h>
 #include <asm/numa.h>
@@ -802,6 +803,11 @@ static void init_amd_bd(struct cpuinfo_x86 *c)
 	clear_rdrand_cpuid_bit(c);
 }
 
+static const struct x86_cpu_desc erratum_1386_microcode[] = {
+	AMD_CPU_DESC(0x17,  0x1, 0x2, 0x0800126e),
+	AMD_CPU_DESC(0x17, 0x31, 0x0, 0x08301052),
+};
+
 static void fix_erratum_1386(struct cpuinfo_x86 *c)
 {
 	/*
@@ -811,7 +817,13 @@ static void fix_erratum_1386(struct cpuinfo_x86 *c)
 	 *
 	 * Affected parts all have no supervisor XSAVE states, meaning that
 	 * the XSAVEC instruction (which works fine) is equivalent.
+	 *
+	 * Clear the feature flag only on microcode revisions which
+	 * don't have the fix.
 	 */
+	if (x86_cpu_has_min_microcode_rev(erratum_1386_microcode))
+		return;
+
 	clear_cpu_cap(c, X86_FEATURE_XSAVES);
 }
 

Re: [PATCH 2/2] x86/CPU/AMD: Improve the erratum 1386 workaround

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> Disable XSAVES only on machines which haven't loaded the microcode
> revision containing the erratum fix.
> 
> This will come in handy when running archaic OSes as guests. OSes whose
> brilliant programmers thought that CPUID is overrated and one should not
> query it but use features directly, ala shoot first, ask questions
> later... but only if you're alive after the shooting.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> ---
>  arch/x86/kernel/cpu/amd.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 6d8677e80ddb..c02b07feff6e 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -15,6 +15,7 @@
>  #include <asm/cpu.h>
>  #include <asm/spec-ctrl.h>
>  #include <asm/smp.h>
> +#include <asm/microcode.h>
>  #include <asm/numa.h>
>  #include <asm/pci-direct.h>
>  #include <asm/delay.h>
> @@ -804,6 +805,16 @@ static void init_amd_bd(struct cpuinfo_x86 *c)
>  
>  static void fix_erratum_1386(struct cpuinfo_x86 *c)
>  {
> +	u8 fam, model, stpng, rev;
> +	union zen_patch_rev p;
> +
> +	p.ucode_rev = c->microcode;
> +
> +	fam   = p.ext_fam + 0xf;
> +	model = p.ext_model << 4 | p.model;
> +	stpng = p.stepping;

Please make this 'stepping' - we don't have a single 'stpng' variable name 
in the kernel currently, and that's very good so.

Thanks,

	Ingo

Re: [PATCH 1/2] x86/microcode/AMD: Add a patch revision number union

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> From: "Borislav Petkov (AMD)" <bp@alien8.de>
> 
> Add a structure which will be used to split the Zen generation of
> microcode revision numbers into its corresponding elements. This will be
> used to match microcode patches a lot easier and obviate the need for
> a equivalence table.
> 
> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
> Cc: John Allen <john.allen@amd.com>
> ---
>  arch/x86/include/asm/microcode.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
> index 695e569159c1..c1de0a6aefbc 100644
> --- a/arch/x86/include/asm/microcode.h
> +++ b/arch/x86/include/asm/microcode.h
> @@ -47,6 +47,18 @@ struct microcode_intel {
>  	unsigned int			bits[];
>  };
>  
> +union zen_patch_rev {
> +	struct {
> +		__u32 rev	: 8,
> +		      stepping	: 4,
> +		      model	: 4,
> +		      __resv    : 4,
> +		      ext_model	: 4,
> +		      ext_fam	: 8;
> +	};
> +	__u32 ucode_rev;
> +};

s/__resv/__reserved please?

Had to look twice (ok, I'm lying, had to look 3 times) whether it stood for 
'reserved' or some source of 'revision' / 'reservation' thing.

Thanks,

	Ingo