[PATCH] x86: Fix Intel microcode revision detection

[PATCH] x86: Fix Intel microcode revision detection

[Junichi Nomura]

early_init_intel() calls sync_core() before rdmsr(MSR_IA32_UCODE_REV),
assuming sync_core() is effectively CPUID(eax=1). However the assumption
no longer holds since commit c198b121b1a1 ("x86/asm: Rewrite sync_core()
to use IRET-to-self").

As a result, kernel fails to detect the revision of microcode, such as:
  microcode: sig=0x206a7, pf=0x2, revision=0x0

Conversion from sync_core() to native_cpuid() has already been done in
Intel microcode driver by commit 484d0e5c7943 ("x86/microcode/intel:
Replace sync_core() with native_cpuid()"). This patch just extends the
conversion for early_init_intel().

Fixes: c198b121b1a1 ("x86/asm: Rewrite sync_core() to use IRET-to-self")
Signed-off-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Cc: Andy Lutomirski <luto@amacapital.net>

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 195becc..19cb4c4 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -66,4 +66,24 @@ static inline void show_ucode_info_early(void) {}
 static inline void reload_ucode_intel(void) {}
 #endif
 
+static inline void intel_microcode_cpuid_1(void)
+{
+	/*
+	 * According to the Intel SDM, Volume 3, 9.11.7:
+	 *
+	 *   CPUID returns a value in a model specific register in
+	 *   addition to its usual register return values. The
+	 *   semantics of CPUID cause it to deposit an update ID value
+	 *   in the 64-bit model-specific register at address 08BH
+	 *   (IA32_BIOS_SIGN_ID). If no update is present in the
+	 *   processor, the value in the MSR remains unmodified.
+	 *
+	 * Use native_cpuid -- this code runs very early and we don't
+	 * want to mess with paravirt.
+	 */
+	unsigned int eax = 1, ebx, ecx = 0, edx;
+
+	native_cpuid(&eax, &ebx, &ecx, &edx);
+}
+
 #endif /* _ASM_X86_MICROCODE_INTEL_H */
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fcd484d..edc38a9 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -14,6 +14,7 @@
 #include <asm/bugs.h>
 #include <asm/cpu.h>
 #include <asm/intel-family.h>
+#include <asm/microcode_intel.h>
 
 #ifdef CONFIG_X86_64
 #include <linux/topology.h>
@@ -83,7 +84,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 
 		wrmsr(MSR_IA32_UCODE_REV, 0, 0);
 		/* Required by the SDM */
-		sync_core();
+		intel_microcode_cpuid_1();
 		rdmsr(MSR_IA32_UCODE_REV, lower_word, c->microcode);
 	}
 
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index b624b54..546c6cf 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -368,26 +368,6 @@ static int microcode_sanity_check(void *mc, int print_err)
 	return patch;
 }
 
