diff mbox

Can't boot as Xen dom0 due to commit fe055896

Message ID e5ebe7f8-62ce-4306-c57f-f5622c05ed2d@suse.com
State New
Headers show

Commit Message

Jürgen Groß Dec. 16, 2016, 7:28 a.m. UTC 
On 15/12/16 18:36, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 12:27:49PM -0500, Boris Ostrovsky wrote:

>> It will probably fix it but I don't think we want this: it's a

>> build-time solution. Most kernels have XEN on even though they are

>> booted bare-metal.

> 

> Lemme tell you want I want: a way to detect I'm running on xen. Does

> CPUID(4) work really early, at load_ucode_bsp() time?

> 

> IOW, can I use some of the functionality hypervisor_cpuid_base() uses to

> detect xen and stop loading any further?


What you really need is to avoid being called on a Xen pv guest. And
this is easy by using xen_domain().

Not trying to load ucode in _any_ guest is an optimization only.

The attached patch works for me in dom0, bare metal and Xen HVM guest.


Juergen

Comments

Jürgen Groß Dec. 16, 2016, 9:20 a.m. UTC | #1 
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();

>  

>
Borislav Petkov Dec. 16, 2016, 9:43 a.m. UTC | #2 
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)
--
Jürgen Groß Dec. 16, 2016, 10 a.m. UTC | #3 
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
Jürgen Groß Dec. 16, 2016, 12:15 p.m. UTC | #4 
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();

>  

>
Borislav Petkov Dec. 16, 2016, 1:07 p.m. UTC | #5 
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)
--
Boris Ostrovsky Dec. 16, 2016, 2:40 p.m. UTC | #6 
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
diff mbox

Patch

From 0b56d1f86679c5dc435ab6d96eb2f68b666271bb Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Fri, 16 Dec 2016 07:18:34 +0100
Subject: [PATCH] x86/microcode: don't try to load microcode when running as a
 xen pv guest

As a Xen pv guest some mechanisms to do microcode loading or
verification might not work. As the hypervisor is responsible for
loading the microcode just skip microcode loading in case of running
under Xen.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 6996413..8dfc8bd 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -32,6 +32,8 @@ 
 #include <linux/fs.h>
 #include <linux/mm.h>
 
+#include <xen/xen.h>
+
 #include <asm/microcode_intel.h>
 #include <asm/cpu_device_id.h>
 #include <asm/microcode_amd.h>
@@ -91,6 +93,9 @@  static bool __init check_loader_disabled_bsp(void)
 	if (cmdline_find_option_bool(cmdline, option))
 		*res = true;
 
+	if (xen_domain())
+		*res = true;
+
 	return *res;
 }
 
@@ -143,6 +148,9 @@  void __init load_ucode_bsp(void)
 
 static bool check_loader_disabled_ap(void)
 {
+	if (xen_domain())
+		return true;
+
 #ifdef CONFIG_X86_32
 	return *((bool *)__pa_nodebug(&dis_ucode_ldr));
 #else
-- 
2.10.2