Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

----- bp@suse.de wrote:

> On Wed, Nov 26, 2014 at 07:39:26AM -0500, boris ostrovsky wrote:
> > https://lkml.org/lkml/2014/11/25/973 is all I have right now.
> 
> Ok, so the Code: section from this splat says:
> 
>   25:   55                      push   %ebp
>   26:   89 e5                   mov    %esp,%ebp
>   28:   83 ec 08                sub    $0x8,%esp
>   2b:*  80 3d c0 ee 97 01 00    cmpb   $0x0,0x197eec0           <--
> trapping instruction
>   32:   89 1c 24                mov    %ebx,(%esp)
>   35:   89 74 24 04             mov    %esi,0x4(%esp)
>   39:   74 12                   je     0x4d
>   3b:   8b 1c 24                mov    (%esp),%ebx
>   3e:   8b                      .byte 0x8b
>   3f:   74                      .byte 0x74
> 
> which I can correlate to the dis_ucode_ldr test here:
> 
>         .loc 1 134 0
>         .loc 1 137 0
>         cmpb    $0, dis_ucode_ldr+1073741824    #, *_11
>         je      .L46    #,
> 
> 
> so we must be faulting when accessing that dis_ucode_ldr thing. But
> you
> said that accessing it through its virtual address doesn't fix the
> issue
> either. Which is very very strange...


I was confusing you: accessing dis_ucode_ldr by virtual address does work on PV. But we then fail later, in load_ucode_intel_ap(), because it also tries to use __pa_nodebug() which can't be used by PV.

So if accessing dis_ucode_ldr by virtual address is acceptable (although I don't think it is?) then we can stick dis_ucode_ldr=1 into xen_start_kernel() and then things look OK.

A better solution may be to replace cpuid in x86_guest() with 'return pv_info.paravirt_enabled' (or paravirt_enabled(), I guess). I gave it a quick spin (32-bit only) and it seems to work. I'll see how my overnight tests behave.

-boris

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Wed, Nov 26, 2014 at 07:13:02PM -0800, Boris Ostrovsky wrote:
> I was confusing you: accessing dis_ucode_ldr by virtual address does
> work on PV. But we then fail later, in load_ucode_intel_ap(), because
> it also tries to use __pa_nodebug() which can't be used by PV.
>
> So if accessing dis_ucode_ldr by virtual address is acceptable
> (although I don't think it is?) then we can stick dis_ucode_ldr=1 into
> xen_start_kernel() and then things look OK.
>
> A better solution may be to replace cpuid in x86_guest() with 'return
> pv_info.paravirt_enabled' (or paravirt_enabled(), I guess). I gave
> it a quick spin (32-bit only) and it seems to work. I'll see how my
> overnight tests behave.

Ok, but let's have a clean design: maybe have a weak default stub which
returns false when PARAVIRT is not enabled in the .config and then add
an override in, say, arch/x86/kernel/paravirt.c which returns true when
running as a guest. Something like that, at least.

I can imagine other stuff wanting to use the dynamic checking at runtime
too...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Konrad Rzeszutek Wilk]

On Thu, Nov 27, 2014 at 10:12:28AM +0100, Borislav Petkov wrote:
> On Wed, Nov 26, 2014 at 07:13:02PM -0800, Boris Ostrovsky wrote:
> > I was confusing you: accessing dis_ucode_ldr by virtual address does
> > work on PV. But we then fail later, in load_ucode_intel_ap(), because
> > it also tries to use __pa_nodebug() which can't be used by PV.
> >
> > So if accessing dis_ucode_ldr by virtual address is acceptable
> > (although I don't think it is?) then we can stick dis_ucode_ldr=1 into
> > xen_start_kernel() and then things look OK.
> >
> > A better solution may be to replace cpuid in x86_guest() with 'return
> > pv_info.paravirt_enabled' (or paravirt_enabled(), I guess). I gave
> > it a quick spin (32-bit only) and it seems to work. I'll see how my
> > overnight tests behave.
> 
> Ok, but let's have a clean design: maybe have a weak default stub which
> returns false when PARAVIRT is not enabled in the .config and then add
> an override in, say, arch/x86/kernel/paravirt.c which returns true when
> running as a guest. Something like that, at least.

You are describing 'paravirt_enabled()' :-)
> 
> I can imagine other stuff wanting to use the dynamic checking at runtime
> too...
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Thu, Nov 27, 2014 at 11:21:19AM -0500, Konrad Rzeszutek Wilk wrote:
> > Ok, but let's have a clean design: maybe have a weak default stub which
> > returns false when PARAVIRT is not enabled in the .config and then add
> > an override in, say, arch/x86/kernel/paravirt.c which returns true when
> > running as a guest. Something like that, at least.
> 
> You are describing 'paravirt_enabled()' :-)

Haha.

Although I have a suspicion this won't work either because we're loading
microcode very early on 32-bit, before paging has been enabled, and
accessing pv_info.paravirt_enabled will probably go boom. AFAICT, from a
quick glance.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

----- bp@suse.de wrote:

> On Thu, Nov 27, 2014 at 09:14:11AM -0800, Boris Ostrovsky wrote:
> > The overnight tests passed. This includes baremetal, HVM and PV(H),
> > 32- and 64-bit.
> 
> Cool. Want to send a proper patch for 3.18 and CC: stable?
> 

So I am not convinced that we actually update microcode on baremetal now. 

I will look into this but it would have to wait until Monday.

-boris

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Thu, Nov 27, 2014 at 09:14:11AM -0800, Boris Ostrovsky wrote:
> The overnight tests passed. This includes baremetal, HVM and PV(H),
> 32- and 64-bit.

Cool. Want to send a proper patch for 3.18 and CC: stable?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

----- bp@suse.de wrote:

