Re: Builtin microcode does nothing..

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Borislav Petkov-5
On Fri, May 20, 2016 at 02:34:34AM +0200, Gabriel C wrote:
> With this kernel the early microcode loading does nothing nor the 'old
> interface'.

Does it work when you disable CONFIG_BLK_DEV_INITRD in there?

I have a patchset which I'm testing right now which should correct some
warts in builtin microcode, if you want me to, I can push it out for you
to test too.

Thanks.

--
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Gabriel C-3
> Does it work when you disable CONFIG_BLK_DEV_INITRD in there?

I can test this whne I'm home since I need to change the config a bit.

> I have a patchset which I'm testing right now which should correct some
> warts in builtin microcode, if you want me to, I can push it out for you
> to test too.

Sure just give me an link or tell me where I can find your testing
patchset and I give it a try.


( I think you forgot to CC me )


Regards,

Gabriel C
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Borislav Petkov-5
On Fri, May 20, 2016 at 12:08:16PM +0200, Gabriel C wrote:
> ( I think you forgot to CC me )

You're in To:

From: Borislav Petkov <[hidden email]>
To: Gabriel C <[hidden email]>
Cc: LKML <[hidden email]>, Ingo Molnar <[hidden email]>,
        Thomas Gleixner <[hidden email]>
Subject: Re: Builtin microcode does nothing..
Message-ID: <[hidden email]>

--
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Gabriel C-3
In reply to this post by Gabriel C-3
2016-05-20 12:08 GMT+02:00 Gabriel C <[hidden email]>:

>> Does it work when you disable CONFIG_BLK_DEV_INITRD in there?
>
> I can test this whne I'm home since I need to change the config a bit.
>
>> I have a patchset which I'm testing right now which should correct some
>> warts in builtin microcode, if you want me to, I can push it out for you
>> to test too.
>
> Sure just give me an link or tell me where I can find your testing
> patchset and I give it a try.
>
>

Some update on that.

( I still did not test with CONFIG_BLK_DEV_INITRD diabled )

I don't have myself an AMD system but someone tested the same config
but with AMD firmware and this works as expected.

...

[    0.545036] microcode: microcode updated early to new patch_level=0x010000c8

....

So it seems just the Intel part of the code does not work.


Regards,

Gabriel C
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Gabriel C-3
In reply to this post by Gabriel C-3
On 20.05.2016 12:08, Gabriel C wrote:

>> Does it work when you disable CONFIG_BLK_DEV_INITRD in there?
> I can test this when I'm home since I need to change the config a bit.

I got to test an kernel without CONFIG_BLK_DEV_INITRD and this way does
work.

Regards,

Gabriel C
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Gabriel C-3
On 21.05.2016 02:20, Gabriel C wrote:

> On 20.05.2016 12:08, Gabriel C wrote:
>
>>> Does it work when you disable CONFIG_BLK_DEV_INITRD in there?
>> I can test this when I'm home since I need to change the config a bit.
>
> I got to test an kernel without CONFIG_BLK_DEV_INITRD and this way
> does work.
>

While that worked I read the code not sure I get this right but:

scan_microcode() has :

...
       if (!size) {
         if (!load_builtin_intel_microcode(&cd))
             return UCODE_ERROR;
     } else {
         cd = find_cpio_data(p, (void *)start, size, &offset);
         if (!cd.data)
             return UCODE_ERROR;
     }

...

So when I get this correctly load_builtin_intel_microcode() only works
on !initrd ?

Should this check not be the other way around ?

something like :

       cd = find_cpio_data(p, (void *)start, size, &offset);
       if (!cd.data) {
            if (!load_builtin_intel_microcode(&cd))
                  return UCODE_ERROR;
       }


Regards,

Gabriel C
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Borislav Petkov-5
In reply to this post by Gabriel C-3
On Sat, May 21, 2016 at 02:20:56AM +0200, Gabriel C wrote:
> I got to test an kernel without CONFIG_BLK_DEV_INITRD and this way
> does work.

Good, this confirms what I've been debugging here too.

Thanks.

--
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Borislav Petkov-5
In reply to this post by Gabriel C-3
On Sat, May 21, 2016 at 04:59:15AM +0200, Gabriel C wrote:
> Should this check not be the other way around ?

Actually, I've changed it to this:

        /* try built-in microcode first */
        if (load_builtin_intel_microcode(&cd))
                /*
                 * clear start as we might've gotten an initrd too supplied by
                 * the boot loader, by mistake or simply forgotten there. That's
                 * fine, we ignore it since we've found builtin microcode.
                 */
                start = 0;
        else {
#ifdef CONFIG_BLK_DEV_INITRD
                static __initdata char ucode_name[] = "kernel/x86/microcode/GenuineIntel.bin";
                char *p = IS_ENABLED(CONFIG_X86_32) ? (char *)__pa_nodebug(ucode_name)
                                                    : ucode_name;

                cd = find_cpio_data(p, (void *)start, size, NULL);
                if (!cd.data)
#endif
                        return UCODE_ERROR;
        }

