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

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

[Evalds Iodzevics]

From: Borislav Petkov <bp@suse.de>

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

Signed-off-by: Borislav Petkov <bp@suse.de>
Link: http://lkml.kernel.org/r/20170109114147.5082-2-bp@alien8.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.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 cac54e61c299..048942d53988 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -212,6 +212,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.17.4

[PATCH 2/2] Fixed broken microcode early loading on 32 bit platforms because it always jumps past cpuid in sync_core() as data structure boot_cpu_data are not populated so early in boot. This is for 4.4. Should be done for 4.9 too

[Evalds Iodzevics]

---
 arch/x86/include/asm/microcode_intel.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 90343ba50485..92ce9c8a508b 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -60,7 +60,7 @@ static inline u32 intel_get_microcode_revision(void)
 	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+	native_cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, dummy, rev);
-- 
2.17.4

Re: [PATCH 2/2] Fixed broken microcode early loading on 32 bit platforms because it always jumps past cpuid in sync_core() as data structure boot_cpu_data are not populated so early in boot. This is for 4.4. Should be done for 4.9 too

[Greg KH]

On Sun, Apr 19, 2020 at 07:29:43PM +0300, Evalds Iodzevics wrote:
> ---
>  arch/x86/include/asm/microcode_intel.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Your subject line is really confused :(

Re: [PATCH 2/2] Fixed broken microcode early loading on 32 bit platforms because it always jumps past cpuid in sync_core() as data structure boot_cpu_data are not populated so early in boot. This is for 4.4. Should be done for 4.9 too

[Evalds Iodzevics]

Yeah, basically its a fix for early microcode load on x86 for 4.4 longterm
Just tried to say same problem exists on 4.9 too

On Sun, Apr 19, 2020 at 7:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Apr 19, 2020 at 07:29:43PM +0300, Evalds Iodzevics wrote:
> > ---
> >  arch/x86/include/asm/microcode_intel.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Your subject line is really confused :(
>

Re: [PATCH 2/2] Fixed broken microcode early loading on 32 bit platforms because it always jumps past cpuid in sync_core() as data structure boot_cpu_data are not populated so early in boot. This is for 4.4. Should be done for 4.9 too

[Greg KH]

On Mon, Apr 20, 2020 at 08:52:07AM +0300, Evalds Iodzevics wrote:
> Yeah, basically its a fix for early microcode load on x86 for 4.4 longterm
> Just tried to say same problem exists on 4.9 too

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

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

[Greg KH]

On Sun, Apr 19, 2020 at 07:29:42PM +0300, Evalds Iodzevics wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> ... similarly to the cpuid_<reg>() variants.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Link: http://lkml.kernel.org/r/20170109114147.5082-2-bp@alien8.de
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/include/asm/processor.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Why are you resending patches from someone else?

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

[Evalds Iodzevics]

The second patch depends on this commit by Borislav so i included
this, probably I should have just mentioned it in my comment?

On Sun, Apr 19, 2020 at 7:54 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sun, Apr 19, 2020 at 07:29:42PM +0300, Evalds Iodzevics wrote:
> > From: Borislav Petkov <bp@suse.de>
> >
> > ... similarly to the cpuid_<reg>() variants.
> >
> > Signed-off-by: Borislav Petkov <bp@suse.de>
> > Link: http://lkml.kernel.org/r/20170109114147.5082-2-bp@alien8.de
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> >  arch/x86/include/asm/processor.h | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
>
> Why are you resending patches from someone else?

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

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Mon, Apr 20, 2020 at 08:52:53AM +0300, Evalds Iodzevics wrote:
> The second patch depends on this commit by Borislav so i included
> this, probably I should have just mentioned it in my comment?

I really do not understand this sentence at all.  I have no context as
to what you are trying to do.

greg k-h

[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.
--

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