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

Groups > linux.kernel > #1542861 > unrolled thread

Can't boot as Xen dom0 due to commit fe055896

Back to article view | Back to linux.kernel

Contents

  Can't boot as Xen dom0 due to commit fe055896 Juergen Gross <jgross@suse.com> - 2016-12-15 17:20 +0100
    Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-15 17:50 +0100
      Re: [Xen-devel] Can't boot as Xen dom0 due to commit fe055896 "Jan Beulich" <JBeulich@suse.com> - 2016-12-15 18:00 +0100
        Re: [Xen-devel] Can't boot as Xen dom0 due to commit fe055896 Andrew Cooper <andrew.cooper3@citrix.com> - 2016-12-15 18:10 +0100
          Re: [Xen-devel] Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-15 18:40 +0100
      Re: Can't boot as Xen dom0 due to commit fe055896 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-15 18:00 +0100
        Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-15 18:20 +0100
          Re: Can't boot as Xen dom0 due to commit fe055896 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-15 18:30 +0100
            Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-15 18:40 +0100
              Re: Can't boot as Xen dom0 due to commit fe055896 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-15 20:10 +0100
                Re: Can't boot as Xen dom0 due to commit fe055896 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-15 20:40 +0100
                  Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-15 21:10 +0100
                    Re: Can't boot as Xen dom0 due to commit fe055896 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-16 00:00 +0100
                      Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-16 00:10 +0100
                        Re: Can't boot as Xen dom0 due to commit fe055896 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-16 05:40 +0100
                          Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-16 10:10 +0100
                            Re: [Xen-devel] Can't boot as Xen dom0 due to commit fe055896 Andrew Cooper <andrew.cooper3@citrix.com> - 2016-12-16 10:40 +0100
                        Re: Can't boot as Xen dom0 due to commit fe055896 Henrique de Moraes Holschuh <hmh@hmh.eng.br> - 2016-12-16 05:40 +0100
                Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-15 20:50 +0100
              Re: Can't boot as Xen dom0 due to commit fe055896 Juergen Gross <jgross@suse.com> - 2016-12-16 08:30 +0100
                Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-16 10:10 +0100
                  Re: Can't boot as Xen dom0 due to commit fe055896 Juergen Gross <jgross@suse.com> - 2016-12-16 10:30 +0100
                    Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-16 10:50 +0100
                      Re: Can't boot as Xen dom0 due to commit fe055896 Juergen Gross <jgross@suse.com> - 2016-12-16 11:10 +0100
                        Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-16 12:10 +0100
                          Re: Can't boot as Xen dom0 due to commit fe055896 Juergen Gross <jgross@suse.com> - 2016-12-16 13:20 +0100
                            Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-16 13:50 +0100
                              Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-16 14:10 +0100
                                Re: Can't boot as Xen dom0 due to commit fe055896 Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-12-16 15:40 +0100
                                  Re: Can't boot as Xen dom0 due to commit fe055896 Borislav Petkov <bp@suse.de> - 2016-12-16 16:20 +0100
      Re: Can't boot as Xen dom0 due to commit fe055896 Juergen Gross <jgross@suse.com> - 2016-12-15 18:30 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1543284

On Fri, Dec 16, 2016 at 08:28:46AM +0100, Juergen Gross wrote:
> Not trying to load ucode in _any_ guest is an optimization only.

Does the hunk below work too?

I don't want to do hypervisor-specific solutions.

