Message ID | 20200320212453.21685-3-andrew.cooper3@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/ucode: Cleanup - Part 2/n | expand |
Actually CC Jan On Fri, Mar 20, 2020 at 09:24:50PM +0000, Andrew Cooper wrote: > In the unlikley case that patch application completes, but the resutling > revision isn't expected, sig->rev doesn't get updated to match reality. > > It will get adjusted the next time collect_cpu_info() gets called, but in the > meantime Xen might operate on a state value. Nothing good will come of this. state -> stale > > Rewrite the logic to always update the stashed revision, before worring about worring -> worrying > whether the attempt was a success or failure. > > Take the opportunity to make the printk() messages as consistent as possible. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Wei Liu <wl@xen.org> > --- > -CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wl@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/cpu/microcode/amd.c | 14 +++++++------- > xen/arch/x86/cpu/microcode/intel.c | 22 +++++++++++----------- > 2 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c > index d4b2874de6..a053e43923 100644 > --- a/xen/arch/x86/cpu/microcode/amd.c > +++ b/xen/arch/x86/cpu/microcode/amd.c > @@ -229,11 +229,11 @@ static enum microcode_match_result compare_patch( > > static int apply_microcode(const struct microcode_patch *patch) > { > - uint32_t rev; > int hw_err; > unsigned int cpu = smp_processor_id(); > struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); > const struct microcode_header_amd *hdr; > + uint32_t rev, old_rev = sig->rev; > > if ( !patch ) > return -ENOENT; > @@ -249,6 +249,7 @@ static int apply_microcode(const struct microcode_patch *patch) > > /* get patch id after patching */ > rdmsrl(MSR_AMD_PATCHLEVEL, rev); > + sig->rev = rev; > > /* > * Some processors leave the ucode blob mapping as UC after the update. > @@ -259,15 +260,14 @@ static int apply_microcode(const struct microcode_patch *patch) > /* check current patch id and patch's id for match */ > if ( hw_err || (rev != hdr->patch_id) ) > { > - printk(KERN_ERR "microcode: CPU%d update from revision " > - "%#x to %#x failed\n", cpu, rev, hdr->patch_id); > + printk(XENLOG_ERR > + "microcode: CPU%u update rev %#x to %#x failed, result %#x\n", > + cpu, old_rev, hdr->patch_id, rev); > return -EIO; > } > > - printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n", > - cpu, sig->rev, hdr->patch_id); > - > - sig->rev = rev; > + printk(XENLOG_WARNING "microcode: CPU%u updated from revision %#x to %#x\n", > + cpu, old_rev, hdr->patch_id); > > return 0; > } > diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c > index 5e9c2a9c7f..6ac5f98694 100644 > --- a/xen/arch/x86/cpu/microcode/intel.c > +++ b/xen/arch/x86/cpu/microcode/intel.c > @@ -278,10 +278,10 @@ static enum microcode_match_result compare_patch( > static int apply_microcode(const struct microcode_patch *patch) > { > uint64_t msr_content; > - unsigned int val[2]; > - unsigned int cpu_num = raw_smp_processor_id(); > + unsigned int cpu = smp_processor_id(); > struct cpu_signature *sig = &this_cpu(cpu_sig); > const struct microcode_intel *mc_intel; > + uint32_t rev, old_rev = sig->rev; > > if ( !patch ) > return -ENOENT; > @@ -302,20 +302,20 @@ static int apply_microcode(const struct microcode_patch *patch) > > /* get the current revision from MSR 0x8B */ > rdmsrl(MSR_IA32_UCODE_REV, msr_content); > - val[1] = (uint32_t)(msr_content >> 32); > + sig->rev = rev = msr_content >> 32; > > - if ( val[1] != mc_intel->hdr.rev ) > + if ( rev != mc_intel->hdr.rev ) > { > - printk(KERN_ERR "microcode: CPU%d update from revision " > - "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num, > - sig->rev, mc_intel->hdr.rev, val[1]); > + printk(XENLOG_ERR > + "microcode: CPU%u update rev %#x to %#x failed, result %#x\n", > + cpu, old_rev, mc_intel->hdr.rev, rev); > return -EIO; > } > - printk(KERN_INFO "microcode: CPU%d updated from revision " > - "%#x to %#x, date = %04x-%02x-%02x\n", > - cpu_num, sig->rev, val[1], mc_intel->hdr.year, > + > + printk(XENLOG_WARNING > + "microcode: CPU%u updated from revision %#x to %#x, date = %04x-%02x-%02x\n", > + cpu, old_rev, rev, mc_intel->hdr.year, > mc_intel->hdr.month, mc_intel->hdr.day); > - sig->rev = val[1]; > > return 0; > } > -- > 2.11.0 >
On 20.03.2020 22:24, Andrew Cooper wrote: > @@ -259,15 +260,14 @@ static int apply_microcode(const struct microcode_patch *patch) > /* check current patch id and patch's id for match */ > if ( hw_err || (rev != hdr->patch_id) ) > { > - printk(KERN_ERR "microcode: CPU%d update from revision " > - "%#x to %#x failed\n", cpu, rev, hdr->patch_id); > + printk(XENLOG_ERR > + "microcode: CPU%u update rev %#x to %#x failed, result %#x\n", > + cpu, old_rev, hdr->patch_id, rev); > return -EIO; > } > > - printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n", > - cpu, sig->rev, hdr->patch_id); > - > - sig->rev = rev; > + printk(XENLOG_WARNING "microcode: CPU%u updated from revision %#x to %#x\n", > + cpu, old_rev, hdr->patch_id); Prefer the local variable here over hdr->patch_id, just like you do on the Intel side? Jan
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index d4b2874de6..a053e43923 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -229,11 +229,11 @@ static enum microcode_match_result compare_patch( static int apply_microcode(const struct microcode_patch *patch) { - uint32_t rev; int hw_err; unsigned int cpu = smp_processor_id(); struct cpu_signature *sig = &per_cpu(cpu_sig, cpu); const struct microcode_header_amd *hdr; + uint32_t rev, old_rev = sig->rev; if ( !patch ) return -ENOENT; @@ -249,6 +249,7 @@ static int apply_microcode(const struct microcode_patch *patch) /* get patch id after patching */ rdmsrl(MSR_AMD_PATCHLEVEL, rev); + sig->rev = rev; /* * Some processors leave the ucode blob mapping as UC after the update. @@ -259,15 +260,14 @@ static int apply_microcode(const struct microcode_patch *patch) /* check current patch id and patch's id for match */ if ( hw_err || (rev != hdr->patch_id) ) { - printk(KERN_ERR "microcode: CPU%d update from revision " - "%#x to %#x failed\n", cpu, rev, hdr->patch_id); + printk(XENLOG_ERR + "microcode: CPU%u update rev %#x to %#x failed, result %#x\n", + cpu, old_rev, hdr->patch_id, rev); return -EIO; } - printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n", - cpu, sig->rev, hdr->patch_id); - - sig->rev = rev; + printk(XENLOG_WARNING "microcode: CPU%u updated from revision %#x to %#x\n", + cpu, old_rev, hdr->patch_id); return 0; } diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c index 5e9c2a9c7f..6ac5f98694 100644 --- a/xen/arch/x86/cpu/microcode/intel.c +++ b/xen/arch/x86/cpu/microcode/intel.c @@ -278,10 +278,10 @@ static enum microcode_match_result compare_patch( static int apply_microcode(const struct microcode_patch *patch) { uint64_t msr_content; - unsigned int val[2]; - unsigned int cpu_num = raw_smp_processor_id(); + unsigned int cpu = smp_processor_id(); struct cpu_signature *sig = &this_cpu(cpu_sig); const struct microcode_intel *mc_intel; + uint32_t rev, old_rev = sig->rev; if ( !patch ) return -ENOENT; @@ -302,20 +302,20 @@ static int apply_microcode(const struct microcode_patch *patch) /* get the current revision from MSR 0x8B */ rdmsrl(MSR_IA32_UCODE_REV, msr_content); - val[1] = (uint32_t)(msr_content >> 32); + sig->rev = rev = msr_content >> 32; - if ( val[1] != mc_intel->hdr.rev ) + if ( rev != mc_intel->hdr.rev ) { - printk(KERN_ERR "microcode: CPU%d update from revision " - "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num, - sig->rev, mc_intel->hdr.rev, val[1]); + printk(XENLOG_ERR + "microcode: CPU%u update rev %#x to %#x failed, result %#x\n", + cpu, old_rev, mc_intel->hdr.rev, rev); return -EIO; } - printk(KERN_INFO "microcode: CPU%d updated from revision " - "%#x to %#x, date = %04x-%02x-%02x\n", - cpu_num, sig->rev, val[1], mc_intel->hdr.year, + + printk(XENLOG_WARNING + "microcode: CPU%u updated from revision %#x to %#x, date = %04x-%02x-%02x\n", + cpu, old_rev, rev, mc_intel->hdr.year, mc_intel->hdr.month, mc_intel->hdr.day); - sig->rev = val[1]; return 0; }
In the unlikley case that patch application completes, but the resutling revision isn't expected, sig->rev doesn't get updated to match reality. It will get adjusted the next time collect_cpu_info() gets called, but in the meantime Xen might operate on a state value. Nothing good will come of this. Rewrite the logic to always update the stashed revision, before worring about whether the attempt was a success or failure. Take the opportunity to make the printk() messages as consistent as possible. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- -CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/microcode/amd.c | 14 +++++++------- xen/arch/x86/cpu/microcode/intel.c | 22 +++++++++++----------- 2 files changed, 18 insertions(+), 18 deletions(-)