[PATCH v1 0/2] Fix Broadwell microcode update after idle-scrub was added

[PATCH v1 0/2] Fix Broadwell microcode update after idle-scrub was added

[Sergey Dyasli]

This issue was discovered during internal testing.

Sergey Dyasli (2):
  system_state: introduce SYS_STATE_smp_booted
  common/page_alloc: don't idle-scrub before microcode update

 xen/arch/arm/setup.c     | 6 ++++++
 xen/arch/x86/setup.c     | 4 ++++
 xen/common/page_alloc.c  | 7 +++++++
 xen/include/xen/kernel.h | 1 +
 4 files changed, 18 insertions(+)

-- 
2.17.1


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

[PATCH v1 1/2] system_state: introduce SYS_STATE_smp_booted

[Sergey Dyasli]

The new state means that all secondary CPUs are up. On x86 this also
means that a microcode was (potentially) updated on all CPUs.

On ARM side of things, additionally set system_state to SYS_STATE_smp_boot
just before bringing up secondary CPUs.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/arm/setup.c     | 4 ++++
 xen/arch/x86/setup.c     | 2 ++
 xen/include/xen/kernel.h | 1 +
 3 files changed, 7 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index e83221ab79..21baee534e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -915,6 +915,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     console_init_postirq();
 
+    system_state = SYS_STATE_smp_boot;
+
     do_presmp_initcalls();
 
     for_each_present_cpu ( i )
@@ -930,6 +932,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     printk("Brought up %ld CPUs\n", (long)num_online_cpus());
     /* TODO: smp_cpus_done(); */
 
+    system_state = SYS_STATE_smp_booted;
+
     setup_virt_paging();
 
     iommu_setup();
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 9cbff22fb3..189481608d 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1683,6 +1683,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         }
     }
 
