[v7,10/10] x86/microcode: always collect_cpu_info() during boot

Commit Message

Chao Gao May 27, 2019, 8:31 a.m. UTC

From: Sergey Dyasli <[e-mail]>

Currently cpu_sig struct is not updated during boot when either:

    1. ucode_scan is set to false (e.g. no "ucode=scan" in cmdline)
    2. initrd does not contain a microcode blob

These will result in cpu_sig.rev being 0 which affects APIC's
check_deadline_errata() and retpoline_safe() functions.

Fix this by getting ucode revision early during boot and SMP bring up.
While at it.

Signed-off-by: Sergey Dyasli <[e-mail]>
Signed-off-by: Chao Gao <[e-mail]>
---
changes in v7:
- rebase on patch 1~9
---
 xen/arch/x86/microcode.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Roger Pau Monné June 5, 2019, 2:56 p.m. UTC | #1

On Mon, May 27, 2019 at 04:31:31PM +0800, Chao Gao wrote:
> From: Sergey Dyasli <[e-mail]>
> 
> Currently cpu_sig struct is not updated during boot when either:
> 
>     1. ucode_scan is set to false (e.g. no "ucode=scan" in cmdline)
>     2. initrd does not contain a microcode blob
> 
> These will result in cpu_sig.rev being 0 which affects APIC's
> check_deadline_errata() and retpoline_safe() functions.
> 
> Fix this by getting ucode revision early during boot and SMP bring up.
> While at it.

I don't understand the last "While at it" sentence. Can it be
removed?

Is this an issue with current code? If so this could be merged ahead of
the rest of the series, and should likely be patch 1.

OTOH if the issue this patch is fixing is introduced by this series
please merge the fix with the respective patch that introduced the
bug.

Thanks, Roger.

Jan Beulich June 5, 2019, 3:05 p.m. UTC | #2

>>> On 27.05.19 at 10:31, <[e-mail]> wrote:
> From: Sergey Dyasli <[e-mail]>
> 
> Currently cpu_sig struct is not updated during boot when either:
> 
>     1. ucode_scan is set to false (e.g. no "ucode=scan" in cmdline)
>     2. initrd does not contain a microcode blob

I thought we'd already discussed this - "ucode=<number>" is not
covered by this.

> These will result in cpu_sig.rev being 0 which affects APIC's
> check_deadline_errata() and retpoline_safe() functions.
> 
> Fix this by getting ucode revision early during boot and SMP bring up.
> While at it.

While at it?

> Signed-off-by: Sergey Dyasli <[e-mail]>
> Signed-off-by: Chao Gao <[e-mail]>
> ---
> changes in v7:
> - rebase on patch 1~9

From the looks of it this doesn't depend on any of the earlier changes
(except the ucode_cpu_info -> cpu_sig change), and hence could go
in right away. Am I overlooking something? If not, all that's needed
would be clarifications of the description as per above.

> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -590,6 +590,10 @@ int __init early_microcode_init(void)
>  
>      if ( microcode_ops )
>      {
> +        rc = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
> +        if ( rc )
> +            return rc;
> +
>          if ( ucode_mod.mod_end || ucode_blob.size )
>              rc = early_microcode_parse_and_update_cpu();
>      }

Do we really need to bail on error here? I don't see anything wrong
with simply continuing. The caller doesn't care about the return
value anyway, so best effort would seem to be good enough.

Jan

Chao Gao June 11, 2019, 12:58 p.m. UTC | #3

On Wed, Jun 05, 2019 at 09:05:49AM -0600, Jan Beulich wrote:
>>>> On 27.05.19 at 10:31, <[e-mail]> wrote:
>> From: Sergey Dyasli <[e-mail]>
>> 
>> Currently cpu_sig struct is not updated during boot when either:
>> 
>>     1. ucode_scan is set to false (e.g. no "ucode=scan" in cmdline)
>>     2. initrd does not contain a microcode blob
>
>I thought we'd already discussed this - "ucode=<number>" is not
>covered by this.
>
>> These will result in cpu_sig.rev being 0 which affects APIC's
>> check_deadline_errata() and retpoline_safe() functions.
>> 
>> Fix this by getting ucode revision early during boot and SMP bring up.
>> While at it.
>
>While at it?
>
>> Signed-off-by: Sergey Dyasli <[e-mail]>
>> Signed-off-by: Chao Gao <[e-mail]>
>> ---
>> changes in v7:
>> - rebase on patch 1~9
>
>From the looks of it this doesn't depend on any of the earlier changes
>(except the ucode_cpu_info -> cpu_sig change), and hence could go
>in right away. Am I overlooking something? If not, all that's needed
>would be clarifications of the description as per above.

I think no. Will send this patch separately.

Thanks
Chao

Chao Gao June 11, 2019, 1:02 p.m. UTC | #4

On Wed, Jun 05, 2019 at 04:56:01PM +0200, Roger Pau Monné wrote:
>On Mon, May 27, 2019 at 04:31:31PM +0800, Chao Gao wrote:
>> From: Sergey Dyasli <[e-mail]>
>> 
>> Currently cpu_sig struct is not updated during boot when either:
>> 
>>     1. ucode_scan is set to false (e.g. no "ucode=scan" in cmdline)
>>     2. initrd does not contain a microcode blob
>> 
>> These will result in cpu_sig.rev being 0 which affects APIC's
>> check_deadline_errata() and retpoline_safe() functions.
>> 
>> Fix this by getting ucode revision early during boot and SMP bring up.
>> While at it.
>
>I don't understand the last "While at it" sentence. Can it be
>removed?

Yes.

>
>Is this an issue with current code? If so this could be merged ahead of
>the rest of the series, and should likely be patch 1.
>
>OTOH if the issue this patch is fixing is introduced by this series
>please merge the fix with the respective patch that introduced the
>bug.

It is the former. Will send it separately.
Really appreciate your other comments.

Thanks
Chao

Patch

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index f4a417e..8aeb152 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -590,6 +590,10 @@  int __init early_microcode_init(void)
 
     if ( microcode_ops )
     {
+        rc = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
+        if ( rc )
+            return rc;
+
         if ( ucode_mod.mod_end || ucode_blob.size )
             rc = early_microcode_parse_and_update_cpu();
     }