---
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 6996413c78c3..54219f619205 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -76,6 +76,7 @@ struct cpu_info_ctx {
 static bool __init check_loader_disabled_bsp(void)
 {
 	static const char *__dis_opt_str = "dis_ucode_ldr";
+	u32 a, b, c, d;
 
 #ifdef CONFIG_X86_32
 	const char *cmdline = (const char *)__pa_nodebug(boot_command_line);
@@ -91,6 +92,17 @@ static bool __init check_loader_disabled_bsp(void)
 	if (cmdline_find_option_bool(cmdline, option))
 		*res = true;
 
+	if (!have_cpuid_p())
+		*res = true;
+
+	a = 1;
+	c = 0;
+	native_cpuid(&a, &b, &c, &d);
+
+	/* CPUID(1).ECX[31]: reserved for hypervisor use */
+	if (c & BIT(31))
+		*res = true;
+
 	return *res;
 }
 
@@ -121,9 +133,6 @@ void __init load_ucode_bsp(void)
 	if (check_loader_disabled_bsp())
 		return;
 
-	if (!have_cpuid_p())
-		return;
-
 	vendor = x86_cpuid_vendor();
 	family = x86_cpuid_family();
 
@@ -157,9 +166,6 @@ void load_ucode_ap(void)
 	if (check_loader_disabled_ap())
 		return;
 
-	if (!have_cpuid_p())
-		return;
-
 	vendor = x86_cpuid_vendor();
 	family = x86_cpuid_family();
 

-- 
Regards/Gruss,
    Boris.

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

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

#1543295

On 16/12/16 10:02, Borislav Petkov wrote:
> On Fri, Dec 16, 2016 at 08:28:46AM +0100, Juergen Gross wrote:
>> Not trying to load ucode in _any_ guest is an optimization only.
> 
> Does the hunk below work too?

Without testing, but I doubt it is working. As pv guests aren't coming
through check_loader_disabled_bsp() at all I can't see why your patch
would work for dom0. Additionally I don't think you want to call
native_cpuid() if have_cpuid_p() returns false.

So I think you want a generic "platform_allows_ucode_load()" function
checking for support of cpuid and virtualization. This function should
be called both in check_loader_disabled_bsp() and
check_loader_disabled_ap() to bail out early.


Juergen

> 
> I don't want to do hypervisor-specific solutions.
> 
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 6996413c78c3..54219f619205 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -76,6 +76,7 @@ struct cpu_info_ctx {
>  static bool __init check_loader_disabled_bsp(void)
>  {
>  	static const char *__dis_opt_str = "dis_ucode_ldr";
> +	u32 a, b, c, d;
>  
>  #ifdef CONFIG_X86_32
>  	const char *cmdline = (const char *)__pa_nodebug(boot_command_line);
> @@ -91,6 +92,17 @@ static bool __init check_loader_disabled_bsp(void)
>  	if (cmdline_find_option_bool(cmdline, option))
>  		*res = true;
>  
> +	if (!have_cpuid_p())
> +		*res = true;
> +
> +	a = 1;
> +	c = 0;
> +	native_cpuid(&a, &b, &c, &d);
> +
> +	/* CPUID(1).ECX[31]: reserved for hypervisor use */
> +	if (c & BIT(31))
> +		*res = true;
> +
>  	return *res;
>  }
>  
> @@ -121,9 +133,6 @@ void __init load_ucode_bsp(void)
>  	if (check_loader_disabled_bsp())
>  		return;
>  
> -	if (!have_cpuid_p())
> -		return;
> -
>  	vendor = x86_cpuid_vendor();
>  	family = x86_cpuid_family();
>  
> @@ -157,9 +166,6 @@ void load_ucode_ap(void)
>  	if (check_loader_disabled_ap())
>  		return;
>  
> -	if (!have_cpuid_p())
> -		return;
> -
>  	vendor = x86_cpuid_vendor();
>  	family = x86_cpuid_family();
>  
> 

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

#1543319

On Fri, Dec 16, 2016 at 10:20:42AM +0100, Juergen Gross wrote:
> Without testing, but I doubt it is working. As pv guests aren't coming
> through check_loader_disabled_bsp() at all I can't see why your patch
> would work for dom0.

Do they go through check_loader_disabled_ap() ?

> Additionally I don't think you want to call native_cpuid() if
> have_cpuid_p() returns false.

Good point, fixed.

> So I think you want a generic "platform_allows_ucode_load()"
> function checking for support of cpuid and virtualization. This
> function should be called both in check_loader_disabled_bsp() and
> check_loader_disabled_ap() to bail out early.

See question above. If they go through check_loader_disabled_ap(),
then I'm inclined to set dis_ucode_ldr to true at build time and let
check_loader_disabled_bsp() set it to false on baremetal or if any of
the other checks pass.

If the pv guests run into check_loader_disabled_ap, then they'll see
dis_ucode_ldr true and return.

Ok?

-- 
Regards/Gruss,
    Boris.

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

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

#1543331

On 16/12/16 10:43, Borislav Petkov wrote:
> On Fri, Dec 16, 2016 at 10:20:42AM +0100, Juergen Gross wrote:
>> Without testing, but I doubt it is working. As pv guests aren't coming
>> through check_loader_disabled_bsp() at all I can't see why your patch
>> would work for dom0.
> 
> Do they go through check_loader_disabled_ap() ?

Yes.

> 
>> Additionally I don't think you want to call native_cpuid() if
>> have_cpuid_p() returns false.
> 
> Good point, fixed.
> 
>> So I think you want a generic "platform_allows_ucode_load()"
>> function checking for support of cpuid and virtualization. This
>> function should be called both in check_loader_disabled_bsp() and
>> check_loader_disabled_ap() to bail out early.
> 
> See question above. If they go through check_loader_disabled_ap(),
> then I'm inclined to set dis_ucode_ldr to true at build time and let
> check_loader_disabled_bsp() set it to false on baremetal or if any of
> the other checks pass.
> 
> If the pv guests run into check_loader_disabled_ap, then they'll see
> dis_ucode_ldr true and return.
> 
> Ok?

Should work. I'm happy to test any patch. :-)


Juergen

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

#1543370

On Fri, Dec 16, 2016 at 11:00:29AM +0100, Juergen Gross wrote:
> Should work. I'm happy to test any patch. :-)

I'm happy that you're happy to! :-)

Let's try this below.

Thanks!

---
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 6996413c78c3..c4bb2f7169f6 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -44,7 +44,7 @@
 #define DRIVER_VERSION	"2.2"
 
 static struct microcode_ops	*microcode_ops;
-static bool dis_ucode_ldr;
+static bool dis_ucode_ldr = true;
 
 LIST_HEAD(microcode_cache);
 
@@ -76,6 +76,7 @@ struct cpu_info_ctx {
 static bool __init check_loader_disabled_bsp(void)
 {
 	static const char *__dis_opt_str = "dis_ucode_ldr";
+	u32 a, b, c, d;
 
 #ifdef CONFIG_X86_32
 	const char *cmdline = (const char *)__pa_nodebug(boot_command_line);
@@ -88,8 +89,23 @@ static bool __init check_loader_disabled_bsp(void)
 	bool *res = &dis_ucode_ldr;
 #endif
 
-	if (cmdline_find_option_bool(cmdline, option))
-		*res = true;
+	if (!have_cpuid_p())
+		return *res;
+
+	a = 1;
+	c = 0;
+	native_cpuid(&a, &b, &c, &d);
+
+	/*
+	 * CPUID(1).ECX[31]: reserved for hypervisor use. This is still not
+	 * completely accurate as xen pv guests don't see that CPUID bit set but
+	 * that's good enough as they don't land on the BSP path anyway.
+	 */
+	if (c & BIT(31))
+		return *res;
+
+	if (cmdline_find_option_bool(cmdline, option) <= 0)
+		*res = false;
 
 	return *res;
 }
@@ -121,9 +137,6 @@ void __init load_ucode_bsp(void)
 	if (check_loader_disabled_bsp())
 		return;
 
-	if (!have_cpuid_p())
-		return;
-
 	vendor = x86_cpuid_vendor();
 	family = x86_cpuid_family();
 
@@ -157,9 +170,6 @@ void load_ucode_ap(void)
 	if (check_loader_disabled_ap())
 		return;
 
-	if (!have_cpuid_p())
-		return;
-
 	vendor = x86_cpuid_vendor();
 	family = x86_cpuid_family();
 

-- 
Regards/Gruss,
    Boris.

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

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

#1543424

On 16/12/16 11:45, Borislav Petkov wrote:
> On Fri, Dec 16, 2016 at 11:00:29AM +0100, Juergen Gross wrote:
>> Should work. I'm happy to test any patch. :-)
> 
> I'm happy that you're happy to! :-)

That makes me happy. :-D

> Let's try this below.

Okay. Results:

Xen HVM domain is working.
Xen dom0 is working.
Bare metal is working, microcode has been loaded.

So you can add my:

Tested-by: Juergen Gross <jgross@suse.com>
Acked-by: Juergen Gross <jgross@suse.com>


Juergen

> 
> Thanks!
> 
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 6996413c78c3..c4bb2f7169f6 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -44,7 +44,7 @@
>  #define DRIVER_VERSION	"2.2"
>  
>  static struct microcode_ops	*microcode_ops;
> -static bool dis_ucode_ldr;
> +static bool dis_ucode_ldr = true;
>  
>  LIST_HEAD(microcode_cache);
>  
> @@ -76,6 +76,7 @@ struct cpu_info_ctx {
>  static bool __init check_loader_disabled_bsp(void)
>  {
>  	static const char *__dis_opt_str = "dis_ucode_ldr";
> +	u32 a, b, c, d;
>  
>  #ifdef CONFIG_X86_32
>  	const char *cmdline = (const char *)__pa_nodebug(boot_command_line);
> @@ -88,8 +89,23 @@ static bool __init check_loader_disabled_bsp(void)
>  	bool *res = &dis_ucode_ldr;
>  #endif
>  
> -	if (cmdline_find_option_bool(cmdline, option))
> -		*res = true;
> +	if (!have_cpuid_p())
> +		return *res;
> +
> +	a = 1;
> +	c = 0;
> +	native_cpuid(&a, &b, &c, &d);
> +
> +	/*
> +	 * CPUID(1).ECX[31]: reserved for hypervisor use. This is still not
> +	 * completely accurate as xen pv guests don't see that CPUID bit set but
> +	 * that's good enough as they don't land on the BSP path anyway.
> +	 */
> +	if (c & BIT(31))
> +		return *res;
> +
> +	if (cmdline_find_option_bool(cmdline, option) <= 0)
> +		*res = false;
>  
>  	return *res;
>  }
> @@ -121,9 +137,6 @@ void __init load_ucode_bsp(void)
>  	if (check_loader_disabled_bsp())
>  		return;
>  
> -	if (!have_cpuid_p())
> -		return;
> -
>  	vendor = x86_cpuid_vendor();
>  	family = x86_cpuid_family();
>  
> @@ -157,9 +170,6 @@ void load_ucode_ap(void)
>  	if (check_loader_disabled_ap())
>  		return;
>  
> -	if (!have_cpuid_p())
> -		return;
> -
>  	vendor = x86_cpuid_vendor();
>  	family = x86_cpuid_family();
>  
> 

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

#1543440

On Fri, Dec 16, 2016 at 01:15:56PM +0100, Juergen Gross wrote:
> On 16/12/16 11:45, Borislav Petkov wrote:
> > On Fri, Dec 16, 2016 at 11:00:29AM +0100, Juergen Gross wrote:
> >> Should work. I'm happy to test any patch. :-)
> > 
> > I'm happy that you're happy to! :-)
> 
> That makes me happy. :-D
> 
> > Let's try this below.
> 
> Okay. Results:
> 
> Xen HVM domain is working.
> Xen dom0 is working.
> Bare metal is working, microcode has been loaded.
> 
> So you can add my:
> 
> Tested-by: Juergen Gross <jgross@suse.com>
> Acked-by: Juergen Gross <jgross@suse.com>

Thanks Jürgen, much appreciated!

-- 
Regards/Gruss,
    Boris.

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

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

#1543452

On Fri, Dec 16, 2016 at 01:19:03PM +0100, Borislav Petkov wrote:
> > Okay. Results:
> > 
> > Xen HVM domain is working.
> > Xen dom0 is working.
> > Bare metal is working, microcode has been loaded.
> > 
> > So you can add my:
> > 
> > Tested-by: Juergen Gross <jgross@suse.com>
> > Acked-by: Juergen Gross <jgross@suse.com>
> 
> Thanks Jürgen, much appreciated!

Btw, Boris can you, too, run this one:

https://lkml.kernel.org/r/20161216104505.lk3s7fc7brrnmbq3@pd.tnic

and let me know if it fixes the issue you see?

Thanks.

-- 
Regards/Gruss,
    Boris.

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

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

#1543495

On 12/16/2016 08:07 AM, Borislav Petkov wrote:
> On Fri, Dec 16, 2016 at 01:19:03PM +0100, Borislav Petkov wrote:
>>> Okay. Results:
>>>
>>> Xen HVM domain is working.
>>> Xen dom0 is working.
>>> Bare metal is working, microcode has been loaded.
>>>
>>> So you can add my:
>>>
>>> Tested-by: Juergen Gross <jgross@suse.com>
>>> Acked-by: Juergen Gross <jgross@suse.com>
>> Thanks Jürgen, much appreciated!
> Btw, Boris can you, too, run this one:
>
> https://lkml.kernel.org/r/20161216104505.lk3s7fc7brrnmbq3@pd.tnic
>
> and let me know if it fixes the issue you see?

It works but I think both of the bugs we talked about yesterday still
need to be fixed, they are not related to Xen.

-boris

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

#1543510

On Fri, Dec 16, 2016 at 09:40:59AM -0500, Boris Ostrovsky wrote:
> It works

Thanks. Added your Tested-by.

> but I think both of the bugs we talked about yesterday still
> need to be fixed, they are not related to Xen.

For the one issue with the eq_id, I have the below cleanup for all the
args passing lined up for testing. For native_cpuid() you could send me
a patch. Unless you don't have time, then I can do it myself.

---
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 6f353bdb3a25..122ed2249367 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -116,10 +116,11 @@ static inline u16 find_equiv_id(struct equiv_cpu_entry *equiv_cpu_table,
 
 /*
  * This scans the ucode blob for the proper container as we can have multiple
- * containers glued together.
+ * containers glued together. Returns the equivalence ID from the equivalence
+ * table.
  */
-static struct container
-find_proper_container(u8 *ucode, size_t size, u16 *ret_id)
+static u16
+find_proper_container(u8 *ucode, size_t size, struct container *ret_cont)
 {
 	struct container ret = { NULL, 0 };
 	u32 eax, ebx, ecx, edx;
@@ -138,7 +139,7 @@ find_proper_container(u8 *ucode, size_t size, u16 *ret_id)
 	if (header[0] != UCODE_MAGIC ||
 	    header[1] != UCODE_EQUIV_CPU_TABLE_TYPE || /* type */
 	    header[2] == 0)                            /* size */
-		return ret;
+		return eq_id;
 
 	eax = 0x00000001;
 	ecx = 0;
@@ -163,8 +164,9 @@ find_proper_container(u8 *ucode, size_t size, u16 *ret_id)
 			 * ucode update loop below
 			 */
 			left = ret.size - offset;
-			*ret_id = eq_id;
-			return ret;
+
+			*ret_cont = ret;
+			return eq_id;
 		}
 
 		/*
@@ -189,7 +191,7 @@ find_proper_container(u8 *ucode, size_t size, u16 *ret_id)
 		ucode     = data;
 	}
 
-	return ret;
+	return eq_id;
 }
 
 static int __apply_microcode_amd(struct microcode_amd *mc_amd)
@@ -214,17 +216,18 @@ static int __apply_microcode_amd(struct microcode_amd *mc_amd)
  * and on 32-bit during save_microcode_in_initrd_amd() -- we can call
  * load_microcode_amd() to save equivalent cpu table and microcode patches in
  * kernel heap memory.
+ *
+ * Returns true if container found (sets @ret_cont), false otherwise.
  */
-static struct container
-apply_microcode_early_amd(void *ucode, size_t size, bool save_patch)
+static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
+				      struct container *ret_cont)
 {
-	struct container ret = { NULL, 0 };
 	u8 (*patch)[PATCH_MAX_SIZE];
+	u32 rev, *header, *new_rev;
+	struct container ret;
 	int offset, left;
-	u32 rev, *header;
-	u8  *data;
 	u16 eq_id = 0;
-	u32 *new_rev;
+	u8  *data;
 
 #ifdef CONFIG_X86_32
 	new_rev = (u32 *)__pa_nodebug(&ucode_new_rev);
@@ -235,11 +238,11 @@ apply_microcode_early_amd(void *ucode, size_t size, bool save_patch)
 #endif
 
 	if (check_current_patch_level(&rev, true))
-		return (struct container){ NULL, 0 };
+		return false;
 
-	ret = find_proper_container(ucode, size, &eq_id);
+	eq_id = find_proper_container(ucode, size, &ret);
 	if (!eq_id)
-		return (struct container){ NULL, 0 };
+		return false;
 
 	this_equiv_id = eq_id;
 	header = (u32 *)ret.data;
@@ -273,7 +276,11 @@ apply_microcode_early_amd(void *ucode, size_t size, bool save_patch)
 		data   += offset;
 		left   -= offset;
 	}
-	return ret;
+
+	if (ret_cont)
+		*ret_cont = ret;
+
+	return true;
 }
 
 static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
