Messages in this thread Patch in this message | | | Date | Tue, 27 Aug 2019 15:43:44 -0400 | From | Boris Ostrovsky <> | Subject | Re: [PATCH 1/2] x86/microcode: Update late microcode in parallel |
| |
On Mon, Aug 26, 2019 at 01:32:48PM -0700, Raj, Ashok wrote: > On Mon, Aug 26, 2019 at 08:53:05AM -0400, Boris Ostrovsky wrote: > > On 8/24/19 4:53 AM, Borislav Petkov wrote: > > > > > > +wait_for_siblings: > > > + if (__wait_for_cpus(&late_cpus_out, NSEC_PER_SEC)) > > > + panic("Timeout during microcode update!\n"); > > > + > > > /* > > > - * Increase the wait timeout to a safe value here since we're > > > - * serializing the microcode update and that could take a while on a > > > - * large number of CPUs. And that is fine as the *actual* timeout will > > > - * be determined by the last CPU finished updating and thus cut short. > > > + * At least one thread has completed update on each core. > > > + * For others, simply call the update to make sure the > > > + * per-cpu cpuinfo can be updated with right microcode > > > + * revision. > > > > > > What is the advantage of having those other threads go through > > find_patch() and (in Intel case) intel_get_microcode_revision() (which > > involves two MSR accesses) vs. having the master sibling update slaves' > > microcode revisions? There are only two things that need to be updated, > > uci->cpu_sig.rev and c->microcode. > > > > True, yes we could do that. But there is some warm and comfy feeling > that you really read the revision from the thread local copy of the revision > and it matches what was updated in the other thread sibling rather than > just hardcoding the fixup's. The code looks clean in the sense it looks like > you are attempting to upgrade but the new revision is reflected correctly > and you skip the update. > > But if you feel complelled, i'm not opposed to it as long as Boris is > happy with the changes :-).
Something like this. I only lightly tested this but if there is interest I can test it more.
From: Boris Ostrovsky <boris.ostrovsky@oracle.com> Date: Tue, 27 Aug 2019 08:26:08 -0400 Subject: [PATCH 2/2] x86/microcode: Have master sibling update slaves' data
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- arch/x86/kernel/cpu/microcode/amd.c | 21 ++++-------------- arch/x86/kernel/cpu/microcode/core.c | 40 ++++++++++++++++++++--------------- arch/x86/kernel/cpu/microcode/intel.c | 18 +++------------- 3 files changed, 30 insertions(+), 49 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c index a0e52bd..a365f72 100644 --- a/arch/x86/kernel/cpu/microcode/amd.c +++ b/arch/x86/kernel/cpu/microcode/amd.c @@ -668,11 +668,9 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig) static enum ucode_state apply_microcode_amd(int cpu) { - struct cpuinfo_x86 *c = &cpu_data(cpu); struct microcode_amd *mc_amd; struct ucode_cpu_info *uci; struct ucode_patch *p; - enum ucode_state ret; u32 rev, dummy; BUG_ON(raw_smp_processor_id() != cpu); @@ -689,10 +687,8 @@ static enum ucode_state apply_microcode_amd(int cpu) rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy); /* need to apply patch? */ - if (rev >= mc_amd->hdr.patch_id) { - ret = UCODE_OK; - goto out; - } + if (rev >= mc_amd->hdr.patch_id) + return UCODE_OK; if (__apply_microcode_amd(mc_amd)) { pr_err("CPU%d: update failed for patch_level=0x%08x\n", @@ -700,20 +696,11 @@ static enum ucode_state apply_microcode_amd(int cpu) return UCODE_ERROR; } - rev = mc_amd->hdr.patch_id; - ret = UCODE_UPDATED; - - pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev); - -out: uci->cpu_sig.rev = rev; - c->microcode = rev; - /* Update boot_cpu_data's revision too, if we're on the BSP: */ - if (c->cpu_index == boot_cpu_data.cpu_index) - boot_cpu_data.microcode = rev; + pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev); - return ret; + return UCODE_UPDATED; } static size_t install_equiv_cpu_table(const u8 *buf, size_t buf_size) diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c index 7019d4b..09643c6 100644 --- a/arch/x86/kernel/cpu/microcode/core.c +++ b/arch/x86/kernel/cpu/microcode/core.c @@ -551,6 +551,8 @@ static int __wait_for_cpus(atomic_t *t, long long timeout) static int __reload_late(void *info) { int cpu = smp_processor_id(); + struct cpuinfo_x86 *c = &cpu_data(cpu); + bool bsp = (c->cpu_index == boot_cpu_data.cpu_index); enum ucode_state err; int ret = 0; @@ -568,30 +570,34 @@ static int __reload_late(void *info) * loading attempts happen on multiple threads of an SMT core. See * below. */ - if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu) + if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu) { apply_microcode_local(&err); - else - goto wait_for_siblings; - if (err > UCODE_NFOUND) { - pr_warn("Error reloading microcode on CPU %d\n", cpu); - ret = -1; - } else if (err == UCODE_UPDATED || err == UCODE_OK) { - ret = 1; + if (err > UCODE_NFOUND) { + pr_warn("Error reloading microcode on CPU %d\n", cpu); + ret = -1; + } else if (err == UCODE_UPDATED || err == UCODE_OK) { + struct ucode_cpu_info *uci = ucode_cpu_info + cpu; + int sibling, rev = uci->cpu_sig.rev; + + /* All siblings have same revision */ + for_each_cpu(sibling, topology_sibling_cpumask(cpu)) { + c = &cpu_data(sibling); + uci = ucode_cpu_info + sibling; + + uci->cpu_sig.rev = rev; + c->microcode = rev; + } + + ret = 1; + } } -wait_for_siblings: if (__wait_for_cpus(&late_cpus_out, NSEC_PER_SEC)) panic("Timeout during microcode update!\n"); - /* - * At least one thread has completed update on each core. - * For others, simply call the update to make sure the - * per-cpu cpuinfo can be updated with right microcode - * revision. - */ - if (cpumask_first(topology_sibling_cpumask(cpu)) != cpu) - apply_microcode_local(&err); + if (bsp) + boot_cpu_data.microcode = c->microcode; return ret; } diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index ce799cf..04e9aa2 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -790,9 +790,7 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig) static enum ucode_state apply_microcode_intel(int cpu) { struct ucode_cpu_info *uci = ucode_cpu_info + cpu; - struct cpuinfo_x86 *c = &cpu_data(cpu); struct microcode_intel *mc; - enum ucode_state ret; static int prev_rev; u32 rev; @@ -814,10 +812,8 @@ static enum ucode_state apply_microcode_intel(int cpu) * already. */ rev = intel_get_microcode_revision(); - if (rev >= mc->hdr.rev) { - ret = UCODE_OK; - goto out; - } + if (rev >= mc->hdr.rev) + return UCODE_OK; /* * Writeback and invalidate caches before updating microcode to avoid @@ -845,17 +841,9 @@ static enum ucode_state apply_microcode_intel(int cpu) prev_rev = rev; } - ret = UCODE_UPDATED; - -out: uci->cpu_sig.rev = rev; - c->microcode = rev; - - /* Update boot_cpu_data's revision too, if we're on the BSP: */ - if (c->cpu_index == boot_cpu_data.cpu_index) - boot_cpu_data.microcode = rev; - return ret; + return UCODE_UPDATED; } static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter) -- 1.8.3.1
| |