Re: [PATCH] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

Re: [PATCH] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Borislav Petkov]

On Mon, Apr 20, 2020 at 03:00:37PM +0300, Evalds Iodzevics wrote:
> sync_core() always jums past cpuid instruction on 32 bit machines
> because data structure boot_cpu_data are not populated so early in boot.

I'm guessing because boot_cpu_data.cpuid_level is not properly set and
very early code in head_32.S sets it to -1 temporarily until the highest
CPUID level has been detected (or not).

But the microcode loading happens *before* that.

I'd probably be interested how you trigger that but since the backport
got changed (see below) and yours is correcting it to the upstream
variant then perhaps maybe I don't care that much. :)

> It depends on commit 5dedade6dfa243c130b85d1e4daba6f027805033 for
> native_cpuid_reg(eax) definitions
> 
> This patch is for 4.4 but also should apply to 4.9
> 
> Signed-off-by: Evalds Iodzevics <evalds.iodzevics@gmail.com>
> ---
>  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);
> -- 

Hrm, the original patch of mine did use native_cpuid_eax():

4167709bbf82 ("x86/microcode/intel: Add a helper which gives the microcode revision")

but the backport:

commit 98cc1464cfd6edf9dc7fa96aaaf596aae952029b
Author: Borislav Petkov <bp@suse.de>
Date:   Mon Jan 9 12:41:45 2017 +0100

    x86/microcode/intel: Add a helper which gives the microcode revision
    
    commit 4167709bbf826512a52ebd6aafda2be104adaec9 upstream.
    
    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>
    Link: http://lkml.kernel.org/r/20170109114147.5082-4-bp@alien8.de
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    [bwh: Backported to 4.4:
     - Don't touch prev_rev variable in apply_microcode()
     - Keep using sync_core(), which will alway includes the necessary CPUID
     	^^^^^^^^^^^^^^^^^^^

decided to use sync_core() for whatever reason. Perhaps because the
native_cpuid* things weren't there. Adding Ben to Cc.

I believe this is the background info Greg needed to figure out *why*
you're doing this.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Greg KH]

On Mon, Apr 20, 2020 at 03:00:37PM +0300, Evalds Iodzevics wrote:
> sync_core() always jums past cpuid instruction on 32 bit machines
> because data structure boot_cpu_data are not populated so early in boot.
> 
> It depends on commit 5dedade6dfa243c130b85d1e4daba6f027805033 for
> native_cpuid_reg(eax) definitions
> 
> This patch is for 4.4 but also should apply to 4.9
> 
> Signed-off-by: Evalds Iodzevics <evalds.iodzevics@gmail.com>
> ---
>  arch/x86/include/asm/microcode_intel.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)


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

[PATCH] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Evalds Iodzevics]

sync_core() always jums past cpuid instruction on 32 bit machines
because data structure boot_cpu_data are not populated so early in boot.

It depends on commit 5dedade6dfa243c130b85d1e4daba6f027805033 for
native_cpuid_reg(eax) definitions

This patch is for 4.4 but also should apply to 4.9

Signed-off-by: Evalds Iodzevics <evalds.iodzevics@gmail.com>
---
 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] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2496 bytes --]

On Mon, 2020-04-20 at 11:31 +0200, Borislav Petkov wrote:
> On Mon, Apr 20, 2020 at 03:00:37PM +0300, Evalds Iodzevics wrote:
> > sync_core() always jums past cpuid instruction on 32 bit machines
> > because data structure boot_cpu_data are not populated so early in boot.
> 
> I'm guessing because boot_cpu_data.cpuid_level is not properly set and
> very early code in head_32.S sets it to -1 temporarily until the highest
> CPUID level has been detected (or not).
> 
> But the microcode loading happens *before* that.
[...]
> Hrm, the original patch of mine did use native_cpuid_eax():
> 
> 4167709bbf82 ("x86/microcode/intel: Add a helper which gives the microcode revision")
> 
> but the backport:
> 
> commit 98cc1464cfd6edf9dc7fa96aaaf596aae952029b
> Author: Borislav Petkov <bp@suse.de>
> Date:   Mon Jan 9 12:41:45 2017 +0100
> 
>     x86/microcode/intel: Add a helper which gives the microcode revision
>     
>     commit 4167709bbf826512a52ebd6aafda2be104adaec9 upstream.
>     
>     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>
>     Link: http://lkml.kernel.org/r/20170109114147.5082-4-bp@alien8.de
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>     [bwh: Backported to 4.4:
>      - Don't touch prev_rev variable in apply_microcode()
>      - Keep using sync_core(), which will alway includes the necessary CPUID
>      	^^^^^^^^^^^^^^^^^^^
> 
> decided to use sync_core() for whatever reason. Perhaps because the
> native_cpuid* things weren't there. Adding Ben to Cc.

This commit didn't introduce the call to native_cpuid_eax(), but only
moved it.  So I didn't think it made sense for the backport to change
from sync_core().

If it's important to use native_cpuid_eax() then these older branches
should presumably get backports of:

484d0e5c7943 x86/microcode/intel: Replace sync_core() with native_cpuid()
f3e2a51f568d x86/microcode: Use native CPUID to tickle out microcode revision

Ben.

> I believe this is the background info Greg needed to figure out *why*
> you're doing this.
> 
-- 
Ben Hutchings
Klipstein's 4th Law of Prototyping and Production:
                               A fail-safe circuit will destroy others.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Evalds Iodzevics]