+    system_state = SYS_STATE_smp_booted;
+
     printk("Brought up %ld CPUs\n", (long)num_online_cpus());
     if ( num_parked )
         printk(XENLOG_INFO "Parked %u CPUs\n", num_parked);
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
index 548b64da9f..bbf255943f 100644
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -93,6 +93,7 @@ extern enum system_state {
     SYS_STATE_early_boot,
     SYS_STATE_boot,
     SYS_STATE_smp_boot,
+    SYS_STATE_smp_booted,
     SYS_STATE_active,
     SYS_STATE_suspend,
     SYS_STATE_resume
-- 
2.17.1


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

[PATCH v1 2/2] common/page_alloc: don't idle-scrub before microcode update

[Sergey Dyasli]

Some x86 CPUs has errata regarding microcode updates. The most notorious
is Broadwell's BDX90: "Loading Microcode ... May Result in a System Hang".
(URL: https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/xeon-e7-v4-spec-update.pdf)

CPUs are supposed to be idle during initial microcode update. Idle-scrub
changes this, making a CPU to go scrubbing (memset) right after it was
brought up. This can get in a way of microcode update for other CPUs,
which results in a system hang:

    [    0.000000] CPU Vendor: Intel, Family 6 (0x6), Model 71 (0x47), Stepping 1 (raw 00040671)
    ...
    [    2.598813] HVM: Hardware Assisted Paging (HAP) detected
    [    2.600211] HVM: HAP page sizes: 4kB, 2MB, 1GB
    [    0.000000] microcode: CPU2 updated from revision 0x11 to 0x1e, date = 2018-04-03
    [    0.000000] microcode: CPU4 updated from revision 0x11 to 0x1e, d€\b \b^[[2J^[[1;1H^[[2J

Prevent this situation by disabling idle scrubbing until
SYS_STATE_smp_booted is reached.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/arm/setup.c    | 2 ++
 xen/arch/x86/setup.c    | 2 ++
 xen/common/page_alloc.c | 7 +++++++
 3 files changed, 11 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 21baee534e..9120c5092d 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -933,6 +933,8 @@ void __init start_xen(unsigned long boot_phys_offset,
     /* TODO: smp_cpus_done(); */
 
     system_state = SYS_STATE_smp_booted;
+    /* Wake up secondary CPUs to start idle memory scrubbing */
+    smp_send_event_check_mask(&cpu_online_map);
 
     setup_virt_paging();
 
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 189481608d..fea83aee5b 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1684,6 +1684,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     }
 
     system_state = SYS_STATE_smp_booted;
+    /* Wake up secondary CPUs to start idle memory scrubbing */
+    smp_send_event_check_mask(&cpu_online_map);
 
     printk("Brought up %ld CPUs\n", (long)num_online_cpus());
     if ( num_parked )
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 4a2cbda1db..a82d70464e 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1261,6 +1261,13 @@ bool scrub_free_pages(void)
     nodeid_t node;
     unsigned int cnt = 0;
   
+    /*
+     * Don't start scrubbing until all secondary CPUs have booted and
+     * updated their microcode.
+     */
+    if ( system_state < SYS_STATE_smp_booted )
+        return false;
+
     node = node_to_scrub(true);
     if ( node == NUMA_NO_NODE )
         return false;
-- 
2.17.1


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

Re: [PATCH v1 2/2] common/page_alloc: don't idle-scrub before microcode update

[Julien Grall]

Hi,

On 11/27/18 10:00 AM, Sergey Dyasli wrote:
> Some x86 CPUs has errata regarding microcode updates. The most notorious
> is Broadwell's BDX90: "Loading Microcode ... May Result in a System Hang".
> (URL: https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/xeon-e7-v4-spec-update.pdf)
> 
> CPUs are supposed to be idle during initial microcode update. Idle-scrub
> changes this, making a CPU to go scrubbing (memset) right after it was
> brought up. This can get in a way of microcode update for other CPUs,
> which results in a system hang:
> 
>      [    0.000000] CPU Vendor: Intel, Family 6 (0x6), Model 71 (0x47), Stepping 1 (raw 00040671)
>      ...
>      [    2.598813] HVM: Hardware Assisted Paging (HAP) detected
>      [    2.600211] HVM: HAP page sizes: 4kB, 2MB, 1GB
>      [    0.000000] microcode: CPU2 updated from revision 0x11 to 0x1e, date = 2018-04-03
>      [    0.000000] microcode: CPU4 updated from revision 0x11 to 0x1e, d€\b \b^[[2J^[[1;1H^[[2J
> 
> Prevent this situation by disabling idle scrubbing until
> SYS_STATE_smp_booted is reached.

I am not aware of any issue on Arm that requires delaying the idle 
scrubbing. It is actually probably better to avoid delaying it as it may 
take a long time to boot all CPUs on platform with a high number of 
cores (48 cores or upper).

Cheers,

-- 
Julien Grall

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

Re: [PATCH v1 2/2] common/page_alloc: don't idle-scrub before microcode update

[Jan Beulich]

>>> On 27.11.18 at 11:10, <julien.grall@arm.com> wrote:
> Hi,
> 
> On 11/27/18 10:00 AM, Sergey Dyasli wrote:
>> Some x86 CPUs has errata regarding microcode updates. The most notorious
>> is Broadwell's BDX90: "Loading Microcode ... May Result in a System Hang".
>> (URL: 
> https://www.intel.com/content/dam/www/public/us/en/documents/specification-up 
> dates/xeon-e7-v4-spec-update.pdf)
>> 
>> CPUs are supposed to be idle during initial microcode update. Idle-scrub
>> changes this, making a CPU to go scrubbing (memset) right after it was
>> brought up. This can get in a way of microcode update for other CPUs,
>> which results in a system hang:
>> 
>>      [    0.000000] CPU Vendor: Intel, Family 6 (0x6), Model 71 (0x47), 
> Stepping 1 (raw 00040671)
>>      ...
>>      [    2.598813] HVM: Hardware Assisted Paging (HAP) detected
>>      [    2.600211] HVM: HAP page sizes: 4kB, 2MB, 1GB
>>      [    0.000000] microcode: CPU2 updated from revision 0x11 to 0x1e, date 
> = 2018-04-03
>>      [    0.000000] microcode: CPU4 updated from revision 0x11 to 0x1e, d€ 
> [2J[1;1H[2J
>> 
>> Prevent this situation by disabling idle scrubbing until
>> SYS_STATE_smp_booted is reached.
> 
> I am not aware of any issue on Arm that requires delaying the idle 
> scrubbing. It is actually probably better to avoid delaying it as it may 
> take a long time to boot all CPUs on platform with a high number of 
> cores (48 cores or upper).

And even on x86 it would perhaps be better to delay things only
if there really is a problem (i.e. Broadwell with too low a ucode
revision).

Jan


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

Re: [PATCH v1 1/2] system_state: introduce SYS_STATE_smp_booted

[Jan Beulich]

>>> On 27.11.18 at 11:00, <sergey.dyasli@citrix.com> wrote:
> The new state means that all secondary CPUs are up. On x86 this also
> means that a microcode was (potentially) updated on all CPUs.

I'm slightly concerned by such an x86 specific: Could we settle on
a more generic description of the state all CPUs are in at that
point, like "fully functional", and only give ucode loading on x86
as an example?

> @@ -930,6 +932,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>      printk("Brought up %ld CPUs\n", (long)num_online_cpus());
>      /* TODO: smp_cpus_done(); */
>  
> +    system_state = SYS_STATE_smp_booted;

The placement here and ...

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1683,6 +1683,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          }
>      }
>  
> +    system_state = SYS_STATE_smp_booted;
> +
>      printk("Brought up %ld CPUs\n", (long)num_online_cpus());
>      if ( num_parked )
>          printk(XENLOG_INFO "Parked %u CPUs\n", num_parked);

... here differ wrt the printk()s - is this intentional, and if so why?

Jan



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

Re: [PATCH v1 2/2] common/page_alloc: don't idle-scrub before microcode update

[Jan Beulich]

>>> On 27.11.18 at 11:00, <sergey.dyasli@citrix.com> wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -933,6 +933,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>      /* TODO: smp_cpus_done(); */
>  
>      system_state = SYS_STATE_smp_booted;
> +    /* Wake up secondary CPUs to start idle memory scrubbing */
> +    smp_send_event_check_mask(&cpu_online_map);

The comment in context is, I think, a pretty good suggestion
where this new code (and possibly also the setting of system_state,
but I'm less sure with that) should live, especially on the x86 side
where this function already exists.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1261,6 +1261,13 @@ bool scrub_free_pages(void)
>      nodeid_t node;
>      unsigned int cnt = 0;
>    
> +    /*
> +     * Don't start scrubbing until all secondary CPUs have booted and
> +     * updated their microcode.
> +     */
> +    if ( system_state < SYS_STATE_smp_booted )
> +        return false;

As said on another sub-thread, I think scrubbing shouldn't be
delayed unconditionally. Quite a bit of progress can presumably
be made already by the CPUs as they come online, while others
are still being onlined.

Jan



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

Re: [PATCH v1 1/2] system_state: introduce SYS_STATE_smp_booted

[Sergey Dyasli]

On 27/11/2018 10:22, Jan Beulich wrote:
>>>> On 27.11.18 at 11:00, <sergey.dyasli@citrix.com> wrote:
>> The new state means that all secondary CPUs are up. On x86 this also
>> means that a microcode was (potentially) updated on all CPUs.
> 
> I'm slightly concerned by such an x86 specific: Could we settle on
> a more generic description of the state all CPUs are in at that
> point, like "fully functional", and only give ucode loading on x86
> as an example?

According to Julien's comment, I need to make this fix to be x86 only.
So I plan to introduce something like "bool suspend_idle_scrub" instead.

>> @@ -930,6 +932,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      printk("Brought up %ld CPUs\n", (long)num_online_cpus());
>>      /* TODO: smp_cpus_done(); */
>>  
>> +    system_state = SYS_STATE_smp_booted;
> 
> The placement here and ...
> 
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1683,6 +1683,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>>          }
>>      }
>>  
>> +    system_state = SYS_STATE_smp_booted;
>> +
>>      printk("Brought up %ld CPUs\n", (long)num_online_cpus());
>>      if ( num_parked )
>>          printk(XENLOG_INFO "Parked %u CPUs\n", num_parked);
> 
> ... here differ wrt the printk()s - is this intentional, and if so why?

This was not intentional, but it hardly matters now.

--
Thanks,
Sergey

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

Re: [PATCH v1 2/2] common/page_alloc: don't idle-scrub before microcode update

[Sergey Dyasli]

On 27/11/2018 10:15, Jan Beulich wrote:
>>>> On 27.11.18 at 11:10, <julien.grall@arm.com> wrote:
>> Hi,
>>
>> On 11/27/18 10:00 AM, Sergey Dyasli wrote:
>>> Some x86 CPUs has errata regarding microcode updates. The most notorious
>>> is Broadwell's BDX90: "Loading Microcode ... May Result in a System Hang".
>>> (URL: 
>> https://www.intel.com/content/dam/www/public/us/en/documents/specification-up 
>> dates/xeon-e7-v4-spec-update.pdf)
>>>
>>> CPUs are supposed to be idle during initial microcode update. Idle-scrub
>>> changes this, making a CPU to go scrubbing (memset) right after it was
>>> brought up. This can get in a way of microcode update for other CPUs,
>>> which results in a system hang:
>>>
>>>      [    0.000000] CPU Vendor: Intel, Family 6 (0x6), Model 71 (0x47), 
>> Stepping 1 (raw 00040671)
>>>      ...
>>>      [    2.598813] HVM: Hardware Assisted Paging (HAP) detected
>>>      [    2.600211] HVM: HAP page sizes: 4kB, 2MB, 1GB
>>>      [    0.000000] microcode: CPU2 updated from revision 0x11 to 0x1e, date 
>> = 2018-04-03
>>>      [    0.000000] microcode: CPU4 updated from revision 0x11 to 0x1e, d€ 
>> [2J[1;1H[2J
>>>
>>> Prevent this situation by disabling idle scrubbing until
>>> SYS_STATE_smp_booted is reached.
>>
>> I am not aware of any issue on Arm that requires delaying the idle 
>> scrubbing. It is actually probably better to avoid delaying it as it may 
>> take a long time to boot all CPUs on platform with a high number of 
>> cores (48 cores or upper).
> 
> And even on x86 it would perhaps be better to delay things only
> if there really is a problem (i.e. Broadwell with too low a ucode
> revision).

Except this happens on a different CPU model (still Broadwell though).
BDX90 is for model 0x4F (INTEL_FAM6_BROADWELL_X). But this bug occurs
on model 0x47 (INTEL_FAM6_BROADWELL_GT3E). I'm yet to hear from Intel
if this is BDX90 erratum or not.

--
Thanks,
Sergey

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