-static void cpuid_1(void)
-{
-	/*
-	 * According to the Intel SDM, Volume 3, 9.11.7:
-	 *
-	 *   CPUID returns a value in a model specific register in
-	 *   addition to its usual register return values. The
-	 *   semantics of CPUID cause it to deposit an update ID value
-	 *   in the 64-bit model-specific register at address 08BH
-	 *   (IA32_BIOS_SIGN_ID). If no update is present in the
-	 *   processor, the value in the MSR remains unmodified.
-	 *
-	 * Use native_cpuid -- this code runs very early and we don't
-	 * want to mess with paravirt.
-	 */
-	unsigned int eax = 1, ebx, ecx = 0, edx;
-
-	native_cpuid(&eax, &ebx, &ecx, &edx);
-}
-
 static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 {
 	unsigned int val[2];
@@ -413,7 +393,7 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	cpuid_1();
+	intel_microcode_cpuid_1();
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
@@ -613,7 +593,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	cpuid_1();
+	intel_microcode_cpuid_1();
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
@@ -825,7 +805,7 @@ static int apply_microcode_intel(int cpu)
 	wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	cpuid_1();
+	intel_microcode_cpuid_1();
 
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

Re: [PATCH] x86: Fix Intel microcode revision detection

[Borislav Petkov]

On Wed, Dec 28, 2016 at 04:39:31AM +0000, Junichi Nomura wrote:
> early_init_intel() calls sync_core() before rdmsr(MSR_IA32_UCODE_REV),
> assuming sync_core() is effectively CPUID(eax=1). However the assumption
> no longer holds since commit c198b121b1a1 ("x86/asm: Rewrite sync_core()
> to use IRET-to-self").
> 
> As a result, kernel fails to detect the revision of microcode, such as:
>   microcode: sig=0x206a7, pf=0x2, revision=0x0
> 
> Conversion from sync_core() to native_cpuid() has already been done in
> Intel microcode driver by commit 484d0e5c7943 ("x86/microcode/intel:
> Replace sync_core() with native_cpuid()"). This patch just extends the
> conversion for early_init_intel().

Good catch, thanks!

However, I want to do something else because we do end up needing those
native CPUID variants pretty often after all so let's generalize the
whole usage case and do the following (patches as reply to this message).

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[PATCH 1/2] x86/CPU: Add native CPUID variants returning a single datum

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

... similar to the cpuid_<reg>() variants.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/processor.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index eaf100508c36..27ae83fc37de 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -219,6 +219,24 @@ static inline void native_cpuid(unsigned int *eax, unsigned int *ebx,
 	    : "memory");
 }
 
+#define native_cpuid_reg(reg)					\
+static inline unsigned int native_cpuid_##reg(unsigned int op)	\
+{								\
+	unsigned int eax = (op), ebx, ecx = 0, edx;		\
+								\
+	native_cpuid(&eax, &ebx, &ecx, &edx);			\
+								\
+	return reg;						\
+}
+
+/*
+ * Native CPUID functions returning a single datum.
+ */
+native_cpuid_reg(eax)
+native_cpuid_reg(ebx)
+native_cpuid_reg(ecx)
+native_cpuid_reg(edx)
+
 static inline void load_cr3(pgd_t *pgdir)
 {
 	write_cr3(__pa(pgdir));
-- 
2.8.4

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[PATCH 2/2] x86/microcode: Use native CPUID to tickle out microcode revision

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Intel supplies the microcode revision value in MSR 0x8b
(IA32_BIOS_SIGN_ID) after CPUID(1) has been executed. Execute it each
time before reading that MSR.

It used to do sync_core() which did do CPUID but

  c198b121b1a1 ("x86/asm: Rewrite sync_core() to use IRET-to-self")

changed the sync_core() implementation so we better make the microcode
loading case explicit, as the SDM documents it.

Reported-by: Jun'ichi Nomura <j-nomura@ce.jp.nec.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/intel.c           |  2 +-
 arch/x86/kernel/cpu/microcode/intel.c | 26 +++-----------------------
 2 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fcd484d2bb03..2d49aa949fa1 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -83,7 +83,7 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 
 		wrmsr(MSR_IA32_UCODE_REV, 0, 0);
 		/* Required by the SDM */
-		sync_core();
+		native_cpuid_eax(1);
 		rdmsr(MSR_IA32_UCODE_REV, lower_word, c->microcode);
 	}
 
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index b624b54912e1..f79249fab389 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -368,26 +368,6 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
 	return patch;
 }
 
-static void cpuid_1(void)
-{
-	/*
-	 * According to the Intel SDM, Volume 3, 9.11.7:
-	 *
-	 *   CPUID returns a value in a model specific register in
-	 *   addition to its usual register return values. The
-	 *   semantics of CPUID cause it to deposit an update ID value
-	 *   in the 64-bit model-specific register at address 08BH
-	 *   (IA32_BIOS_SIGN_ID). If no update is present in the
-	 *   processor, the value in the MSR remains unmodified.
-	 *
-	 * Use native_cpuid -- this code runs very early and we don't
-	 * want to mess with paravirt.
-	 */
-	unsigned int eax = 1, ebx, ecx = 0, edx;
-
-	native_cpuid(&eax, &ebx, &ecx, &edx);
-}
-
 static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 {
 	unsigned int val[2];
@@ -413,7 +393,7 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	cpuid_1();
+	native_cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
@@ -613,7 +593,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	cpuid_1();
+	native_cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
@@ -825,7 +805,7 @@ static int apply_microcode_intel(int cpu)
 	wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	cpuid_1();
+	native_cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
-- 
2.8.4



-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[PATCH 3/2] x86/microcode/intel: Add a helper which gives the microcode revision

[Borislav Petkov]

On Wed, Dec 28, 2016 at 12:21:20PM +0100, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Intel supplies the microcode revision value in MSR 0x8b
> (IA32_BIOS_SIGN_ID) after CPUID(1) has been executed. Execute it each
> time before reading that MSR.

And then, we can go a step further and even do a separate helper which does the
required steps to read out the microcode revision so that we don't forget them
next time we change the code.

Provided that works for xen, though, because I need to do the native
variants but early_init_intel() can call the paravirt *msr() versions
and I have no idea whether that's kosher on xen pv.

Boris, any objections?

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 28 Dec 2016 13:44:56 +0100
Subject: [PATCH] x86/microcode/intel: Add a helper which gives the microcode
 revision

Since on Intel we're required to do CPUID(1) first, before reading
the microcode revision MSR, let's add a special helper which does the
required steps so that we don't forget to do them next time, when we
want to read the microcode revision.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/microcode_intel.h | 15 ++++++++++++
 arch/x86/kernel/cpu/intel.c            | 11 +++------
 arch/x86/kernel/cpu/microcode/intel.c  | 43 ++++++++++------------------------
 3 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 195becc6f780..e793fc9a9b20 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -52,6 +52,21 @@ struct extended_sigtable {
 
 #define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
 
+static inline u32 intel_get_microcode_revision(void)
+{
+	u32 rev, dummy;
+
+	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
+
+	/* As documented in the SDM: Do a CPUID 1 here */
+	native_cpuid_eax(1);
+
+	/* get the current revision from MSR 0x8B */
+	native_rdmsr(MSR_IA32_UCODE_REV, dummy, rev);
+
+	return rev;
+}
+
 #ifdef CONFIG_MICROCODE_INTEL
 extern void __init load_ucode_intel_bsp(void);
 extern void load_ucode_intel_ap(void);
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 2d49aa949fa1..203f860d2ab3 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -14,6 +14,7 @@
 #include <asm/bugs.h>
 #include <asm/cpu.h>
 #include <asm/intel-family.h>
+#include <asm/microcode_intel.h>
 
 #ifdef CONFIG_X86_64
 #include <linux/topology.h>
@@ -78,14 +79,8 @@ static void early_init_intel(struct cpuinfo_x86 *c)
 		(c->x86 == 0x6 && c->x86_model >= 0x0e))
 		set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
 
-	if (c->x86 >= 6 && !cpu_has(c, X86_FEATURE_IA64)) {
-		unsigned lower_word;
-
-		wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-		/* Required by the SDM */
-		native_cpuid_eax(1);
-		rdmsr(MSR_IA32_UCODE_REV, lower_word, c->microcode);
-	}
+	if (c->x86 >= 6 && !cpu_has(c, X86_FEATURE_IA64))
+		c->microcode = intel_get_microcode_revision();
 
 	/*
 	 * Atom erratum AAE44/AAF40/AAG38/AAH41:
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index f79249fab389..faec8fa68ffd 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -390,15 +390,8 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 		native_rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
 		csig.pf = 1 << ((val[1] >> 18) & 7);
 	}
-	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
-	/* As documented in the SDM: Do a CPUID 1 here */
-	native_cpuid_eax(1);
-
-	/* get the current revision from MSR 0x8B */
-	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
-
-	csig.rev = val[1];
+	csig.rev = intel_get_microcode_revision();
 
 	uci->cpu_sig = csig;
 	uci->valid = 1;
@@ -582,7 +575,7 @@ static inline void print_ucode(struct ucode_cpu_info *uci)
 static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 {
 	struct microcode_intel *mc;
-	unsigned int val[2];
+	u32 rev;
 
 	mc = uci->mc;
 	if (!mc)
@@ -590,21 +583,16 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 
 	/* write microcode via MSR 0x79 */
 	native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
-	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
-	/* As documented in the SDM: Do a CPUID 1 here */
-	native_cpuid_eax(1);
-
-	/* get the current revision from MSR 0x8B */
-	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
-	if (val[1] != mc->hdr.rev)
+	rev = intel_get_microcode_revision();
+	if (rev != mc->hdr.rev)
 		return -1;
 
 #ifdef CONFIG_X86_64
 	/* Flush global tlb. This is precaution. */
 	flush_tlb_early();
 #endif
-	uci->cpu_sig.rev = val[1];
+	uci->cpu_sig.rev = rev;
 
 	if (early)
 		print_ucode(uci);
@@ -784,8 +772,8 @@ static int apply_microcode_intel(int cpu)
 	struct microcode_intel *mc;
 	struct ucode_cpu_info *uci;
 	struct cpuinfo_x86 *c;
-	unsigned int val[2];
 	static int prev_rev;
+	u32 rev;
 
 	/* We should bind the task to the CPU */
 	if (WARN_ON(raw_smp_processor_id() != cpu))
@@ -802,33 +790,28 @@ static int apply_microcode_intel(int cpu)
 
 	/* write microcode via MSR 0x79 */
 	wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
-	wrmsrl(MSR_IA32_UCODE_REV, 0);
-
-	/* As documented in the SDM: Do a CPUID 1 here */
-	native_cpuid_eax(1);
 
-	/* get the current revision from MSR 0x8B */
-	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
+	rev = intel_get_microcode_revision();
 
-	if (val[1] != mc->hdr.rev) {
+	if (rev != mc->hdr.rev) {
 		pr_err("CPU%d update to revision 0x%x failed\n",
 		       cpu, mc->hdr.rev);
 		return -1;
 	}
 
-	if (val[1] != prev_rev) {
+	if (rev != prev_rev) {
 		pr_info("updated to revision 0x%x, date = %04x-%02x-%02x\n",
-			val[1],
+			rev,
 			mc->hdr.date & 0xffff,
 			mc->hdr.date >> 24,
 			(mc->hdr.date >> 16) & 0xff);
-		prev_rev = val[1];
+		prev_rev = rev;
 	}
 
 	c = &cpu_data(cpu);
 
-	uci->cpu_sig.rev = val[1];
-	c->microcode = val[1];
+	uci->cpu_sig.rev = rev;
+	c->microcode = rev;
 
 	return 0;
 }
-- 
2.8.4

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 1/2] x86/CPU: Add native CPUID variants returning a single datum

[Andy Lutomirski]

On Wed, Dec 28, 2016 at 3:20 AM, Borislav Petkov <bp@alien8.de> wrote:
> From: Borislav Petkov <bp@suse.de>
>
> ... similar to the cpuid_<reg>() variants.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/include/asm/processor.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index eaf100508c36..27ae83fc37de 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -219,6 +219,24 @@ static inline void native_cpuid(unsigned int *eax, unsigned int *ebx,
>             : "memory");
>  }
>
> +#define native_cpuid_reg(reg)                                  \
> +static inline unsigned int native_cpuid_##reg(unsigned int op) \
> +{                                                              \
> +       unsigned int eax = (op), ebx, ecx = 0, edx;             \

The parens around (op) shouldn't be needed.

> +                                                               \
> +       native_cpuid(&eax, &ebx, &ecx, &edx);                   \
> +                                                               \
> +       return reg;                                             \
> +}
> +
> +/*
> + * Native CPUID functions returning a single datum.
> + */
> +native_cpuid_reg(eax)
> +native_cpuid_reg(ebx)
> +native_cpuid_reg(ecx)
> +native_cpuid_reg(edx)
> +

On a very quick read, it looks like none of your new call sites
actually use the return value at all.  Since you also appear to be
consolidating them all, would it make sense to just open-code the
single (?) remaining user?

--Andy

Re: [PATCH 3/2] x86/microcode/intel: Add a helper which gives the microcode revision

[Andy Lutomirski]

On Wed, Dec 28, 2016 at 4:53 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Dec 28, 2016 at 12:21:20PM +0100, Borislav Petkov wrote:
>> From: Borislav Petkov <bp@suse.de>
>>
>> Intel supplies the microcode revision value in MSR 0x8b
>> (IA32_BIOS_SIGN_ID) after CPUID(1) has been executed. Execute it each
>> time before reading that MSR.
>
> And then, we can go a step further and even do a separate helper which does the
> required steps to read out the microcode revision so that we don't forget them
> next time we change the code.
>
> Provided that works for xen, though, because I need to do the native
> variants but early_init_intel() can call the paravirt *msr() versions
> and I have no idea whether that's kosher on xen pv.
>
> Boris, any objections?
>
> ---
> From: Borislav Petkov <bp@suse.de>
> Date: Wed, 28 Dec 2016 13:44:56 +0100
> Subject: [PATCH] x86/microcode/intel: Add a helper which gives the microcode
>  revision
>
> Since on Intel we're required to do CPUID(1) first, before reading
> the microcode revision MSR, let's add a special helper which does the
> required steps so that we don't forget to do them next time, when we
> want to read the microcode revision.
>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/include/asm/microcode_intel.h | 15 ++++++++++++
>  arch/x86/kernel/cpu/intel.c            | 11 +++------
>  arch/x86/kernel/cpu/microcode/intel.c  | 43 ++++++++++------------------------
>  3 files changed, 31 insertions(+), 38 deletions(-)
>
> diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
> index 195becc6f780..e793fc9a9b20 100644
> --- a/arch/x86/include/asm/microcode_intel.h
> +++ b/arch/x86/include/asm/microcode_intel.h
> @@ -52,6 +52,21 @@ struct extended_sigtable {
>
>  #define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
>
> +static inline u32 intel_get_microcode_revision(void)
> +{
> +       u32 rev, dummy;
> +
> +       native_wrmsrl(MSR_IA32_UCODE_REV, 0);
> +
> +       /* As documented in the SDM: Do a CPUID 1 here */
> +       native_cpuid_eax(1);

As in the other email, could this just be native_cpuid()?

--Andy

Re: [PATCH 3/2] x86/microcode/intel: Add a helper which gives the microcode revision

[Boris Ostrovsky]

On 12/28/2016 07:53 AM, Borislav Petkov wrote:
> On Wed, Dec 28, 2016 at 12:21:20PM +0100, Borislav Petkov wrote:
>> From: Borislav Petkov <bp@suse.de>
>>
>> Intel supplies the microcode revision value in MSR 0x8b
>> (IA32_BIOS_SIGN_ID) after CPUID(1) has been executed. Execute it each
>> time before reading that MSR.
>
> And then, we can go a step further and even do a separate helper which does the
> required steps to read out the microcode revision so that we don't forget them
> next time we change the code.
>
> Provided that works for xen, though, because I need to do the native
> variants but early_init_intel() can call the paravirt *msr() versions
> and I have no idea whether that's kosher on xen pv.
>
> Boris, any objections?

I think this should work. MSR_IA32_UCODE_REV can be natively read and 
written with zero without any side effects.

-boris

Re: [PATCH 1/2] x86/CPU: Add native CPUID variants returning a single datum

[Borislav Petkov]

On Wed, Dec 28, 2016 at 10:11:22AM -0800, Andy Lutomirski wrote:
> On a very quick read, it looks like none of your new call sites
> actually use the return value at all.  Since you also appear to be
> consolidating them all, would it make sense to just open-code the
> single (?) remaining user?

I've got stuff coming up which will use the retval but it is not fully
cooked yet. And also, we want to have generic helpers so that people do
not reimplement them left and right.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 3/2] x86/microcode/intel: Add a helper which gives the microcode revision

[Borislav Petkov]

On Wed, Dec 28, 2016 at 10:12:22AM -0800, Andy Lutomirski wrote:
> As in the other email, could this just be native_cpuid()?

Right, so we only have use for the native_cpuid_eax() variant right now
but having them all is nicely consistent. They come for almost for free
too.

Also, they're cpuid_<reg>() counterparts and if we don't add the
native_cpuid_e[bcd]x() now I can already see the question: "But but, why
didn't you implement the rest of the CPUID regs?"

Considering how cheap they are, I'd say we keep them all 4.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 3/2] x86/microcode/intel: Add a helper which gives the microcode revision

[Borislav Petkov]

On Wed, Dec 28, 2016 at 02:26:24PM -0500, Boris Ostrovsky wrote:
> I think this should work. MSR_IA32_UCODE_REV can be natively read and
> written with zero without any side effects.

Cool, thanks.

I'll run the pile through the boxes next year to make sure all is fine.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 1/2] x86/CPU: Add native CPUID variants returning a single datum

[Andy Lutomirski]

On Thu, Dec 29, 2016 at 1:30 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Dec 28, 2016 at 10:11:22AM -0800, Andy Lutomirski wrote:
>> On a very quick read, it looks like none of your new call sites
>> actually use the return value at all.  Since you also appear to be
>> consolidating them all, would it make sense to just open-code the
>> single (?) remaining user?
>
> I've got stuff coming up which will use the retval but it is not fully
> cooked yet. And also, we want to have generic helpers so that people do
> not reimplement them left and right.

Okay, but I still think that a variant that says "do cpuid and ignore
the return value" would make sense.  Imagine a very clever
implementation of native_cpuid_eax like:

asm ("cpuid" : "=a" (eax) : ...);
return eax;

Now you call it and ignore the return value and the compiler optimizes
it out :)  Also, someone reading the code might scratch their head and
wonder why you picked eax and not ebx, ecx, or edx.

--Andy

Re: [PATCH 1/2] x86/CPU: Add native CPUID variants returning a single datum

[Borislav Petkov]

On Fri, Dec 30, 2016 at 06:13:24PM -0800, Andy Lutomirski wrote:
> Now you call it and ignore the return value and the compiler optimizes
> it out :)

Does it, really?

It is an inlined asm volatile. I checked all call sites and the CPUID
call is there. gcc 6 simply issues the CPUID and then later code
overwrites rAX. I.e., it looks ok to me.

Or what example scenario do you have in mind?

> Also, someone reading the code might scratch their head and
> wonder why you picked eax and not ebx, ecx, or edx.

We have comments for her/him :-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH 1/2] x86/CPU: Add native CPUID variants returning a single datum