so we're trying the built-in microcode first.

Why, you ask?

Well, I'm following the intention here - the user has gone the length
and has explicitly configured builtin microcode into the kernel and
therefore we're looking for it first.

If we don't find it, we fall back to initrd. This way, even if you have
an initrd forgotten in the bootloader, we ignore it.

I'll ping you once I'm done testing here.

--
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Borislav Petkov-5
On Sat, May 21, 2016 at 09:51:18AM +0200, Borislav Petkov wrote:
> I'll ping you once I'm done testing here.

Ok, I've just uploaded a branch, it passes testing here.

http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git, branch tip-microcode

@Jim, I'd appreciate it if you ran it again, if you get a chance, to
confirm everything is still ok.

Thanks.

--
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Jim Bos
On 05/25/2016 11:31 AM, Borislav Petkov wrote:

> On Sat, May 21, 2016 at 09:51:18AM +0200, Borislav Petkov wrote:
>> I'll ping you once I'm done testing here.
>
> Ok, I've just uploaded a branch, it passes testing here.
>
> http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git, branch tip-microcode
>
> @Jim, I'd appreciate it if you ran it again, if you get a chance, to
> confirm everything is still ok.
>
> Thanks.
>

Works fine here:

- no initrd.gz (simply all needed drivers builtin the main kernel)
- however, CONFIG_BLK_DEV_INITRD=y to keep option for initrd image
- intel ucode built-in kernel image

It also survives suspend/resume S2RAM cycle.

Thanks,

_
Jim

Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Borislav Petkov-5
On Wed, May 25, 2016 at 03:48:51PM +0200, Jim Bos wrote:
> Works fine here:
>
> - no initrd.gz (simply all needed drivers builtin the main kernel)
> - however, CONFIG_BLK_DEV_INITRD=y to keep option for initrd image
> - intel ucode built-in kernel image
>
> It also survives suspend/resume S2RAM cycle.

Very nice, thanks for testing again!

--
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Gabriel C-3
In reply to this post by Borislav Petkov-5
On 25.05.2016 11:31, Borislav Petkov wrote:

> On Sat, May 21, 2016 at 09:51:18AM +0200, Borislav Petkov wrote:
>> I'll ping you once I'm done testing here.
> Ok, I've just uploaded a branch, it passes testing here.
>
> http://git.kernel.org/cgit/linux/kernel/git/bp/bp.git, branch tip-microcode

  I tested your branch on top 4.6 stable.

  initrd + buitl-in intel ucode won't boot here. It hangs on grub
'Loading initrd image'..

  I'll build an kernel with your patches on my other box later /
tomorrow just to be sure..

  I didn't tested without initrd since that worked anyway...

Regards,

Garbiel C
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Borislav Petkov-5
On Wed, May 25, 2016 at 11:29:03PM +0200, Gabriel C wrote:
>  initrd + buitl-in intel ucode won't boot here. It hangs on grub 'Loading
> initrd image'..

That's with my patches? Can I have the .config please so that I can
reproduce?

With initrd you mean, you supply an initrd with the micrcode with grub
*and* have the same microcode builtin into the kernel, correct?

Thanks.

--
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Gabriel C-3


On 25.05.2016 23:38, Borislav Petkov wrote:
> On Wed, May 25, 2016 at 11:29:03PM +0200, Gabriel C wrote:
>>   initrd + buitl-in intel ucode won't boot here. It hangs on grub 'Loading
>> initrd image'..
> That's with my patches? Can I have the .config please so that I can
> reproduce?
>
> With initrd you mean, you supply an initrd with the micrcode with grub
> *and* have the same microcode builtin into the kernel, correct?

  No is a initrd without microcode with grub just built-in one.

  config is same like in my first email.

  http://crazy.dnshome.de/~crazy/lkml/config/config

I build an kernel on my oder box in a bit and let you know .. this
server I tested on is sometimes strange.


Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Gabriel C-3
On 25.05.2016 23:50, Gabriel C wrote:

>
> I build an kernel on my oder box in a bit and let you know .. this
> server I tested on is sometimes strange
  Hangs on the second box too also..
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Borislav Petkov-5
On Thu, May 26, 2016 at 01:36:51AM +0200, Gabriel C wrote:
>  Hangs on the second box too also..

Ok, please try the diff below ontop to see if it fixes your issue.