> On Thu, Nov 27, 2014 at 11:21:19AM -0500, Konrad Rzeszutek Wilk
> wrote:
> > > Ok, but let's have a clean design: maybe have a weak default stub
> which
> > > returns false when PARAVIRT is not enabled in the .config and then
> add
> > > an override in, say, arch/x86/kernel/paravirt.c which returns true
> when
> > > running as a guest. Something like that, at least.
> > 
> > You are describing 'paravirt_enabled()' :-)
> 
> Haha.
> 
> Although I have a suspicion this won't work either because we're
> loading
> microcode very early on 32-bit, before paging has been enabled, and
> accessing pv_info.paravirt_enabled will probably go boom. AFAICT, from
> a
> quick glance.


The overnight tests passed. This includes baremetal, HVM and PV(H), 32- and 64-bit.

-boris

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Wed, Nov 26, 2014 at 07:39:26AM -0500, boris ostrovsky wrote:
> https://lkml.org/lkml/2014/11/25/973 is all I have right now.

Ok, so the Code: section from this splat says:

  25:   55                      push   %ebp
  26:   89 e5                   mov    %esp,%ebp
  28:   83 ec 08                sub    $0x8,%esp
  2b:*  80 3d c0 ee 97 01 00    cmpb   $0x0,0x197eec0           <-- trapping instruction
  32:   89 1c 24                mov    %ebx,(%esp)
  35:   89 74 24 04             mov    %esi,0x4(%esp)
  39:   74 12                   je     0x4d
  3b:   8b 1c 24                mov    (%esp),%ebx
  3e:   8b                      .byte 0x8b
  3f:   74                      .byte 0x74

which I can correlate to the dis_ucode_ldr test here:

        .loc 1 134 0
        .loc 1 137 0
        cmpb    $0, dis_ucode_ldr+1073741824    #, *_11
        je      .L46    #,


so we must be faulting when accessing that dis_ucode_ldr thing. But you
said that accessing it through its virtual address doesn't fix the issue
either. Which is very very strange...

Hmmm.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[boris ostrovsky]

On 11/26/2014 5:55 AM, Borislav Petkov wrote:
> On Wed, Nov 26, 2014 at 12:00:45AM -0500, Boris Ostrovsky wrote:
>> Sigh... I take this back. It breaks 32-bit baremetal. I haven't looked any
>> further but it seems to be dying very early. I suspect cpuid pv_op is not
>> set up yet. If that's true, perhaps you could check whether it is valid in
>> x86_guest()?
> Right, this is why we're using the native variants in the early loader.
> So we need a different method for detecting very early whether we're
> running as a guest.
>
> What I'd like more, though, is if we continue debugging the original
> issue where we fail in load_ucode_intel_ap(). Does it happen on this line:
>
> initrd_start_addr = (unsigned long)__pa_nodebug(*initrd_start_p);

I don't have access to my test setup right now (and won't be until late 
today at best) but I am pretty sure this was the line when I was looking 
at this yesterday.

>
> where we deref the initrd_start_p? Do you have a full splat with a Code:
> section?

https://lkml.org/lkml/2014/11/25/973 is all I have right now.


-boris

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Wed, Nov 26, 2014 at 12:00:45AM -0500, Boris Ostrovsky wrote:
> Sigh... I take this back. It breaks 32-bit baremetal. I haven't looked any
> further but it seems to be dying very early. I suspect cpuid pv_op is not
> set up yet. If that's true, perhaps you could check whether it is valid in
> x86_guest()?

Right, this is why we're using the native variants in the early loader.
So we need a different method for detecting very early whether we're
running as a guest.

What I'd like more, though, is if we continue debugging the original
issue where we fail in load_ucode_intel_ap(). Does it happen on this line:

initrd_start_addr = (unsigned long)__pa_nodebug(*initrd_start_p);

where we deref the initrd_start_p? Do you have a full splat with a Code:
section?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

On 11/25/2014 05:18 PM, Borislav Petkov wrote:
> Adding x86 people.
>
> On Tue, Nov 25, 2014 at 04:59:34PM -0500, Boris Ostrovsky wrote:
>> This should be cpuid(0x1, &eax, &ebx, &ecx, &edx). Otherwise we are not
>> getting bits that the hypervisor wants the guest to see (on Xen cpuid()
>> turns into hypercall, on baremetal it's native).
>>
>> With that change it works and
>>
>> Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Sigh... I take this back. It breaks 32-bit baremetal. I haven't looked 
any further but it seems to be dying very early. I suspect cpuid pv_op 
is not set up yet. If that's true, perhaps you could check whether it is 
valid in x86_guest()?

I won't be able to do anything tomorrow morning, the best I can hope for 
is evening.


-boris



> Thanks for testing.
>
>> (May be worth adding a comment as to what is_guest() is checking for since
>> 31 is a magic number).
> See below.
>
>> BTW, the crash had nothing to do with accessing dis_ucode_ldr, we are
>> crashing much later, in load_ucode_intel_ap(), trying to access
>> *initrd_start_p. And the reason we didn't crash before was because compiler
>> optimized out whole load_ucode_ap() since check_loader_disabled_ap() was
>> always true.
> Right, and my fix actually uncovered the original issue :-\
>
> Ok, here's a v2 which adds the check to the late loader too, for
> completeness. I'll write a proper commit message tomorrow.
>
> ---
> diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
> index 64dc362506b7..654907db5f09 100644
> --- a/arch/x86/include/asm/microcode.h
> +++ b/arch/x86/include/asm/microcode.h
> @@ -87,4 +87,9 @@ static inline int __init save_microcode_in_initrd(void)
>   }
>   #endif
>   
> +/* Check whether we're running as a guest on a hypervisor. */
> +static inline bool x86_guest(void)
> +{
> +	return !!(cpuid_ecx(1) & BIT(31));
> +}
>   #endif /* _ASM_X86_MICROCODE_H */
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index 2ce9051174e6..0b6db2a97f61 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -557,6 +557,9 @@ static int __init microcode_init(void)
>   	struct cpuinfo_x86 *c = &cpu_data(0);
>   	int error;
>   
> +	if (x86_guest())
> +		return 0;
> +
>   	if (dis_ucode_ldr)
>   		return 0;
>   
> diff --git a/arch/x86/kernel/cpu/microcode/core_early.c b/arch/x86/kernel/cpu/microcode/core_early.c
> index 2c017f242a78..dfa93e74c370 100644
> --- a/arch/x86/kernel/cpu/microcode/core_early.c
> +++ b/arch/x86/kernel/cpu/microcode/core_early.c
> @@ -98,6 +98,9 @@ void __init load_ucode_bsp(void)
>   {
>   	int vendor, x86;
>   
> +	if (x86_guest())
> +		return;
> +
>   	if (check_loader_disabled_bsp())
>   		return;
>   
> @@ -134,6 +137,9 @@ void load_ucode_ap(void)
>   {
>   	int vendor, x86;
>   
> +	if (x86_guest())
> +		return;
> +
>   	if (check_loader_disabled_ap())
>   		return;
>   
>

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

Adding x86 people.

On Tue, Nov 25, 2014 at 04:59:34PM -0500, Boris Ostrovsky wrote:
> This should be cpuid(0x1, &eax, &ebx, &ecx, &edx). Otherwise we are not
> getting bits that the hypervisor wants the guest to see (on Xen cpuid()
> turns into hypercall, on baremetal it's native).
> 
> With that change it works and
> 
> Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Thanks for testing.

> (May be worth adding a comment as to what is_guest() is checking for since
> 31 is a magic number).

See below.

> BTW, the crash had nothing to do with accessing dis_ucode_ldr, we are
> crashing much later, in load_ucode_intel_ap(), trying to access
> *initrd_start_p. And the reason we didn't crash before was because compiler
> optimized out whole load_ucode_ap() since check_loader_disabled_ap() was
> always true.

Right, and my fix actually uncovered the original issue :-\

Ok, here's a v2 which adds the check to the late loader too, for
completeness. I'll write a proper commit message tomorrow.

---
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 64dc362506b7..654907db5f09 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -87,4 +87,9 @@ static inline int __init save_microcode_in_initrd(void)
 }
 #endif
 
+/* Check whether we're running as a guest on a hypervisor. */
+static inline bool x86_guest(void)
+{
+	return !!(cpuid_ecx(1) & BIT(31));
+}
 #endif /* _ASM_X86_MICROCODE_H */
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 2ce9051174e6..0b6db2a97f61 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -557,6 +557,9 @@ static int __init microcode_init(void)
 	struct cpuinfo_x86 *c = &cpu_data(0);
 	int error;
 
+	if (x86_guest())
+		return 0;
+
 	if (dis_ucode_ldr)
 		return 0;
 
diff --git a/arch/x86/kernel/cpu/microcode/core_early.c b/arch/x86/kernel/cpu/microcode/core_early.c
index 2c017f242a78..dfa93e74c370 100644
--- a/arch/x86/kernel/cpu/microcode/core_early.c
+++ b/arch/x86/kernel/cpu/microcode/core_early.c
@@ -98,6 +98,9 @@ void __init load_ucode_bsp(void)
 {
 	int vendor, x86;
 
+	if (x86_guest())
+		return;
+
 	if (check_loader_disabled_bsp())
 		return;
 
@@ -134,6 +137,9 @@ void load_ucode_ap(void)
 {
 	int vendor, x86;
 
+	if (x86_guest())
+		return;
+
 	if (check_loader_disabled_ap())
 		return;
 

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

On 11/25/2014 04:17 PM, Borislav Petkov wrote:
> On Tue, Nov 25, 2014 at 03:36:34PM -0500, Konrad Rzeszutek Wilk wrote:
>> Is there an use-case for this in virtualization at all?
> Not that I know of...
>
>> Why not make it in general then? Like:
>>
>> if (cpu_has_hypervisor)
>> 	return;
> Ah, good idea. Although we need to do it by-foot because the cpu_has
> stuff hasn't been initialized yet that early. Boris, I'm guessing
> something that should work... ?
>
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/core_early.c b/arch/x86/kernel/cpu/microcode/core_early.c
> index 2c017f242a78..77137b317e2a 100644
> --- a/arch/x86/kernel/cpu/microcode/core_early.c
> +++ b/arch/x86/kernel/cpu/microcode/core_early.c
> @@ -74,6 +74,16 @@ static int x86_family(void)
>   	return x86;
>   }
>   
> +static bool x86_guest(void)
> +{
> +	u32 eax = 0x1;
> +	u32 ebx, ecx = 0, edx;
> +
> +	native_cpuid(&eax, &ebx, &ecx, &edx);

This should be cpuid(0x1, &eax, &ebx, &ecx, &edx). Otherwise we are not 
getting bits that the hypervisor wants the guest to see (on Xen cpuid() 
turns into hypercall, on baremetal it's native).

With that change it works and

Tested-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

(May be worth adding a comment as to what is_guest() is checking for 
since 31 is a magic number).

BTW, the crash had nothing to do with accessing dis_ucode_ldr, we are 
crashing much later, in load_ucode_intel_ap(), trying to access 
*initrd_start_p. And the reason we didn't crash before was because 
compiler optimized out whole load_ucode_ap() since 
check_loader_disabled_ap() was always true.

Thanks.
-boris

> +
> +	return !!(ecx & BIT(31));
> +}
> +
>   static bool __init check_loader_disabled_bsp(void)
>   {
>   #ifdef CONFIG_X86_32
> @@ -98,6 +108,9 @@ void __init load_ucode_bsp(void)
>   {
>   	int vendor, x86;
>   
> +	if (x86_guest())
> +		return;
> +
>   	if (check_loader_disabled_bsp())
>   		return;
>   
> @@ -134,6 +147,9 @@ void load_ucode_ap(void)
>   {
>   	int vendor, x86;
>   
> +	if (x86_guest())
> +		return;
> +
>   	if (check_loader_disabled_ap())
>   		return;
>

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Tue, Nov 25, 2014 at 03:36:34PM -0500, Konrad Rzeszutek Wilk wrote:
> Is there an use-case for this in virtualization at all?

Not that I know of...

> Why not make it in general then? Like:
> 
> if (cpu_has_hypervisor)
> 	return;

Ah, good idea. Although we need to do it by-foot because the cpu_has
stuff hasn't been initialized yet that early. Boris, I'm guessing
something that should work... ?

---
diff --git a/arch/x86/kernel/cpu/microcode/core_early.c b/arch/x86/kernel/cpu/microcode/core_early.c
index 2c017f242a78..77137b317e2a 100644
--- a/arch/x86/kernel/cpu/microcode/core_early.c
+++ b/arch/x86/kernel/cpu/microcode/core_early.c
@@ -74,6 +74,16 @@ static int x86_family(void)
 	return x86;
 }
 
+static bool x86_guest(void)
+{
+	u32 eax = 0x1;
+	u32 ebx, ecx = 0, edx;
+
+	native_cpuid(&eax, &ebx, &ecx, &edx);
+
+	return !!(ecx & BIT(31));
+}
+
 static bool __init check_loader_disabled_bsp(void)
 {
 #ifdef CONFIG_X86_32
@@ -98,6 +108,9 @@ void __init load_ucode_bsp(void)
 {
 	int vendor, x86;
 
+	if (x86_guest())
+		return;
+
 	if (check_loader_disabled_bsp())
 		return;
 
@@ -134,6 +147,9 @@ void load_ucode_ap(void)
 {
 	int vendor, x86;
 
+	if (x86_guest())
+		return;
+
 	if (check_loader_disabled_ap())
 		return;

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Konrad Rzeszutek Wilk]

On Tue, Nov 25, 2014 at 09:26:28PM +0100, Borislav Petkov wrote:
> On Tue, Nov 25, 2014 at 02:28:46PM -0500, Boris Ostrovsky wrote:
> > You'd have to decide at runtime --- many baremetal systems are
> > compiled with PARAVIRT.
> 
> Right, but the microcode loader is not used at all on PV, right?

Is there an use-case for this in virtualization at all?
> 
> If so, I'd like to add a arch_something_blabla_disabled_loader()
> function which is run in the loader init path and returns false on
> baremetal and a true when running as a xen guest. I'm not sure how the
> detection should be done, though... CPUID with the hypervisor leaf?

Why not make it in general then? Like:

if (cpu_has_hypervisor)
	return;

?

> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Tue, Nov 25, 2014 at 02:28:46PM -0500, Boris Ostrovsky wrote:
> You'd have to decide at runtime --- many baremetal systems are
> compiled with PARAVIRT.

Right, but the microcode loader is not used at all on PV, right?

If so, I'd like to add a arch_something_blabla_disabled_loader()
function which is run in the loader init path and returns false on
baremetal and a true when running as a xen guest. I'm not sure how the
detection should be done, though... CPUID with the hypervisor leaf?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

On 11/25/2014 02:08 PM, Borislav Petkov wrote:
> On Tue, Nov 25, 2014 at 01:55:29PM -0500, Boris Ostrovsky wrote:
>> On 11/25/2014 01:43 PM, Borislav Petkov wrote:
>>> On Tue, Nov 25, 2014 at 01:43:26PM -0500, Boris Ostrovsky wrote:
>>>> I don't think this routine is called on PV.
>>> They're either both called or none is. At least on baremetal, that is.
>>>
>> PV guests don't start with startup_32.
>>
>> We are coming from a resume into load_ucode_ap as:
> Btw, why is this thing even running on xen? I'd like to make
> CONFIG_MICROCODE depend on !PARAVIRT and be done with it.

You'd have to decide at runtime --- many baremetal systems are compiled 
with PARAVIRT.

-boris

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

On 11/25/2014 02:03 PM, Borislav Petkov wrote:
> On Tue, Nov 25, 2014 at 01:55:29PM -0500, Boris Ostrovsky wrote:
>> On 11/25/2014 01:43 PM, Borislav Petkov wrote:
>>> On Tue, Nov 25, 2014 at 01:43:26PM -0500, Boris Ostrovsky wrote:
>>>> I don't think this routine is called on PV.
>>> They're either both called or none is. At least on baremetal, that is.
>>>
>> PV guests don't start with startup_32.
>>
>> We are coming from a resume into load_ucode_ap as:
>>
>> [   38.644599] BUG: unable to handle kernel paging request at 0197eec0
>> [   38.644599] IP: [<c1071fa6>] load_ucode_ap+0x6/0xe0
> Aha, and at that point, the APs have enabled paging and switched to
> virtual addresses, correct?

Right.

>
> Does that fix it?

Hmm... no, although expected this to fix it.

-boris

>
> ---
> diff --git a/arch/x86/kernel/cpu/microcode/core_early.c b/arch/x86/kernel/cpu/microcode/core_early.c
> index 2c017f242a78..11ff39fe9d88 100644
> --- a/arch/x86/kernel/cpu/microcode/core_early.c
> +++ b/arch/x86/kernel/cpu/microcode/core_early.c
> @@ -123,11 +123,7 @@ void __init load_ucode_bsp(void)
>   
>   static bool check_loader_disabled_ap(void)
>   {
> -#ifdef CONFIG_X86_32
> -	return *((bool *)__pa_nodebug(&dis_ucode_ldr));
> -#else
>   	return dis_ucode_ldr;
> -#endif
>   }
>   
>   void load_ucode_ap(void)
>
>
>> [   38.644599] *pdpt = 0000000003267007 *pde = 0000000000000000
>> [   38.644599] Oops: 0000 [#1] SMP
>> [   38.644599] Modules linked in: sg sd_mod dm_multipath dm_mod xen_evtchn
>> iscsi_boot_sysfs iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi
>> scsi_mod libcrc32c crc32c_generic radeon fbcon tileblit font bitblit ttm
>> softcursor drm_kms_helper x86_pkg_temp_thermal crc32c_intel xen_blkfront
>> xen_netfront xen_fbfront fb_sys_fops sysimgblt sysfillrect syscopyarea
>> xen_kbdfront xenfs xen_privcmd
>> [   38.644599] CPU: 0 PID: 9 Comm: migration/0 Tainted: G W
>> 3.18.0-rc6upstream-00001-g0de9524 #1
>> [   38.644599] task: eb894650 ti: eb89c000 task.ti: eb89c000
>> [   38.644599] EIP: 0061:[<c1071fa6>] EFLAGS: 00010082 CPU: 0
>> [   38.644599] EIP is at load_ucode_ap+0x6/0xe0
>> [   38.644599] EAX: 00000000 EBX: c1823160 ECX: 00000000 EDX: c197eee0
>> [   38.644599] ESI: eb9bded0 EDI: c1793e95 EBP: eb89de28 ESP: eb89de20
>> [   38.644599]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0069
>> [   38.644599] CR0: 80050033 CR2: 0197eec0 CR3: 03280000 CR4: 00042660
>> [   38.644599] Stack:
>> [   38.644599]  eb89de30 c10539b0 eb89de30 c1070f9d eb89de54 c140de9e
>> eb9bded0 eb89de54
>> [   38.644599]  c103dbcc 00000008 deadbeef eb9bded0 eb9bdee4 eb89de80
>> c1397f57 00000000
>> [   38.644599]  00000000 80000002 eb9bdf1c 00000000 00000002 00000003
>> eb9bded0 eb9bdee4
>> [   38.644599] Call Trace:
>> [   38.644599]  [<c10539b0>] ? i8237A_resume+0xb0/0xe0
>> [   38.644599]  [<c1070f9d>] mc_bp_resume+0x3d/0x50
>> [   38.644599]  [<c140de9e>] syscore_resume+0x4e/0x190
>> [   38.644599]  [<c103dbcc>] ? xen_timer_resume+0x3c/0x60
>> [   38.644599]  [<c1397f57>] xen_suspend+0x77/0xf0
>> [   38.644599]  [<c110dc63>] multi_cpu_stop+0x93/0xc0
>> [   38.644599]  [<c110de76>] cpu_stopper_thread+0x46/0x170
>> [   38.644599]  [<c110dbd0>] ? irq_cpu_stop_queue_work+0x20/0x20
>> [   38.644599]  [<c162c536>] ? __schedule+0x356/0x880
>> [   38.644599]  [<c10b7aab>] ? default_wake_function+0xb/0x10
>> [   38.644599]  [<c10c7c00>] ? __wake_up_common+0x40/0x70
>> [   38.644599]  [<c1630730>] ? _raw_spin_unlock_irqrestore+0x20/0x90
>> [   38.644599]  [<c1630730>] ? _raw_spin_unlock_irqrestore+0x20/0x90
>> [   38.644599]  [<c16304ef>] ? _raw_spin_lock_irqsave+0x1f/0x80
>> [   38.644599]  [<c16306bf>] ? _raw_spin_lock_irq+0xf/0x60
>> [   38.644599]  [<c10aff47>] smpboot_thread_fn+0x117/0x1a0
>> [   38.644599]  [<c10ac824>] kthread+0xa4/0xc0
>> [   38.644599]  [<c10afe30>] ? smpboot_create_threads+0x60/0x60
>> [   38.644599]  [<c1630bc1>] ret_from_kernel_thread+0x21/0x30
>> [   38.644599]  [<c10ac780>] ? kthread_freezable_should_stop+0x60/0x60
>> [   38.644599] Code: 55 cc eb 93 89 44 24 04 66 31 f6 c7 04 24 78 f0 74 c1
>> e8 84 9f 5b 00 eb ae 90 90 90 90 90 90 90 90 90 90 90 90 55 89 e5 83 ec 08
>> <80> 3d c0 ee 97 01 00 89 1c 24 89 74 24 04 74 12 8b 1c 24 8b 74
>> [   38.644599] EIP: [<c1071fa6>] load_ucode_ap+0x6/0xe0 SS:ESP 0069:eb89de20
>> [   38.644599] CR2: 000000000197eec0
>> [   38.644599] ---[ end trace 0ad7358b42202518 ]---
>> [   38.644599] Kernel panic - not syncing: Fatal exception
>> [   38.644599] Kernel Offset: 0x0 from 0xc1000000 (relocation range:
>> 0xc0000000-0xed7fdfff)
>>
>>
>>
>>
>> -boris

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Tue, Nov 25, 2014 at 01:55:29PM -0500, Boris Ostrovsky wrote:
> On 11/25/2014 01:43 PM, Borislav Petkov wrote:
> >On Tue, Nov 25, 2014 at 01:43:26PM -0500, Boris Ostrovsky wrote:
> >>I don't think this routine is called on PV.
> >They're either both called or none is. At least on baremetal, that is.
> >
> 
> PV guests don't start with startup_32.
> 
> We are coming from a resume into load_ucode_ap as:

Btw, why is this thing even running on xen? I'd like to make
CONFIG_MICROCODE depend on !PARAVIRT and be done with it.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Tue, Nov 25, 2014 at 01:55:29PM -0500, Boris Ostrovsky wrote:
> On 11/25/2014 01:43 PM, Borislav Petkov wrote:
> >On Tue, Nov 25, 2014 at 01:43:26PM -0500, Boris Ostrovsky wrote:
> >>I don't think this routine is called on PV.
> >They're either both called or none is. At least on baremetal, that is.
> >
> 
> PV guests don't start with startup_32.
> 
> We are coming from a resume into load_ucode_ap as:
> 
> [   38.644599] BUG: unable to handle kernel paging request at 0197eec0
> [   38.644599] IP: [<c1071fa6>] load_ucode_ap+0x6/0xe0

Aha, and at that point, the APs have enabled paging and switched to
virtual addresses, correct?

Does that fix it?

---
diff --git a/arch/x86/kernel/cpu/microcode/core_early.c b/arch/x86/kernel/cpu/microcode/core_early.c
index 2c017f242a78..11ff39fe9d88 100644
--- a/arch/x86/kernel/cpu/microcode/core_early.c
+++ b/arch/x86/kernel/cpu/microcode/core_early.c
@@ -123,11 +123,7 @@ void __init load_ucode_bsp(void)
 
 static bool check_loader_disabled_ap(void)
 {
-#ifdef CONFIG_X86_32
-	return *((bool *)__pa_nodebug(&dis_ucode_ldr));
-#else
 	return dis_ucode_ldr;
-#endif
 }
 
 void load_ucode_ap(void)


> [   38.644599] *pdpt = 0000000003267007 *pde = 0000000000000000
> [   38.644599] Oops: 0000 [#1] SMP
> [   38.644599] Modules linked in: sg sd_mod dm_multipath dm_mod xen_evtchn
> iscsi_boot_sysfs iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi
> scsi_mod libcrc32c crc32c_generic radeon fbcon tileblit font bitblit ttm
> softcursor drm_kms_helper x86_pkg_temp_thermal crc32c_intel xen_blkfront
> xen_netfront xen_fbfront fb_sys_fops sysimgblt sysfillrect syscopyarea
> xen_kbdfront xenfs xen_privcmd
> [   38.644599] CPU: 0 PID: 9 Comm: migration/0 Tainted: G W
> 3.18.0-rc6upstream-00001-g0de9524 #1
> [   38.644599] task: eb894650 ti: eb89c000 task.ti: eb89c000
> [   38.644599] EIP: 0061:[<c1071fa6>] EFLAGS: 00010082 CPU: 0
> [   38.644599] EIP is at load_ucode_ap+0x6/0xe0
> [   38.644599] EAX: 00000000 EBX: c1823160 ECX: 00000000 EDX: c197eee0
> [   38.644599] ESI: eb9bded0 EDI: c1793e95 EBP: eb89de28 ESP: eb89de20
> [   38.644599]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0069
> [   38.644599] CR0: 80050033 CR2: 0197eec0 CR3: 03280000 CR4: 00042660
> [   38.644599] Stack:
> [   38.644599]  eb89de30 c10539b0 eb89de30 c1070f9d eb89de54 c140de9e
> eb9bded0 eb89de54
> [   38.644599]  c103dbcc 00000008 deadbeef eb9bded0 eb9bdee4 eb89de80
> c1397f57 00000000
> [   38.644599]  00000000 80000002 eb9bdf1c 00000000 00000002 00000003
> eb9bded0 eb9bdee4
> [   38.644599] Call Trace:
> [   38.644599]  [<c10539b0>] ? i8237A_resume+0xb0/0xe0
> [   38.644599]  [<c1070f9d>] mc_bp_resume+0x3d/0x50
> [   38.644599]  [<c140de9e>] syscore_resume+0x4e/0x190
> [   38.644599]  [<c103dbcc>] ? xen_timer_resume+0x3c/0x60
> [   38.644599]  [<c1397f57>] xen_suspend+0x77/0xf0
> [   38.644599]  [<c110dc63>] multi_cpu_stop+0x93/0xc0
> [   38.644599]  [<c110de76>] cpu_stopper_thread+0x46/0x170
> [   38.644599]  [<c110dbd0>] ? irq_cpu_stop_queue_work+0x20/0x20
> [   38.644599]  [<c162c536>] ? __schedule+0x356/0x880
> [   38.644599]  [<c10b7aab>] ? default_wake_function+0xb/0x10
> [   38.644599]  [<c10c7c00>] ? __wake_up_common+0x40/0x70
> [   38.644599]  [<c1630730>] ? _raw_spin_unlock_irqrestore+0x20/0x90
> [   38.644599]  [<c1630730>] ? _raw_spin_unlock_irqrestore+0x20/0x90
> [   38.644599]  [<c16304ef>] ? _raw_spin_lock_irqsave+0x1f/0x80
> [   38.644599]  [<c16306bf>] ? _raw_spin_lock_irq+0xf/0x60
> [   38.644599]  [<c10aff47>] smpboot_thread_fn+0x117/0x1a0
> [   38.644599]  [<c10ac824>] kthread+0xa4/0xc0
> [   38.644599]  [<c10afe30>] ? smpboot_create_threads+0x60/0x60
> [   38.644599]  [<c1630bc1>] ret_from_kernel_thread+0x21/0x30
> [   38.644599]  [<c10ac780>] ? kthread_freezable_should_stop+0x60/0x60
> [   38.644599] Code: 55 cc eb 93 89 44 24 04 66 31 f6 c7 04 24 78 f0 74 c1
> e8 84 9f 5b 00 eb ae 90 90 90 90 90 90 90 90 90 90 90 90 55 89 e5 83 ec 08
> <80> 3d c0 ee 97 01 00 89 1c 24 89 74 24 04 74 12 8b 1c 24 8b 74
> [   38.644599] EIP: [<c1071fa6>] load_ucode_ap+0x6/0xe0 SS:ESP 0069:eb89de20
> [   38.644599] CR2: 000000000197eec0
> [   38.644599] ---[ end trace 0ad7358b42202518 ]---
> [   38.644599] Kernel panic - not syncing: Fatal exception
> [   38.644599] Kernel Offset: 0x0 from 0xc1000000 (relocation range:
> 0xc0000000-0xed7fdfff)
> 
> 
> 
> 
> -boris

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

On 11/25/2014 01:43 PM, Borislav Petkov wrote:
> On Tue, Nov 25, 2014 at 01:43:26PM -0500, Boris Ostrovsky wrote:
>> I don't think this routine is called on PV.
> They're either both called or none is. At least on baremetal, that is.
>

PV guests don't start with startup_32.

We are coming from a resume into load_ucode_ap as:

[   38.644599] BUG: unable to handle kernel paging request at 0197eec0
[   38.644599] IP: [<c1071fa6>] load_ucode_ap+0x6/0xe0
[   38.644599] *pdpt = 0000000003267007 *pde = 0000000000000000
[   38.644599] Oops: 0000 [#1] SMP
[   38.644599] Modules linked in: sg sd_mod dm_multipath dm_mod 
xen_evtchn iscsi_boot_sysfs iscsi_tcp libiscsi_tcp libiscsi 
scsi_transport_iscsi scsi_mod libcrc32c crc32c_generic radeon fbcon 
tileblit font bitblit ttm softcursor drm_kms_helper x86_pkg_temp_thermal 
crc32c_intel xen_blkfront xen_netfront xen_fbfront fb_sys_fops sysimgblt 
sysfillrect syscopyarea xen_kbdfront xenfs xen_privcmd
[   38.644599] CPU: 0 PID: 9 Comm: migration/0 Tainted: G W      
3.18.0-rc6upstream-00001-g0de9524 #1
[   38.644599] task: eb894650 ti: eb89c000 task.ti: eb89c000
[   38.644599] EIP: 0061:[<c1071fa6>] EFLAGS: 00010082 CPU: 0
[   38.644599] EIP is at load_ucode_ap+0x6/0xe0
[   38.644599] EAX: 00000000 EBX: c1823160 ECX: 00000000 EDX: c197eee0
[   38.644599] ESI: eb9bded0 EDI: c1793e95 EBP: eb89de28 ESP: eb89de20
[   38.644599]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0069
[   38.644599] CR0: 80050033 CR2: 0197eec0 CR3: 03280000 CR4: 00042660
[   38.644599] Stack:
[   38.644599]  eb89de30 c10539b0 eb89de30 c1070f9d eb89de54 c140de9e 
eb9bded0 eb89de54
[   38.644599]  c103dbcc 00000008 deadbeef eb9bded0 eb9bdee4 eb89de80 
c1397f57 00000000
[   38.644599]  00000000 80000002 eb9bdf1c 00000000 00000002 00000003 
eb9bded0 eb9bdee4
[   38.644599] Call Trace:
[   38.644599]  [<c10539b0>] ? i8237A_resume+0xb0/0xe0
[   38.644599]  [<c1070f9d>] mc_bp_resume+0x3d/0x50
[   38.644599]  [<c140de9e>] syscore_resume+0x4e/0x190
[   38.644599]  [<c103dbcc>] ? xen_timer_resume+0x3c/0x60
[   38.644599]  [<c1397f57>] xen_suspend+0x77/0xf0
[   38.644599]  [<c110dc63>] multi_cpu_stop+0x93/0xc0
[   38.644599]  [<c110de76>] cpu_stopper_thread+0x46/0x170
[   38.644599]  [<c110dbd0>] ? irq_cpu_stop_queue_work+0x20/0x20
[   38.644599]  [<c162c536>] ? __schedule+0x356/0x880
[   38.644599]  [<c10b7aab>] ? default_wake_function+0xb/0x10
[   38.644599]  [<c10c7c00>] ? __wake_up_common+0x40/0x70
[   38.644599]  [<c1630730>] ? _raw_spin_unlock_irqrestore+0x20/0x90
[   38.644599]  [<c1630730>] ? _raw_spin_unlock_irqrestore+0x20/0x90
[   38.644599]  [<c16304ef>] ? _raw_spin_lock_irqsave+0x1f/0x80
[   38.644599]  [<c16306bf>] ? _raw_spin_lock_irq+0xf/0x60
[   38.644599]  [<c10aff47>] smpboot_thread_fn+0x117/0x1a0
[   38.644599]  [<c10ac824>] kthread+0xa4/0xc0
[   38.644599]  [<c10afe30>] ? smpboot_create_threads+0x60/0x60
[   38.644599]  [<c1630bc1>] ret_from_kernel_thread+0x21/0x30
[   38.644599]  [<c10ac780>] ? kthread_freezable_should_stop+0x60/0x60
[   38.644599] Code: 55 cc eb 93 89 44 24 04 66 31 f6 c7 04 24 78 f0 74 
c1 e8 84 9f 5b 00 eb ae 90 90 90 90 90 90 90 90 90 90 90 90 55 89 e5 83 
ec 08 <80> 3d c0 ee 97 01 00 89 1c 24 89 74 24 04 74 12 8b 1c 24 8b 74
[   38.644599] EIP: [<c1071fa6>] load_ucode_ap+0x6/0xe0 SS:ESP 0069:eb89de20
[   38.644599] CR2: 000000000197eec0
[   38.644599] ---[ end trace 0ad7358b42202518 ]---
[   38.644599] Kernel panic - not syncing: Fatal exception
[   38.644599] Kernel Offset: 0x0 from 0xc1000000 (relocation range: 
0xc0000000-0xed7fdfff)




-boris

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

On 11/25/2014 01:45 PM, Greg Kroah-Hartman wrote:
> On Tue, Nov 25, 2014 at 01:12:10PM -0500, Boris Ostrovsky wrote:
>> On 11/19/2014 03:52 PM, Greg Kroah-Hartman wrote:
>>> 3.17-stable review patch.  If anyone has any objections, please let me know.
>>
>> This breaks PV on Xen.
> Does that mean it is also broken in Linus's tree?  If so, please fix it
> there.  If not, is there some other patch I am missing for 3.17-stable
> to resolve this?

Yes, it is broken in Linus's tree. That's the only tree that I tested 
and before we have a fix I wanted to avoid for this to trickle into 
stable trees as well (although I may be late).

-boris

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Tue, Nov 25, 2014 at 10:45:01AM -0800, Greg Kroah-Hartman wrote:
> Does that mean it is also broken in Linus's tree?

Should be.

> If so, please fix it there.

Gladly, if Boris would share some more info as to why it breaks the PV
gunk...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Greg Kroah-Hartman]

On Tue, Nov 25, 2014 at 01:12:10PM -0500, Boris Ostrovsky wrote:
> On 11/19/2014 03:52 PM, Greg Kroah-Hartman wrote:
> >3.17-stable review patch.  If anyone has any objections, please let me know.
> 
> 
> This breaks PV on Xen.

Does that mean it is also broken in Linus's tree?  If so, please fix it
there.  If not, is there some other patch I am missing for 3.17-stable
to resolve this?

thanks,

greg k-h

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Tue, Nov 25, 2014 at 01:43:26PM -0500, Boris Ostrovsky wrote:
> I don't think this routine is called on PV.

They're either both called or none is. At least on baremetal, that is.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

On 11/25/2014 01:24 PM, Borislav Petkov wrote:
> On Tue, Nov 25, 2014 at 01:12:10PM -0500, Boris Ostrovsky wrote:
>> On 11/19/2014 03:52 PM, Greg Kroah-Hartman wrote:
>>> 3.17-stable review patch.  If anyone has any objections, please let me know.
>>
>> This breaks PV on Xen.
>>
>> -boris
>>
>>> ------------------
>>>
>>> From: Borislav Petkov <bp@suse.de>
>>>
>>> commit 85be07c32496dc264661308e4d9d4e9ccaff8072 upstream.
>>>
>>> We should be accessing it through a pointer, like on the BSP.
>>>
>>> Tested-by: Richard Hendershot <rshendershot@mchsi.com>
>>> Fixes: 65cef1311d5d ("x86, microcode: Add a disable chicken bit")
>>> Signed-off-by: Borislav Petkov <bp@suse.de>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>
>>> ---
>>>   arch/x86/kernel/cpu/microcode/core_early.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> --- a/arch/x86/kernel/cpu/microcode/core_early.c
>>> +++ b/arch/x86/kernel/cpu/microcode/core_early.c
>>> @@ -124,7 +124,7 @@ void __init load_ucode_bsp(void)
>>>   static bool check_loader_disabled_ap(void)
>>>   {
>>>   #ifdef CONFIG_X86_32
>>> -	return __pa_nodebug(dis_ucode_ldr);
>>> +	return *((bool *)__pa_nodebug(&dis_ucode_ldr));
> And practically the same line in check_loader_disabled_bsp() doesn't?


I don't think this routine is called on PV.


-boris

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Borislav Petkov]

On Tue, Nov 25, 2014 at 01:12:10PM -0500, Boris Ostrovsky wrote:
> On 11/19/2014 03:52 PM, Greg Kroah-Hartman wrote:
> >3.17-stable review patch.  If anyone has any objections, please let me know.
> 
> 
> This breaks PV on Xen.
> 
> -boris
> 
> >
> >------------------
> >
> >From: Borislav Petkov <bp@suse.de>
> >
> >commit 85be07c32496dc264661308e4d9d4e9ccaff8072 upstream.
> >
> >We should be accessing it through a pointer, like on the BSP.
> >
> >Tested-by: Richard Hendershot <rshendershot@mchsi.com>
> >Fixes: 65cef1311d5d ("x86, microcode: Add a disable chicken bit")
> >Signed-off-by: Borislav Petkov <bp@suse.de>
> >Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> >---
> >  arch/x86/kernel/cpu/microcode/core_early.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >--- a/arch/x86/kernel/cpu/microcode/core_early.c
> >+++ b/arch/x86/kernel/cpu/microcode/core_early.c
> >@@ -124,7 +124,7 @@ void __init load_ucode_bsp(void)
> >  static bool check_loader_disabled_ap(void)
> >  {
> >  #ifdef CONFIG_X86_32
> >-	return __pa_nodebug(dis_ucode_ldr);
> >+	return *((bool *)__pa_nodebug(&dis_ucode_ldr));

And practically the same line in check_loader_disabled_bsp() doesn't?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Boris Ostrovsky]

On 11/19/2014 03:52 PM, Greg Kroah-Hartman wrote:
> 3.17-stable review patch.  If anyone has any objections, please let me know.


This breaks PV on Xen.

-boris

>
> ------------------
>
> From: Borislav Petkov <bp@suse.de>
>
> commit 85be07c32496dc264661308e4d9d4e9ccaff8072 upstream.
>
> We should be accessing it through a pointer, like on the BSP.
>
> Tested-by: Richard Hendershot <rshendershot@mchsi.com>
> Fixes: 65cef1311d5d ("x86, microcode: Add a disable chicken bit")
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
>   arch/x86/kernel/cpu/microcode/core_early.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/arch/x86/kernel/cpu/microcode/core_early.c
> +++ b/arch/x86/kernel/cpu/microcode/core_early.c
> @@ -124,7 +124,7 @@ void __init load_ucode_bsp(void)
>   static bool check_loader_disabled_ap(void)
>   {
>   #ifdef CONFIG_X86_32
> -	return __pa_nodebug(dis_ucode_ldr);
> +	return *((bool *)__pa_nodebug(&dis_ucode_ldr));
>   #else
>   	return dis_ucode_ldr;
>   #endif
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

[PATCH 3.17 100/141] x86, microcode: Fix accessing dis_ucode_ldr on 32-bit

[Greg Kroah-Hartman]

3.17-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Borislav Petkov <bp@suse.de>

commit 85be07c32496dc264661308e4d9d4e9ccaff8072 upstream.

We should be accessing it through a pointer, like on the BSP.

Tested-by: Richard Hendershot <rshendershot@mchsi.com>
Fixes: 65cef1311d5d ("x86, microcode: Add a disable chicken bit")
Signed-off-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 arch/x86/kernel/cpu/microcode/core_early.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/arch/x86/kernel/cpu/microcode/core_early.c
+++ b/arch/x86/kernel/cpu/microcode/core_early.c
@@ -124,7 +124,7 @@ void __init load_ucode_bsp(void)
 static bool check_loader_disabled_ap(void)
 {
 #ifdef CONFIG_X86_32
-	return __pa_nodebug(dis_ucode_ldr);
+	return *((bool *)__pa_nodebug(&dis_ucode_ldr));
 #else
 	return dis_ucode_ldr;
 #endif