[PATCH v1] x86/microcode: always collect_cpu_info() during boot

[PATCH v1] x86/microcode: always collect_cpu_info() during boot

[Sergey Dyasli]

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, protect early_microcode_update_cpu() for cases when
microcode_ops is NULL.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: Chao Gao <chao.gao@intel.com>
---
 xen/arch/x86/microcode.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 4163f50bb7..421d57e925 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -383,10 +383,15 @@ static struct notifier_block microcode_percpu_nfb = {
 
 int __init early_microcode_update_cpu(bool start_update)
 {
+    unsigned int cpu = smp_processor_id();
+    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     int rc = 0;
     void *data = NULL;
     size_t len;
 
+    if ( !microcode_ops )
+        return -ENOSYS;
+
     if ( ucode_blob.size )
     {
         len = ucode_blob.size;
@@ -397,6 +402,9 @@ int __init early_microcode_update_cpu(bool start_update)
         len = ucode_mod.mod_end;
         data = bootstrap_map(&ucode_mod);
     }
+
+    microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+
     if ( data )
     {
         if ( start_update && microcode_ops->start_update )
@@ -413,6 +421,8 @@ int __init early_microcode_update_cpu(bool start_update)
 
 int __init early_microcode_init(void)
 {
+    unsigned int cpu = smp_processor_id();
+    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     int rc;
 
     rc = microcode_init_intel();
@@ -425,6 +435,8 @@ int __init early_microcode_init(void)
 
     if ( microcode_ops )
     {
+        microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+
         if ( ucode_mod.mod_end || ucode_blob.size )
             rc = early_microcode_update_cpu(true);
 
-- 
2.17.1


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

Re: [PATCH v1] x86/microcode: always collect_cpu_info() during boot

[Chao Gao]

On Mon, Mar 25, 2019 at 11:12:17AM +0000, Sergey Dyasli wrote:
>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, protect early_microcode_update_cpu() for cases when
>microcode_ops is NULL.
>
>Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Reviewed-by: Chao Gao <chao.gao@intel.com>

Thanks
Chao

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

Re: [PATCH v1] x86/microcode: always collect_cpu_info() during boot

[Jan Beulich]

>>> On 25.03.19 at 12:12, <sergey.dyasli@citrix.com> wrote:
> 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

What about "ucode=<number>"?

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

Can't you then drop the call from microcode_update_cpu()?

> @@ -413,6 +421,8 @@ int __init early_microcode_update_cpu(bool start_update)
>  
>  int __init early_microcode_init(void)
>  {
> +    unsigned int cpu = smp_processor_id();
> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);

Would you mind moving these declarations ...

> @@ -425,6 +435,8 @@ int __init early_microcode_init(void)
>  
>      if ( microcode_ops )
>      {
> +        microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);

... into this scope?

Jan



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

Re: [PATCH v1] x86/microcode: always collect_cpu_info() during boot

[Sergey Dyasli]

On 25/03/2019 17:08, Jan Beulich wrote:
>>>> On 25.03.19 at 12:12, <sergey.dyasli@citrix.com> wrote:
>> 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
> 
> What about "ucode=<number>"?

Yes, it's equally affected, I'll update the commit message.

>> 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.
> 
> Can't you then drop the call from microcode_update_cpu()?

I tried this and found that properly handling errors requires
a call to __microcode_fini_cpu(). But instead of doing all this
refactoring now, I'd prefer this patch to go on top of Chao's
series, which greatly simplifies things in "remove struct
ucode_cpu_info". Otherwise, most of my work here would be removed
by the next version of Chao's series.

Chao,
To help me with this decision, when can we expect a v7 of your
series?

Thanks,
Sergey

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

Re: [PATCH v1] x86/microcode: always collect_cpu_info() during boot

[Chao Gao]

On Mon, Apr 01, 2019 at 11:19:19AM +0100, Sergey Dyasli wrote:
>On 25/03/2019 17:08, Jan Beulich wrote:
>>>>> On 25.03.19 at 12:12, <sergey.dyasli@citrix.com> wrote:
>>> 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
>> 
>> What about "ucode=<number>"?
>
>Yes, it's equally affected, I'll update the commit message.
>
>>> 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.
>> 
>> Can't you then drop the call from microcode_update_cpu()?
>
>I tried this and found that properly handling errors requires
>a call to __microcode_fini_cpu(). But instead of doing all this
>refactoring now, I'd prefer this patch to go on top of Chao's
>series, which greatly simplifies things in "remove struct
>ucode_cpu_info". Otherwise, most of my work here would be removed
>by the next version of Chao's series.
>
>Chao,
>To help me with this decision, when can we expect a v7 of your
>series?

Will start to work on v7 from tomorrow. And I hope that it will be finished
in this week.

Thanks
Chao

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