| Message ID | 20191122105202.25507-1-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,for,4.13] x86/microcode: refuse to load the same revision ucode | expand |
On 22.11.2019 11:52, Sergey Dyasli wrote: > Currently if a user tries to live-load the same ucode revision that CPU > already has, he will get a single message in Xen log like: > > (XEN) 128 cores are to update their microcode > > No actual ucode loading will happen and this situation can be quite > confusing. Fix this by starting ucode update only when a newer ucode > revision has been provided. This is based on an assumption that all CPUs > in the system have the same ucode revision. If that's not the case, > the system must be considered unstable. Unstable or not, I did specifically convince Chao to handle such systems, bringing them into better shape. I can only repeat that I actually have a system where on each socket firmware loads ucode only on the first core. I don't see why boot time loading and late loading should differ in behavior for such a system. Jan
On Fri, Nov 22, 2019 at 12:19:41PM +0100, Jan Beulich wrote: >On 22.11.2019 11:52, Sergey Dyasli wrote: >> Currently if a user tries to live-load the same ucode revision that CPU >> already has, he will get a single message in Xen log like: >> >> (XEN) 128 cores are to update their microcode >> >> No actual ucode loading will happen and this situation can be quite >> confusing. Fix this by starting ucode update only when a newer ucode >> revision has been provided. This is based on an assumption that all CPUs >> in the system have the same ucode revision. If that's not the case, >> the system must be considered unstable. > >Unstable or not, I did specifically convince Chao to handle such >systems, bringing them into better shape. I can only repeat that >I actually have a system where on each socket firmware loads ucode >only on the first core. I don't see why boot time loading and late >loading should differ in behavior for such a system. Yes. I tried to load an older ucode but also got the same message. So I think an optimization can be done: we can assume that if there is a microcode_cache, all CPUs should have equal or newer revision than the microcode_cache. If the patch to be loaded has equal or older revision, we can refuse to load to avoid the heavy stop_machine(). Thanks Chao
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index 65d1f41e7c..255613569d 100644 --- a/xen/arch/x86/microcode.c +++ b/xen/arch/x86/microcode.c @@ -641,6 +641,8 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len) if ( !patch ) { ret = -ENOENT; + printk(XENLOG_WARNING "microcode: couldn't find any newer revision in " + "the provided blob!\n"); goto put; } diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c index 1e52f7f49a..dd81b5239a 100644 --- a/xen/arch/x86/microcode_amd.c +++ b/xen/arch/x86/microcode_amd.c @@ -518,7 +518,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, * If the new ucode covers current CPU, compare ucodes and store the * one with higher revision. */ - if ( (microcode_fits(mc_amd) != MIS_UCODE) && + if ( (microcode_fits(mc_amd) == NEW_UCODE) && (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) ) { xfree(saved); @@ -576,7 +576,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, free_patch(mc_amd); out: - if ( error && !patch ) + if ( error && error != -ENODATA && !patch ) patch = ERR_PTR(error); return patch; diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c index 9f66057aad..a76b860226 100644 --- a/xen/arch/x86/microcode_intel.c +++ b/xen/arch/x86/microcode_intel.c @@ -377,7 +377,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, * If the new update covers current CPU, compare updates and store the * one with higher revision. */ - if ( (microcode_update_match(&mc->hdr) != MIS_UCODE) && + if ( (microcode_update_match(&mc->hdr) == NEW_UCODE) && (!saved || (mc->hdr.rev > saved->hdr.rev)) ) { xfree(saved);
Currently if a user tries to live-load the same ucode revision that CPU already has, he will get a single message in Xen log like: (XEN) 128 cores are to update their microcode No actual ucode loading will happen and this situation can be quite confusing. Fix this by starting ucode update only when a newer ucode revision has been provided. This is based on an assumption that all CPUs in the system have the same ucode revision. If that's not the case, the system must be considered unstable. Additionally, print a user friendly message if no newer ucode can be found. This also requires ignoring -ENODATA in AMD-side code, otherwise the message given to user is: (XEN) Parsing microcode blob error -61 Which actually means that a ucode blob was parsed fine, but no matching ucode was found. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- CC: Jan Beulich <jbeulich@suse.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Chao Gao <chao.gao@intel.com> CC: Juergen Gross <jgross@suse.com> --- xen/arch/x86/microcode.c | 2 ++ xen/arch/x86/microcode_amd.c | 4 ++-- xen/arch/x86/microcode_intel.c | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-)