Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1404302 > unrolled thread

Re: Builtin microcode does nothing..

Back to article view | Back to linux.kernel

Contents

  Re: Builtin microcode does nothing.. Gabriel C <nix.or.die@gmail.com> - 2016-05-20 12:10 +0200
    Re: Builtin microcode does nothing.. Borislav Petkov <bp@suse.de> - 2016-05-20 12:30 +0200
    Re: Builtin microcode does nothing.. Gabriel C <nix.or.die@gmail.com> - 2016-05-20 18:10 +0200
    Re: Builtin microcode does nothing.. Gabriel C <nix.or.die@gmail.com> - 2016-05-21 02:30 +0200
      Re: Builtin microcode does nothing.. Gabriel C <nix.or.die@gmail.com> - 2016-05-21 05:00 +0200
        Re: Builtin microcode does nothing.. Borislav Petkov <bp@suse.de> - 2016-05-21 10:00 +0200
          Re: Builtin microcode does nothing.. Borislav Petkov <bp@suse.de> - 2016-05-25 11:40 +0200
            Re: Builtin microcode does nothing.. Jim Bos <jim876@xs4all.nl> - 2016-05-25 15:50 +0200
              Re: Builtin microcode does nothing.. Borislav Petkov <bp@suse.de> - 2016-05-25 17:00 +0200
            Re: Builtin microcode does nothing.. Gabriel C <nix.or.die@gmail.com> - 2016-05-25 23:30 +0200
              Re: Builtin microcode does nothing.. Borislav Petkov <bp@suse.de> - 2016-05-25 23:40 +0200
                Re: Builtin microcode does nothing.. Gabriel C <nix.or.die@gmail.com> - 2016-05-26 00:00 +0200
                  Re: Builtin microcode does nothing.. Gabriel C <nix.or.die@gmail.com> - 2016-05-26 04:00 +0200
                    Re: Builtin microcode does nothing.. Borislav Petkov <bp@suse.de> - 2016-05-26 12:10 +0200
                      Re: Builtin microcode does nothing.. Gabriel C <nix.or.die@gmail.com> - 2016-05-26 14:00 +0200
                        Re: Builtin microcode does nothing.. Borislav Petkov <bp@suse.de> - 2016-05-26 14:50 +0200
      Re: Builtin microcode does nothing.. Borislav Petkov <bp@suse.de> - 2016-05-21 09:50 +0200

#1404302 — Re: Builtin microcode does nothing..

> 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

[toc] | [next] | [standalone]

#1404324

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 <bp@suse.de>
To: Gabriel C <nix.or.die@gmail.com>
Cc: LKML <linux-kernel@vger.kernel.org>, Ingo Molnar <mingo@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: Builtin microcode does nothing..
Message-ID: <20160520083634.GA594@pd.tnic>

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1404570

2016-05-20 12:08 GMT+02:00 Gabriel C <nix.or.die@gmail.com>:
>> 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

[toc] | [prev] | [next] | [standalone]

#1404778

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

[toc] | [prev] | [next] | [standalone]

#1404794

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

[toc] | [prev] | [next] | [standalone]

#1404825

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

[toc] | [prev] | [next] | [standalone]

#1406776

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

[toc] | [prev] | [next] | [standalone]

#1406921

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

[toc] | [prev] | [next] | [standalone]

#1406980

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

[toc] | [prev] | [next] | [standalone]

#1407218

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

[toc] | [prev] | [next] | [standalone]

#1407229

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

[toc] | [prev] | [next] | [standalone]

#1407252

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.

[toc] | [prev] | [next] | [standalone]

#1407344

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

[toc] | [prev] | [next] | [standalone]

#1407495

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

[toc] | [prev] | [next] | [standalone]

#1407533

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

[toc] | [prev] | [next] | [standalone]

#1407547

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

[toc] | [prev] | [next] | [standalone]

#1404823

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web