[Andy Lutomirski]

On Sat, Dec 31, 2016 at 3:09 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Dec 30, 2016 at 06:13:24PM -0800, Andy Lutomirski wrote:
>> Now you call it and ignore the return value and the compiler optimizes
>> it out :)
>
> Does it, really?
>
> It is an inlined asm volatile. I checked all call sites and the CPUID
> call is there. gcc 6 simply issues the CPUID and then later code
> overwrites rAX. I.e., it looks ok to me.
>
> Or what example scenario do you have in mind?

That's why I didn't type "volatile". :)

>
>> Also, someone reading the code might scratch their head and
>> wonder why you picked eax and not ebx, ecx, or edx.
>
> We have comments for her/him :-)

Okay.  Anyway, this particular nit is minor and I'll shut up.

>
> --
> Regards/Gruss,
>     Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 1/2] x86/CPU: Add native CPUID variants returning a single datum

[Borislav Petkov]

On Tue, Jan 03, 2017 at 10:35:15AM -0800, Andy Lutomirski wrote:
> That's why I didn't type "volatile". :)

I think you lost me: the version I'm proposing is with a retval you can
choose to ignore or not and it works either way due to the volatile...

Hmmm?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86: Fix Intel microcode revision detection

[Borislav Petkov]

On Thu, Jan 05, 2017 at 01:01:15AM +0000, Junichi Nomura wrote:
> I tested your series of patches and they work fine, too.
> Thank you!

Thanks, adding your Tested-by.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.