@@ -317,7 +324,7 @@ void __init load_ucode_amd_bsp(unsigned int family)
 	/* Get BSP's CPUID.EAX(1), needed in load_microcode_amd() */
 	uci->cpu_sig.sig = cpuid_eax(1);
 
-	apply_microcode_early_amd(cp.data, cp.size, true);
+	apply_microcode_early_amd(cp.data, cp.size, true, NULL);
 }
 
 #ifdef CONFIG_X86_32
@@ -349,7 +356,7 @@ void load_ucode_amd_ap(unsigned int family)
 	 * This would set amd_ucode_patch above so that the following APs can
 	 * use it directly instead of going down this path again.
 	 */
-	apply_microcode_early_amd(cp.data, cp.size, true);
+	apply_microcode_early_amd(cp.data, cp.size, true, NULL);
 }
 #else
 void load_ucode_amd_ap(unsigned int family)
@@ -387,8 +394,7 @@ void load_ucode_amd_ap(unsigned int family)
 			}
 		}
 
-		cont = apply_microcode_early_amd(cp.data, cp.size, false);
-		if (!(cont.data && cont.size)) {
+		if (apply_microcode_early_amd(cp.data, cp.size, false, &cont)) {
 			cont.size = -1;
 			return;
 		}
@@ -443,7 +449,7 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
 				return -EINVAL;
 			}
 