Looks like I'd need to rewrite the figuring out where the microcode data
is. Currently, it is ugly and error prone.

Thanks.

---
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 01c2d14ec05f..4bee5bdbaf2c 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -143,8 +143,13 @@ static inline bool
 get_builtin_firmware(struct cpio_data *cd, const char *name) { return false; }
 #endif
 
+static bool initrd_valid;
+
 static inline unsigned long get_initrd_start(void)
 {
+ if (!initrd_valid)
+ return 0;
+
 #ifdef CONFIG_BLK_DEV_INITRD
  return initrd_start;
 #else
@@ -154,6 +159,9 @@ static inline unsigned long get_initrd_start(void)
 
 static inline unsigned long get_initrd_start_addr(void)
 {
+ if (!initrd_valid)
+ return 0;
+
 #ifdef CONFIG_BLK_DEV_INITRD
 #ifdef CONFIG_X86_32
  unsigned long *initrd_start_p = (unsigned long *)__pa_nodebug(&initrd_start);
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 43f7caff4749..7ed06c397e2b 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -55,6 +55,8 @@ static struct mc_saved_data {
  struct microcode_intel **mc_saved;
 } mc_saved_data;
 
+static bool initrd_valid;
+
 /* Go through saved patches and find the one suitable for the current CPU. */
 static enum ucode_state
 find_microcode_patch(struct microcode_intel **saved,
@@ -533,12 +535,10 @@ static bool __init load_builtin_intel_microcode(struct cpio_data *cp)
 
 static __init enum ucode_state
 scan_microcode(struct mc_saved_data *mcs, unsigned long *mc_ptrs,
-       unsigned long *start, unsigned long size,
+       unsigned long start, unsigned long size,
        struct ucode_cpu_info *uci)
 {
- struct cpio_data cd;
- cd.data = NULL;
- cd.size = 0;
+ struct cpio_data cd = { NULL, 0, "" };
 
  /* try built-in microcode first */
  if (load_builtin_intel_microcode(&cd))
@@ -547,21 +547,23 @@ scan_microcode(struct mc_saved_data *mcs, unsigned long *mc_ptrs,
  * the boot loader, by mistake or simply forgotten there. That's
  * fine, we ignore it since we've found builtin microcode.
  */
- *start = 0;
+ initrd_valid = false;
  else {
 #ifdef CONFIG_BLK_DEV_INITRD
  static __initdata char ucode_name[] = "kernel/x86/microcode/GenuineIntel.bin";
  char *p = IS_ENABLED(CONFIG_X86_32) ? (char *)__pa_nodebug(ucode_name)
     : ucode_name;
 
- cd = find_cpio_data(p, (void *)*start, size, NULL);
- if (!cd.data)
+ cd = find_cpio_data(p, (void *)start, size, NULL);
+ if (cd.data)
+ initrd_valid = true;
+ else
 #endif
  return UCODE_ERROR;
  }
 
- return get_matching_model_microcode(*start, cd.data, cd.size,
-    mcs, mc_ptrs, uci);
+ return get_matching_model_microcode(initrd_valid ? start : 0,
+    cd.data, cd.size, mcs, mc_ptrs, uci);
 }
 
 /*
@@ -703,20 +705,16 @@ static void __init
 _load_ucode_intel_bsp(struct mc_saved_data *mcs, unsigned long *mc_ptrs,
       unsigned long start, unsigned long size)
 {
- unsigned long _start = start;
  struct ucode_cpu_info uci;
  enum ucode_state ret;
 
  collect_cpu_info_early(&uci);
 
- ret = scan_microcode(mcs, mc_ptrs, &_start, size, &uci);
+ ret = scan_microcode(mcs, mc_ptrs, start, size, &uci);
  if (ret != UCODE_OK)
  return;
 
- /* Pass updated starting address of blobs to the next routine.  */
- start = _start;
-
- ret = load_microcode(mcs, mc_ptrs, start, &uci);
+ ret = load_microcode(mcs, mc_ptrs, initrd_valid ? start : 0, &uci);
  if (ret != UCODE_OK)
  return;
 


--
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Gabriel C-3

On 26.05.2016 12:03, Borislav Petkov wrote:
> On Thu, May 26, 2016 at 01:36:51AM +0200, Gabriel C wrote:
>>   Hangs on the second box too also..
> Ok, please try the diff below ontop to see if it fixes your issue.
>
> Looks like I'd need to rewrite the figuring out where the microcode data
> is. Currently, it is ugly and error prone.

  With this patch ontop your your tip brach all is fine.
  Tested on both server and both are up and running.


>
> ---
> diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
> index 01c2d14ec05f..4bee5bdbaf2c 100644
> --- a/arch/x86/include/asm/microcode.h
> +++ b/arch/x86/include/asm/microcode.h
> @@ -143,8 +143,13 @@ static inline bool
>   get_builtin_firmware(struct cpio_data *cd, const char *name) { return false; }
>   #endif
>  
> +static bool initrd_valid;
> +
>   static inline unsigned long get_initrd_start(void)
>   {
> + if (!initrd_valid)
> + return 0;
> +
>   #ifdef CONFIG_BLK_DEV_INITRD
>   return initrd_start;
>   #else
> @@ -154,6 +159,9 @@ static inline unsigned long get_initrd_start(void)
>  
>   static inline unsigned long get_initrd_start_addr(void)
>   {
> + if (!initrd_valid)
> + return 0;
> +
>   #ifdef CONFIG_BLK_DEV_INITRD
>   #ifdef CONFIG_X86_32
>   unsigned long *initrd_start_p = (unsigned long *)__pa_nodebug(&initrd_start);
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 43f7caff4749..7ed06c397e2b 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -55,6 +55,8 @@ static struct mc_saved_data {
>   struct microcode_intel **mc_saved;
>   } mc_saved_data;
>  
> +static bool initrd_valid;
> +
>   /* Go through saved patches and find the one suitable for the current CPU. */
>   static enum ucode_state
>   find_microcode_patch(struct microcode_intel **saved,
> @@ -533,12 +535,10 @@ static bool __init load_builtin_intel_microcode(struct cpio_data *cp)
>  
>   static __init enum ucode_state
>   scan_microcode(struct mc_saved_data *mcs, unsigned long *mc_ptrs,
> -       unsigned long *start, unsigned long size,
> +       unsigned long start, unsigned long size,
>         struct ucode_cpu_info *uci)
>   {
> - struct cpio_data cd;
> - cd.data = NULL;
> - cd.size = 0;
> + struct cpio_data cd = { NULL, 0, "" };
>  
>   /* try built-in microcode first */
>   if (load_builtin_intel_microcode(&cd))
> @@ -547,21 +547,23 @@ scan_microcode(struct mc_saved_data *mcs, unsigned long *mc_ptrs,
>   * the boot loader, by mistake or simply forgotten there. That's
>   * fine, we ignore it since we've found builtin microcode.
>   */
> - *start = 0;
> + initrd_valid = false;
>   else {
>   #ifdef CONFIG_BLK_DEV_INITRD
>   static __initdata char ucode_name[] = "kernel/x86/microcode/GenuineIntel.bin";
>   char *p = IS_ENABLED(CONFIG_X86_32) ? (char *)__pa_nodebug(ucode_name)
>      : ucode_name;
>  
> - cd = find_cpio_data(p, (void *)*start, size, NULL);
> - if (!cd.data)
> + cd = find_cpio_data(p, (void *)start, size, NULL);
> + if (cd.data)
> + initrd_valid = true;
> + else
>   #endif
>   return UCODE_ERROR;
>   }
>  
> - return get_matching_model_microcode(*start, cd.data, cd.size,
> -    mcs, mc_ptrs, uci);
> + return get_matching_model_microcode(initrd_valid ? start : 0,
> +    cd.data, cd.size, mcs, mc_ptrs, uci);
>   }
>  
>   /*
> @@ -703,20 +705,16 @@ static void __init
>   _load_ucode_intel_bsp(struct mc_saved_data *mcs, unsigned long *mc_ptrs,
>        unsigned long start, unsigned long size)
>   {
> - unsigned long _start = start;
>   struct ucode_cpu_info uci;
>   enum ucode_state ret;
>  
>   collect_cpu_info_early(&uci);
>  
> - ret = scan_microcode(mcs, mc_ptrs, &_start, size, &uci);
> + ret = scan_microcode(mcs, mc_ptrs, start, size, &uci);
>   if (ret != UCODE_OK)
>   return;
>  
> - /* Pass updated starting address of blobs to the next routine.  */
> - start = _start;
> -
> - ret = load_microcode(mcs, mc_ptrs, start, &uci);
> + ret = load_microcode(mcs, mc_ptrs, initrd_valid ? start : 0, &uci);
>   if (ret != UCODE_OK)
>   return;
>  
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Builtin microcode does nothing..

Borislav Petkov-5
On Thu, May 26, 2016 at 01:52:22PM +0200, Gabriel C wrote:
>  With this patch ontop your your tip brach all is fine.
>  Tested on both server and both are up and running.

Thanks.

In the meantime, I've simplified the code even more and testing here
with your .config looks good. I'll prepare another patchset next week
and post it here in case you guys feel bored and want to test it too.

:-)

Thanks again!

--
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--