Can't boot as Xen dom0 due to commit fe055896

Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

Boris,

with today's kernel the system isn't coming up when booted as Xen dom0:

[   33.575326] NMI watchdog: BUG: soft lockup - CPU#0 stuck for 23s!
[swapper/0:1]
[   33.589795] Modules linked in:
[   33.596015] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-pv+ #687
[   33.608844] Hardware name: Dell Inc. Latitude E6440/0159N7, BIOS A07
06/26/2014
[   33.623590] task: ffff8801fa574dc0 task.stack: ffffc90001e68000
[   33.635535] RIP: e030:xen_hypercall_sched_op+0xa/0x20
[   33.645756] RSP: e02b:ffffc90001e6bdc8 EFLAGS: 00000246
[   33.656331] RAX: 0000000000000000 RBX: 0000000000000001 RCX:
ffffffff810013aa
[   33.670718] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
0000000000000000
[   33.685102] RBP: 0000000000000000 R08: 0000000000000000 R09:
0000000000000001
[   33.699487] R10: 000000000001b448 R11: 0000000000000246 R12:
000000000000b8b8
[   33.713872] R13: 0000000000000001 R14: 00000000001ff889 R15:
00000000001193a2
[   33.728260] FS:  0000000000000000(0000) GS:ffff8801ff800000(0000)
knlGS:0000000000000000
[   33.744562] CS:  e033 DS: 0000 ES: 0000 CR0: 0000000080050033
[   33.756158] CR2: ffffc900010f6000 CR3: 0000000001807000 CR4:
0000000000042660
[   33.770545] Call Trace:
[   33.775567]  ? xen_cpu_up+0x1da/0x500
[   33.783020]  ? put_online_cpus+0x70/0x70
[   33.790992]  ? bringup_cpu+0x1e/0x80
[   33.798269]  ? cpuhp_invoke_callback+0x7b/0x3e0
[   33.807459]  ? ring_buffer_record_is_on+0x10/0x10
[   33.816989]  ? cpuhp_up_callbacks+0x2b/0xa0
[   33.825482]  ? _cpu_up+0x6d/0xc0
[   33.832068]  ? rest_init+0x70/0x70
[   33.838998]  ? do_cpu_up+0x77/0xa0
[   33.845931]  ? smp_init+0xd7/0xdc
[   33.852688]  ? kernel_init_freeable+0xe3/0x1fd
[   33.861705]  ? rest_init+0x70/0x70
[   33.868635]  ? rest_init+0x70/0x70
[   33.875568]  ? kernel_init+0x5/0x100
[   33.882848]  ? ret_from_fork+0x25/0x30
[   33.890473] Code: cc 51 41 53 b8 1c 00 00 00 0f 05 41 5b 59 c3 cc cc
cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc 51 41 53 b8 1d 00 00 00
0f 05 <41> 5b 59 c3 cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc

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.

I'm not sure whether it is best to add a maximum loop counter or to
correct the situation in another way.


Juergen

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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.

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

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

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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.

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

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

Thanks.

-- 
Regards/Gruss,
    Boris.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Can't boot as Xen dom0 due to commit fe055896

[Jan Beulich]

>>> On 15.12.16 at 17:46, <bp@suse.de> 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?

Yes, it's the hypervisor. Dom0 can at best hand the blob to it via
hypercall, but iirc upstream Linux doesn't make use of that.

Jan

Re: Can't boot as Xen dom0 due to commit fe055896

[Jan Beulich]

>>> On 15.12.16 at 17:46, <bp@suse.de> 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?

Yes, it's the hypervisor. Dom0 can at best hand the blob to it via
hypercall, but iirc upstream Linux doesn't make use of that.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Can't boot as Xen dom0 due to commit fe055896

[Andrew Cooper]

On 15/12/16 16:53, Jan Beulich wrote:
>>>> On 15.12.16 at 17:46, <bp@suse.de> 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?
> Yes, it's the hypervisor. Dom0 can at best hand the blob to it via
> hypercall, but iirc upstream Linux doesn't make use of that.

Xen has the same logic as Linux does to look for an uncompressed
microcode blob in the initrd and apply it early in boot.

If dom0 goes looking, it should see the same microcode version as it has
in its initrd.

~Andrew