-			cont = find_proper_container(cp.data, cp.size, &eq_id);
+			eq_id = find_proper_container(cp.data, cp.size, &cont);
 			if (!eq_id) {
 				cont.size = -1;
 				return -EINVAL;

-- 
Regards/Gruss,
    Boris.

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

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

#1542913

On 15/12/16 17:46, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 05:12:04PM +0100, Juergen Gross wrote:
>> with today's kernel the system isn't coming up when booted as Xen dom0:
> 
> Remind me again pls, is dom0 even supposed to load microcode? Isn't the
> hypervisor supposed to apply microcode?
> 
>> Looking into the state of cpu 1 I find the following backtrace (created
>> manually by looking up addresses from a stack dump retrieved from the
>> hypervisor):
>>
>> find_cpio_data()
>> find_microcode_in_initrd()
>> __load_ucode_intel()
>> load_ucode_intel_ap()
>> cpu_init()
>> cpu_bringup()
>> cpu_bringup_and_idle()
>>
>> It seems as if load_ucode_intel_ap() is looping. You introduced a
>> possibly endless loop in it with commit fe055896.
> 
> Are you sure you mean:
> 
>   fe055896c040 ("x86/microcode: Merge the early microcode loader")
> 
> because that commit is a year old.

OMG. Sorry, somehow I managed to read the date of that patch wrong.

> So from looking at the *current* code:
> 
> 	if (apply_microcode_early(&uci, true)) {
> 
> fails probably because MSR_IA32_UCODE_REV doesn't get read properly due
> to virtualized MSRs, bla, yadda yadda...

I'll check.

BTW: Adding a retry count doesn't help. Just tried.

> But before we debug this further, I'd like to make sure I'm debugging
> the proper thing and not some situation again where xen wasn't even
> supposed to run the microcode loader but it does it anyway...

I guess this might be the case.


Juergen

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web