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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Thomas Gleixner <[email protected]>
[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
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 <[email protected]>
> ---
> 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>
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 <[email protected]>
> 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 <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: Thomas Gleixner <[email protected]>
> [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.
On Mon, Apr 20, 2020 at 4:33 PM Ben Hutchings <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
> > Link: http://lkml.kernel.org/r/[email protected]
> > Signed-off-by: Thomas Gleixner <[email protected]>
> > [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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
On Tue, Apr 21, 2020 at 8:59 AM Greg KH <[email protected]> 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 <[email protected]>
> > ---
> > 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
[email protected] or try to tag it Cc: stable... in sign-off
area, its quite confusing for newcomer.
Thanks for patience!
On Tue, Apr 21, 2020 at 09:41:42AM +0300, Evalds Iodzevics wrote:
> On Tue, Apr 21, 2020 at 8:59 AM Greg KH <[email protected]> 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 <[email protected]>
> > > ---
> > > 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
> [email protected] or try to tag it Cc: stable... in sign-off
> area, its quite confusing for newcomer.
Yes, be sure to cc: the [email protected] list when you send
patches that you want to have applied to a stable kernel tree.
thanks,
greg k-h