Re: Can't boot as Xen dom0 due to commit fe055896

[Andrew Cooper]

On 15/12/16 16:53, Jan Beulich wrote:
>>>> On 15.12.16 at 17:46, <bp@suse.de> 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?
> Yes, it's the hypervisor. Dom0 can at best hand the blob to it via
> hypercall, but iirc upstream Linux doesn't make use of that.

Xen has the same logic as Linux does to look for an uncompressed
microcode blob in the initrd and apply it early in boot.

If dom0 goes looking, it should see the same microcode version as it has
in its initrd.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 11:46 AM, 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.
>
> 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...
>
> 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...
>

There is an error on AMD as well. We end up being called at
load_microcode_amd() with size=0 and crash soon after.

(As a side note, I think verify_and_add_patch() should return error
codes and not crnt_size, which may be a positive number. Which it was in
my case.)

-boris

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 11:46 AM, 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.
>
> 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...
>
> 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...
>

There is an error on AMD as well. We end up being called at
load_microcode_amd() with size=0 and crash soon after.

(As a side note, I think verify_and_add_patch() should return error
codes and not crnt_size, which may be a positive number. Which it was in
my case.)

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 04:59:49PM +0000, Andrew Cooper wrote:
> Xen has the same logic as Linux does to look for an uncompressed
> microcode blob in the initrd and apply it early in boot.
> 
> If dom0 goes looking, it should see the same microcode version as it has
> in its initrd.

So it sounds to me like I wanna do this:

---
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index dd47e60aabf5..e238119b5dff 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1202,6 +1202,7 @@ config X86_REBOOTFIXUPS
 config MICROCODE
 	bool "CPU microcode loading support"
 	default y
+	depends on !XEN
 	depends on CPU_SUP_AMD || CPU_SUP_INTEL
 	select FW_LOADER
 	---help---
--

-- 
Regards/Gruss,
    Boris.

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 04:59:49PM +0000, Andrew Cooper wrote:
> Xen has the same logic as Linux does to look for an uncompressed
> microcode blob in the initrd and apply it early in boot.
> 
> If dom0 goes looking, it should see the same microcode version as it has
> in its initrd.

So it sounds to me like I wanna do this:

---
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index dd47e60aabf5..e238119b5dff 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1202,6 +1202,7 @@ config X86_REBOOTFIXUPS
 config MICROCODE
 	bool "CPU microcode loading support"
 	default y
+	depends on !XEN
 	depends on CPU_SUP_AMD || CPU_SUP_INTEL
 	select FW_LOADER
 	---help---
--

-- 
Regards/Gruss,
    Boris.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 12:00:22PM -0500, Boris Ostrovsky wrote:
> There is an error on AMD as well. We end up being called at
> load_microcode_amd() with size=0 and crash soon after.

Does that fix it?

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index dd47e60aabf5..e238119b5dff 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1202,6 +1202,7 @@ config X86_REBOOTFIXUPS
 config MICROCODE
 	bool "CPU microcode loading support"
 	default y
+	depends on !XEN
 	depends on CPU_SUP_AMD || CPU_SUP_INTEL
 	select FW_LOADER
 	---help---

> (As a side note, I think verify_and_add_patch() should return error
> codes and not crnt_size, which may be a positive number. Which it was in
> my case.)

It does return negative values when there's some failure. It returns
crnt_size which can be a positive number so that we can advance to the
next patch in the blob.

-- 
Regards/Gruss,
    Boris.

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 12:00:22PM -0500, Boris Ostrovsky wrote:
> There is an error on AMD as well. We end up being called at
> load_microcode_amd() with size=0 and crash soon after.

Does that fix it?

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index dd47e60aabf5..e238119b5dff 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1202,6 +1202,7 @@ config X86_REBOOTFIXUPS
 config MICROCODE
 	bool "CPU microcode loading support"
 	default y
+	depends on !XEN
 	depends on CPU_SUP_AMD || CPU_SUP_INTEL
 	select FW_LOADER
 	---help---

> (As a side note, I think verify_and_add_patch() should return error
> codes and not crnt_size, which may be a positive number. Which it was in
> my case.)

It does return negative values when there's some failure. It returns
crnt_size which can be a positive number so that we can advance to the
next patch in the blob.