On Mon, Apr 20, 2020 at 4:33 PM Ben Hutchings <ben@decadent.org.uk> wrote:
>
> On Mon, 2020-04-20 at 11:31 +0200, Borislav Petkov wrote:
> > On Mon, Apr 20, 2020 at 03:00:37PM +0300, Evalds Iodzevics wrote:
> > > sync_core() always jums past cpuid instruction on 32 bit machines
> > > because data structure boot_cpu_data are not populated so early in boot.
> >
> > I'm guessing because boot_cpu_data.cpuid_level is not properly set and
> > very early code in head_32.S sets it to -1 temporarily until the highest
> > CPUID level has been detected (or not).
> >
> > But the microcode loading happens *before* that.
> [...]
> > Hrm, the original patch of mine did use native_cpuid_eax():
> >
> > 4167709bbf82 ("x86/microcode/intel: Add a helper which gives the microcode revision")
> >
> > but the backport:
> >
> > commit 98cc1464cfd6edf9dc7fa96aaaf596aae952029b
> > Author: Borislav Petkov <bp@suse.de>
> > Date:   Mon Jan 9 12:41:45 2017 +0100
> >
> >     x86/microcode/intel: Add a helper which gives the microcode revision
> >
> >     commit 4167709bbf826512a52ebd6aafda2be104adaec9 upstream.
> >
> >     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>
> >     Link: http://lkml.kernel.org/r/20170109114147.5082-4-bp@alien8.de
> >     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >     [bwh: Backported to 4.4:
> >      - Don't touch prev_rev variable in apply_microcode()
> >      - Keep using sync_core(), which will alway includes the necessary CPUID
> >       ^^^^^^^^^^^^^^^^^^^
> >
> > decided to use sync_core() for whatever reason. Perhaps because the
> > native_cpuid* things weren't there. Adding Ben to Cc.
>
> This commit didn't introduce the call to native_cpuid_eax(), but only
> moved it.  So I didn't think it made sense for the backport to change
> from sync_core().
>
> If it's important to use native_cpuid_eax() then these older branches
> should presumably get backports of:
>
> 484d0e5c7943 x86/microcode/intel: Replace sync_core() with native_cpuid()
> f3e2a51f568d x86/microcode: Use native CPUID to tickle out microcode revision
>
> Ben.
>
> > I believe this is the background info Greg needed to figure out *why*
> > you're doing this.
> >
> --
> Ben Hutchings
> Klipstein's 4th Law of Prototyping and Production:
>                                A fail-safe circuit will destroy others.
>
As far as i can tell, these two:

484d0e5c7943 x86/microcode/intel: Replace sync_core() with native_cpuid()
f3e2a51f568d x86/microcode: Use native CPUID to tickle out microcode revision

are already made obsolete by:

98cc1464cfd6  x86/microcode/intel: Add a helper which gives the
microcode revision

except that it still relied on sync_core() witch did not behave
correctly on 32 bit machines

Re: [PATCH v2] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Greg KH]

On Tue, Apr 21, 2020 at 11:53:44AM +0300, Evalds Iodzevics wrote:
> On Intel it is required to do CPUID(1) before reading the microcode
> revision MSR. Current code in 4.4 an 4.9 relies on sync_core() to call
> CPUID, unfortunately on 32 bit machines code inside sync_core() always
> jumps past CPUID instruction as it depends on data structure boot_cpu_data
> witch are not populated correctly so early in boot sequence.
> 
> It depends on:
> commit 5dedade6dfa2 ("x86/CPU: Add native CPUID variants returning a single
> datum")
> 
> This patch is for 4.4 but also should apply to 4.9
> 
> Signed-off-by: Evalds Iodzevics <evalds.iodzevics@gmail.com>
> ---
>  arch/x86/include/asm/microcode_intel.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Why are you not sending this to the stable mailing list like I have
pointed out numerous times by sending you a link to _how_ to get a patch
into the stable kernel trees?

Again, here it is:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

Please follow that so that we can do this correctly.

thanks,

greg k-h

Re: [PATCH v2] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Evalds Iodzevics]

On Tue, Apr 21, 2020 at 8:59 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 21, 2020 at 11:53:44AM +0300, Evalds Iodzevics wrote:
> > On Intel it is required to do CPUID(1) before reading the microcode
> > revision MSR. Current code in 4.4 an 4.9 relies on sync_core() to call
> > CPUID, unfortunately on 32 bit machines code inside sync_core() always
> > jumps past CPUID instruction as it depends on data structure boot_cpu_data
> > witch are not populated correctly so early in boot sequence.
> >
> > It depends on:
> > commit 5dedade6dfa2 ("x86/CPU: Add native CPUID variants returning a single
> > datum")
> >
> > This patch is for 4.4 but also should apply to 4.9
> >
> > Signed-off-by: Evalds Iodzevics <evalds.iodzevics@gmail.com>
> > ---
> >  arch/x86/include/asm/microcode_intel.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Why are you not sending this to the stable mailing list like I have
> pointed out numerous times by sending you a link to _how_ to get a patch
> into the stable kernel trees?
>
> Again, here it is:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>
> Please follow that so that we can do this correctly.
>
> thanks,
>
> greg k-h
Sorry, I might sound dumb here but should i just send it to
stable@vger.kernel.org or try to tag it Cc: stable... in sign-off
area, its quite confusing for newcomer.
Thanks for patience!

Re: [PATCH v2] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Greg KH]

On Tue, Apr 21, 2020 at 09:41:42AM +0300, Evalds Iodzevics wrote:
> On Tue, Apr 21, 2020 at 8:59 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 21, 2020 at 11:53:44AM +0300, Evalds Iodzevics wrote:
> > > On Intel it is required to do CPUID(1) before reading the microcode
> > > revision MSR. Current code in 4.4 an 4.9 relies on sync_core() to call
> > > CPUID, unfortunately on 32 bit machines code inside sync_core() always
> > > jumps past CPUID instruction as it depends on data structure boot_cpu_data
> > > witch are not populated correctly so early in boot sequence.
> > >
> > > It depends on:
> > > commit 5dedade6dfa2 ("x86/CPU: Add native CPUID variants returning a single
> > > datum")
> > >
> > > This patch is for 4.4 but also should apply to 4.9
> > >
> > > Signed-off-by: Evalds Iodzevics <evalds.iodzevics@gmail.com>
> > > ---
> > >  arch/x86/include/asm/microcode_intel.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Why are you not sending this to the stable mailing list like I have
> > pointed out numerous times by sending you a link to _how_ to get a patch
> > into the stable kernel trees?
> >
> > Again, here it is:
> >     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> >
> > Please follow that so that we can do this correctly.
> >
> > thanks,
> >
> > greg k-h
> Sorry, I might sound dumb here but should i just send it to
> stable@vger.kernel.org or try to tag it Cc: stable... in sign-off
> area, its quite confusing for newcomer.

Yes, be sure to cc: the stable@vger.kernel.org list when you send
patches that you want to have applied to a stable kernel tree.

thanks,

greg k-h

[PATCH v2] x86/microcode/intel: replace sync_core() with native_cpuid_reg(eax)

[Evalds Iodzevics]

On Intel it is required to do CPUID(1) before reading the microcode
revision MSR. Current code in 4.4 an 4.9 relies on sync_core() to call
CPUID, unfortunately on 32 bit machines code inside sync_core() always
jumps past CPUID instruction as it depends on data structure boot_cpu_data
witch are not populated correctly so early in boot sequence.

It depends on:
commit 5dedade6dfa2 ("x86/CPU: Add native CPUID variants returning a single
datum")

This patch is for 4.4 but also should apply to 4.9

Signed-off-by: Evalds Iodzevics <evalds.iodzevics@gmail.com>
---
 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