| Message ID | 20190325111217.25649-1-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] x86/microcode: always collect_cpu_info() during boot | expand |
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
>>> 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
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
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
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);
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(+)