-- 
Regards/Gruss,
    Boris.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 12:17 PM, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 12:00:22PM -0500, Boris Ostrovsky wrote:
>> There is an error on AMD as well. We end up being called at
>> load_microcode_amd() with size=0 and crash soon after.
> Does that fix it?
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index dd47e60aabf5..e238119b5dff 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1202,6 +1202,7 @@ config X86_REBOOTFIXUPS
>  config MICROCODE
>  	bool "CPU microcode loading support"
>  	default y
> +	depends on !XEN
>  	depends on CPU_SUP_AMD || CPU_SUP_INTEL
>  	select FW_LOADER
>  	---help---


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.

>
>> (As a side note, I think verify_and_add_patch() should return error
>> codes and not crnt_size, which may be a positive number. Which it was in
>> my case.)
> It does return negative values when there's some failure. 

I meant, for example:

        proc_fam = find_cpu_family_by_equiv_cpu(proc_id);
        if (!proc_fam) {
                pr_err("No patch family for equiv ID: 0x%04x\n", proc_id);
                return crnt_size;
        }

-boris

> It returns
> crnt_size which can be a positive number so that we can advance to the
> next patch in the blob.
>

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 12:17 PM, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 12:00:22PM -0500, Boris Ostrovsky wrote:
>> There is an error on AMD as well. We end up being called at
>> load_microcode_amd() with size=0 and crash soon after.
> Does that fix it?
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index dd47e60aabf5..e238119b5dff 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1202,6 +1202,7 @@ config X86_REBOOTFIXUPS
>  config MICROCODE
>  	bool "CPU microcode loading support"
>  	default y
> +	depends on !XEN
>  	depends on CPU_SUP_AMD || CPU_SUP_INTEL
>  	select FW_LOADER
>  	---help---


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.

>
>> (As a side note, I think verify_and_add_patch() should return error
>> codes and not crnt_size, which may be a positive number. Which it was in
>> my case.)
> It does return negative values when there's some failure. 

I meant, for example:

        proc_fam = find_cpu_family_by_equiv_cpu(proc_id);
        if (!proc_fam) {
                pr_err("No patch family for equiv ID: 0x%04x\n", proc_id);
                return crnt_size;
        }

-boris

> It returns
> crnt_size which can be a positive number so that we can advance to the
> next patch in the blob.
>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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?

> I meant, for example:
> 
>         proc_fam = find_cpu_family_by_equiv_cpu(proc_id);
>         if (!proc_fam) {
>                 pr_err("No patch family for equiv ID: 0x%04x\n", proc_id);
>                 return crnt_size;
>         }
> 
> -boris
> 
> > It returns
> > crnt_size which can be a positive number so that we can advance to the
> > next patch in the blob.
^^^^^^^^^^^^^^^

Read what I wrote here.

-- 
Regards/Gruss,
    Boris.

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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?

> I meant, for example:
> 
>         proc_fam = find_cpu_family_by_equiv_cpu(proc_id);
>         if (!proc_fam) {
>                 pr_err("No patch family for equiv ID: 0x%04x\n", proc_id);
>                 return crnt_size;
>         }
> 
> -boris
> 
> > It returns
> > crnt_size which can be a positive number so that we can advance to the
> > next patch in the blob.
^^^^^^^^^^^^^^^

Read what I wrote here.

-- 
Regards/Gruss,
    Boris.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 12:36 PM, 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?

I think we need at least

diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 6f353bd..383b635 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -427,7 +427,7 @@ int __init save_microcode_in_initrd_amd(unsigned int
fam)
 {
        enum ucode_state ret;
        int retval = 0;
-       u16 eq_id;
+       u16 eq_id = 0;
 
        if (!cont.data) {
                if (IS_ENABLED(CONFIG_X86_32) && (cont.size != -1)) {



This fixes my PV boot problem. I am still failing to boot HVM, will need
to look at this some more.

-boris

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 12:36 PM, 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?

I think we need at least

diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 6f353bd..383b635 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -427,7 +427,7 @@ int __init save_microcode_in_initrd_amd(unsigned int
fam)
 {
        enum ucode_state ret;
        int retval = 0;
-       u16 eq_id;
+       u16 eq_id = 0;
 
        if (!cont.data) {
                if (IS_ENABLED(CONFIG_X86_32) && (cont.size != -1)) {



This fixes my PV boot problem. I am still failing to boot HVM, will need
to look at this some more.

-boris



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 02:08:50PM -0500, Boris Ostrovsky wrote:
> This fixes my PV boot problem. I am still failing to boot HVM, will
> need to look at this some more.

No, no more stabbing in the dark and no more brown paper bags.

Please check whether CPUID(4) works that early in any xen guest and
let's add that check to a function which does something like:

bool loader_disabled(void)
{
	if (running_on_a_xen_guest)
		return true;

        if (check_loader_disabled_bsp())
                return true;

        if (!have_cpuid_p())
                return true;

	return false;

}

and call that at the entry points and be done with it.

Or if there's some other clean method to detect I'm running on a xen
guest.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 02:08:50PM -0500, Boris Ostrovsky wrote:
> This fixes my PV boot problem. I am still failing to boot HVM, will
> need to look at this some more.

No, no more stabbing in the dark and no more brown paper bags.

Please check whether CPUID(4) works that early in any xen guest and
let's add that check to a function which does something like:

bool loader_disabled(void)
{
	if (running_on_a_xen_guest)
		return true;

        if (check_loader_disabled_bsp())
                return true;

        if (!have_cpuid_p())
                return true;

	return false;

}

and call that at the entry points and be done with it.

Or if there's some other clean method to detect I'm running on a xen
guest.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 02:23 PM, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 02:08:50PM -0500, Boris Ostrovsky wrote:
>> This fixes my PV boot problem. I am still failing to boot HVM, will
>> need to look at this some more.
> No, no more stabbing in the dark and no more brown paper bags.

This fixes a bug that has nothing to do with Xen.

We are calling find_proper_container(..., &eq_id) and determine result
based on eq_id not being zero. If find_proper_container() doesn't find
anything it will not modify eq_id and so you get back whatever you
passed in.

What the patch that I sent does is no different from how
apply_microcode_early_amd() makes the call to find_proper_container.

The fact that  I am having problems with HVM may or may not have
anything to do with microcode. I don't know yet but it's separate from
save_microcode_in_initrd_amd() patch. I am pretty sure about that
because unlike PV it is failing in early boot code.

-boris

>
> Please check whether CPUID(4) works that early in any xen guest and
> let's add that check to a function which does something like:
>
> bool loader_disabled(void)
> {
> 	if (running_on_a_xen_guest)
> 		return true;
>
>         if (check_loader_disabled_bsp())
>                 return true;
>
>         if (!have_cpuid_p())
>                 return true;
>
> 	return false;
>
> }
>
> and call that at the entry points and be done with it.
>
> Or if there's some other clean method to detect I'm running on a xen
> guest.
>
> Thanks.
>

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 02:23 PM, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 02:08:50PM -0500, Boris Ostrovsky wrote:
>> This fixes my PV boot problem. I am still failing to boot HVM, will
>> need to look at this some more.
> No, no more stabbing in the dark and no more brown paper bags.

This fixes a bug that has nothing to do with Xen.

We are calling find_proper_container(..., &eq_id) and determine result
based on eq_id not being zero. If find_proper_container() doesn't find
anything it will not modify eq_id and so you get back whatever you
passed in.

What the patch that I sent does is no different from how
apply_microcode_early_amd() makes the call to find_proper_container.

The fact that  I am having problems with HVM may or may not have
anything to do with microcode. I don't know yet but it's separate from
save_microcode_in_initrd_amd() patch. I am pretty sure about that
because unlike PV it is failing in early boot code.

-boris

>
> Please check whether CPUID(4) works that early in any xen guest and
> let's add that check to a function which does something like:
>
> bool loader_disabled(void)
> {
> 	if (running_on_a_xen_guest)
> 		return true;
>
>         if (check_loader_disabled_bsp())
>                 return true;
>
>         if (!have_cpuid_p())
>                 return true;
>
> 	return false;
>
> }
>
> and call that at the entry points and be done with it.
>
> Or if there's some other clean method to detect I'm running on a xen
> guest.
>
> Thanks.
>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 02:36:46PM -0500, Boris Ostrovsky wrote:
> We are calling find_proper_container(..., &eq_id) and determine result
> based on eq_id not being zero. If find_proper_container() doesn't find
> anything it will not modify eq_id and so you get back whatever you
> passed in.

The correct fix for that is to not return struct container at all from
those functions but simply an eq_id or even an int error value or so.
Passing back a struct is simply silly. I'll prep a proper patch for
that.

> The fact that  I am having problems with HVM may or may not have
> anything to do with microcode. I don't know yet but it's separate from
> save_microcode_in_initrd_amd() patch. I am pretty sure about that
> because unlike PV it is failing in early boot code.

I still need a fix for Jürgen's use case where he lands in the
microcode loader. And that he shouldn't do in the first place when
running in a xen guest.

-- 
Regards/Gruss,
    Boris.

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 02:36:46PM -0500, Boris Ostrovsky wrote:
> We are calling find_proper_container(..., &eq_id) and determine result
> based on eq_id not being zero. If find_proper_container() doesn't find
> anything it will not modify eq_id and so you get back whatever you
> passed in.

The correct fix for that is to not return struct container at all from
those functions but simply an eq_id or even an int error value or so.
Passing back a struct is simply silly. I'll prep a proper patch for
that.

> The fact that  I am having problems with HVM may or may not have
> anything to do with microcode. I don't know yet but it's separate from
> save_microcode_in_initrd_amd() patch. I am pretty sure about that
> because unlike PV it is failing in early boot code.

I still need a fix for Jürgen's use case where he lands in the
microcode loader. And that he shouldn't do in the first place when
running in a xen guest.

-- 
Regards/Gruss,
    Boris.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 03:03 PM, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 02:36:46PM -0500, Boris Ostrovsky wrote:
>> We are calling find_proper_container(..., &eq_id) and determine result
>> based on eq_id not being zero. If find_proper_container() doesn't find
>> anything it will not modify eq_id and so you get back whatever you
>> passed in.
> The correct fix for that is to not return struct container at all from
> those functions but simply an eq_id or even an int error value or so.
> Passing back a struct is simply silly. I'll prep a proper patch for
> that.
>
>> The fact that  I am having problems with HVM may or may not have
>> anything to do with microcode. I don't know yet but it's separate from
>> save_microcode_in_initrd_amd() patch. I am pretty sure about that
>> because unlike PV it is failing in early boot code.

For AMD you also need something like:

--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -297,6 +297,7 @@ void __init load_ucode_amd_bsp(unsigned int family)
        struct cpio_data cp;
        const char *path;
        bool use_pa;
+       u32 eax = 1, ebx, ecx = 0, edx;
 
        if (IS_ENABLED(CONFIG_X86_32)) {
                uci     = (struct ucode_cpu_info
*)__pa_nodebug(ucode_cpu_info);
@@ -315,7 +316,8 @@ void __init load_ucode_amd_bsp(unsigned int family)
                return;
 
        /* Get BSP's CPUID.EAX(1), needed in load_microcode_amd() */
-       uci->cpu_sig.sig = cpuid_eax(1);
+       native_cpuid(&eax, &ebx, &ecx, &edx);
+       uci->cpu_sig.sig = eax;
 
        apply_microcode_early_amd(cp.data, cp.size, true);
 }


With CONFIG_PARAVIRT (which I suspect you don't have on) cpuid() is a
call via a pointer, you can see it, for example, if you disassemble
load_ucode_amd_bsp(). And we don't have paging on yet when we call
load_ucode_amd_bsp (at least in 32-bit mode).


> I still need a fix for Jürgen's use case where he lands in the
> microcode loader. And that he shouldn't do in the first place when
> running in a xen guest.
>

Since this happens during AP bringup PV cpuid is available and can be
checked to see whether we are a guest.

But I think it may be worthwhile trying to understand why we are dying,
it hasn't happened until now.

-boris

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 03:03 PM, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 02:36:46PM -0500, Boris Ostrovsky wrote:
>> We are calling find_proper_container(..., &eq_id) and determine result
>> based on eq_id not being zero. If find_proper_container() doesn't find
>> anything it will not modify eq_id and so you get back whatever you
>> passed in.
> The correct fix for that is to not return struct container at all from
> those functions but simply an eq_id or even an int error value or so.
> Passing back a struct is simply silly. I'll prep a proper patch for
> that.
>
>> The fact that  I am having problems with HVM may or may not have
>> anything to do with microcode. I don't know yet but it's separate from
>> save_microcode_in_initrd_amd() patch. I am pretty sure about that
>> because unlike PV it is failing in early boot code.

For AMD you also need something like:

--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -297,6 +297,7 @@ void __init load_ucode_amd_bsp(unsigned int family)
        struct cpio_data cp;
        const char *path;
        bool use_pa;
+       u32 eax = 1, ebx, ecx = 0, edx;
 
        if (IS_ENABLED(CONFIG_X86_32)) {
                uci     = (struct ucode_cpu_info
*)__pa_nodebug(ucode_cpu_info);
@@ -315,7 +316,8 @@ void __init load_ucode_amd_bsp(unsigned int family)
                return;
 
        /* Get BSP's CPUID.EAX(1), needed in load_microcode_amd() */
-       uci->cpu_sig.sig = cpuid_eax(1);
+       native_cpuid(&eax, &ebx, &ecx, &edx);
+       uci->cpu_sig.sig = eax;
 
        apply_microcode_early_amd(cp.data, cp.size, true);
 }


With CONFIG_PARAVIRT (which I suspect you don't have on) cpuid() is a
call via a pointer, you can see it, for example, if you disassemble
load_ucode_amd_bsp(). And we don't have paging on yet when we call
load_ucode_amd_bsp (at least in 32-bit mode).


> I still need a fix for Jürgen's use case where he lands in the
> microcode loader. And that he shouldn't do in the first place when
> running in a xen guest.
>

Since this happens during AP bringup PV cpuid is available and can be
checked to see whether we are a guest.

But I think it may be worthwhile trying to understand why we are dying,
it hasn't happened until now.

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 05:56:44PM -0500, Boris Ostrovsky wrote:
> With CONFIG_PARAVIRT (which I suspect you don't have on) cpuid() is a
> call via a pointer, you can see it, for example, if you disassemble
> load_ucode_amd_bsp(). And we don't have paging on yet when we call
> load_ucode_amd_bsp (at least in 32-bit mode).

That's a good catch, you can send me that one as a proper patch with a
commit message and so on...

> Since this happens during AP bringup PV cpuid is available and can be
> checked to see whether we are a guest.

What exactly do I need to check there? CPUID(4), leaf which, reg which?
How do I identify a xen hypervisor properly?

> But I think it may be worthwhile trying to understand why we are
> dying, it hasn't happened until now.

What for? We don't want to run the microcode loader on xen at all.

-- 
Regards/Gruss,
    Boris.

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 05:56:44PM -0500, Boris Ostrovsky wrote:
> With CONFIG_PARAVIRT (which I suspect you don't have on) cpuid() is a
> call via a pointer, you can see it, for example, if you disassemble
> load_ucode_amd_bsp(). And we don't have paging on yet when we call
> load_ucode_amd_bsp (at least in 32-bit mode).

That's a good catch, you can send me that one as a proper patch with a
commit message and so on...

> Since this happens during AP bringup PV cpuid is available and can be
> checked to see whether we are a guest.

What exactly do I need to check there? CPUID(4), leaf which, reg which?
How do I identify a xen hypervisor properly?

> But I think it may be worthwhile trying to understand why we are
> dying, it hasn't happened until now.

What for? We don't want to run the microcode loader on xen at all.

-- 
Regards/Gruss,
    Boris.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 06:04 PM, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 05:56:44PM -0500, Boris Ostrovsky wrote:
>> With CONFIG_PARAVIRT (which I suspect you don't have on) cpuid() is a
>> call via a pointer, you can see it, for example, if you disassemble
>> load_ucode_amd_bsp(). And we don't have paging on yet when we call
>> load_ucode_amd_bsp (at least in 32-bit mode).
>
> That's a good catch, you can send me that one as a proper patch with a
> commit message and so on...

OK, I'll send something tomorrow.

>
>> Since this happens during AP bringup PV cpuid is available and can be
>> checked to see whether we are a guest.
>
> What exactly do I need to check there? CPUID(4), leaf which, reg which?
> How do I identify a xen hypervisor properly?

You can use xen_cpuid_base(), for example. It will prevent microcode 
loading for all types of Xen guests (PV, HVM and, in the near future, 
PVH). Which I think is fine. This will require adding a #define for 
xen_cpu_base() for !CONFIG_XEN.

Alternatively, if we think that no guest should ever load microcode, we 
could check for 'x86_hyper != 0'. This will be cleaner but I can't speak 
for KVM/VMware/Hyper-V.

Neither of these solutions will work for load_ucode_bsp() because 
xen_cpuid_base() calls cpuid() (and so we are back to the above bug) and 
x86_hyper is not set yet. This is OK for Xen as PV guests never call 
load_ucode_bsp and HVM/PVH guests should be able to handle MSR accesses.

>
>> But I think it may be worthwhile trying to understand why we are
>> dying, it hasn't happened until now.
>
> What for? We don't want to run the microcode loader on xen at all.

True. But I don't think it's clear that the problem we are seeing is 
Xen-specific.

-boris

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

On 12/15/2016 06:04 PM, Borislav Petkov wrote:
> On Thu, Dec 15, 2016 at 05:56:44PM -0500, Boris Ostrovsky wrote:
>> With CONFIG_PARAVIRT (which I suspect you don't have on) cpuid() is a
>> call via a pointer, you can see it, for example, if you disassemble
>> load_ucode_amd_bsp(). And we don't have paging on yet when we call
>> load_ucode_amd_bsp (at least in 32-bit mode).
>
> That's a good catch, you can send me that one as a proper patch with a
> commit message and so on...

OK, I'll send something tomorrow.

>
>> Since this happens during AP bringup PV cpuid is available and can be
>> checked to see whether we are a guest.
>
> What exactly do I need to check there? CPUID(4), leaf which, reg which?
> How do I identify a xen hypervisor properly?

You can use xen_cpuid_base(), for example. It will prevent microcode 
loading for all types of Xen guests (PV, HVM and, in the near future, 
PVH). Which I think is fine. This will require adding a #define for 
xen_cpu_base() for !CONFIG_XEN.

Alternatively, if we think that no guest should ever load microcode, we 
could check for 'x86_hyper != 0'. This will be cleaner but I can't speak 
for KVM/VMware/Hyper-V.

Neither of these solutions will work for load_ucode_bsp() because 
xen_cpuid_base() calls cpuid() (and so we are back to the above bug) and 
x86_hyper is not set yet. This is OK for Xen as PV guests never call 
load_ucode_bsp and HVM/PVH guests should be able to handle MSR accesses.

>
>> But I think it may be worthwhile trying to understand why we are
>> dying, it hasn't happened until now.
>
> What for? We don't want to run the microcode loader on xen at all.

True. But I don't think it's clear that the problem we are seeing is 
Xen-specific.

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Henrique de Moraes Holschuh]

On Fri, 16 Dec 2016, Borislav Petkov wrote:
> What for? We don't want to run the microcode loader on xen at all.

Or under KVM, or any other hypervisor, really.

-- 
  Henrique Holschuh

Re: Can't boot as Xen dom0 due to commit fe055896

[Henrique de Moraes Holschuh]

On Fri, 16 Dec 2016, Borislav Petkov wrote:
> What for? We don't want to run the microcode loader on xen at all.

Or under KVM, or any other hypervisor, really.

-- 
  Henrique Holschuh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

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

[-- Attachment #2: 0001-x86-microcode-don-t-try-to-load-microcode-when-runni.patch --]
[-- Type: text/x-patch, Size: 1403 bytes --]

>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

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

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

[-- Attachment #2: 0001-x86-microcode-don-t-try-to-load-microcode-when-runni.patch --]
[-- Type: text/x-patch, Size: 1403 bytes --]

>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


[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 10:56:25PM -0500, Boris Ostrovsky wrote:
> You can use xen_cpuid_base(), for example. It will prevent microcode loading

Actually I want to do this at the end. CPUID(1).ECX[31] is reserved by
both vendors for hypervisor use.

> True. But I don't think it's clear that the problem we are seeing is
> Xen-specific.

If you can trigger it on baremetal, I'm all ears.

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

On Thu, Dec 15, 2016 at 10:56:25PM -0500, Boris Ostrovsky wrote:
> You can use xen_cpuid_base(), for example. It will prevent microcode loading

Actually I want to do this at the end. CPUID(1).ECX[31] is reserved by
both vendors for hypervisor use.

> True. But I don't think it's clear that the problem we are seeing is
> Xen-specific.

If you can trigger it on baremetal, I'm all ears.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] Can't boot as Xen dom0 due to commit fe055896

[Andrew Cooper]

On 16/12/2016 09:01, Borislav Petkov wrote:
> @@ -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;

This will work for any VT-x/SVM based virt, but won't work for ring
de-privileging based virt such as Xen PV and lguest.  Without CPUID
Faulting, this will still get the real hardware CPUID without the
hypervisor bit set.

How about an additional cpl check here, which should cover all the ring
de-privileging methods in a virt-neutral way?

~Andrew

Re: Can't boot as Xen dom0 due to commit fe055896

[Andrew Cooper]

On 16/12/2016 09:01, Borislav Petkov wrote:
> @@ -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;

This will work for any VT-x/SVM based virt, but won't work for ring
de-privileging based virt such as Xen PV and lguest.  Without CPUID
Faulting, this will still get the real hardware CPUID without the
hypervisor bit set.

How about an additional cpl check here, which should cover all the ring
de-privileging methods in a virt-neutral way?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Boris Ostrovsky]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

Re: Can't boot as Xen dom0 due to commit fe055896

[Borislav Petkov]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Can't boot as Xen dom0 due to commit fe055896

[Juergen Gross]

Boris,

with today's kernel the system isn't coming up when booted as Xen dom0:

[   33.575326] NMI watchdog: BUG: soft lockup - CPU#0 stuck for 23s!
[swapper/0:1]
[   33.589795] Modules linked in:
[   33.596015] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-pv+ #687
[   33.608844] Hardware name: Dell Inc. Latitude E6440/0159N7, BIOS A07
06/26/2014
[   33.623590] task: ffff8801fa574dc0 task.stack: ffffc90001e68000
[   33.635535] RIP: e030:xen_hypercall_sched_op+0xa/0x20
[   33.645756] RSP: e02b:ffffc90001e6bdc8 EFLAGS: 00000246
[   33.656331] RAX: 0000000000000000 RBX: 0000000000000001 RCX:
ffffffff810013aa
[   33.670718] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
0000000000000000
[   33.685102] RBP: 0000000000000000 R08: 0000000000000000 R09:
0000000000000001
[   33.699487] R10: 000000000001b448 R11: 0000000000000246 R12:
000000000000b8b8
[   33.713872] R13: 0000000000000001 R14: 00000000001ff889 R15:
00000000001193a2
[   33.728260] FS:  0000000000000000(0000) GS:ffff8801ff800000(0000)
knlGS:0000000000000000
[   33.744562] CS:  e033 DS: 0000 ES: 0000 CR0: 0000000080050033
[   33.756158] CR2: ffffc900010f6000 CR3: 0000000001807000 CR4:
0000000000042660
[   33.770545] Call Trace:
[   33.775567]  ? xen_cpu_up+0x1da/0x500
[   33.783020]  ? put_online_cpus+0x70/0x70
[   33.790992]  ? bringup_cpu+0x1e/0x80
[   33.798269]  ? cpuhp_invoke_callback+0x7b/0x3e0
[   33.807459]  ? ring_buffer_record_is_on+0x10/0x10
[   33.816989]  ? cpuhp_up_callbacks+0x2b/0xa0
[   33.825482]  ? _cpu_up+0x6d/0xc0
[   33.832068]  ? rest_init+0x70/0x70
[   33.838998]  ? do_cpu_up+0x77/0xa0
[   33.845931]  ? smp_init+0xd7/0xdc
[   33.852688]  ? kernel_init_freeable+0xe3/0x1fd
[   33.861705]  ? rest_init+0x70/0x70
[   33.868635]  ? rest_init+0x70/0x70
[   33.875568]  ? kernel_init+0x5/0x100
[   33.882848]  ? ret_from_fork+0x25/0x30
[   33.890473] Code: cc 51 41 53 b8 1c 00 00 00 0f 05 41 5b 59 c3 cc cc
cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc 51 41 53 b8 1d 00 00 00
0f 05 <41> 5b 59 c3 cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc cc

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.

I'm not sure whether it is best to add a maximum loop counter or to
correct the situation in another way.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel