Re: [Xen-devel] [PATCH v2 for 4.13] x86/microcode: refuse to load the same revision ucode

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Nov 22, 2019 at 04:47:23PM +0000, Sergey Dyasli wrote:
>Currently if a user tries to live-load the same or older ucode revision
>than 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 the provided
>ucode revision is higher than the currently cached one (if any).
>This is based on the property that if microcode_cache exists, all CPUs
>in the system should have at least that ucode revision.
>
>Additionally, print a user friendly message if no newer ucode can be
>found in the provided blob. This also requires ignoring -ENODATA in
>AMD-side code, otherwise the message given to the 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@xxxxxxxxxx>

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

I wonder whether it is better to put the comparison ...

>---
>v1 --> v2:
>- compare provided ucode with the currently cached one
>
>CC: Jan Beulich <jbeulich@xxxxxxxx>
>CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>CC: Chao Gao <chao.gao@xxxxxxxxx>
>CC: Juergen Gross <jgross@xxxxxxxx>
>---
> xen/arch/x86/microcode.c        | 12 ++++++++++--
> xen/arch/x86/microcode_amd.c    | 14 ++++++++++----
> xen/arch/x86/microcode_intel.c  | 12 +++++++++---
> xen/include/asm-x86/microcode.h |  3 ++-
> 4 files changed, 31 insertions(+), 10 deletions(-)
>
>diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>index 65d1f41e7c..dcd2c3ff77 100644
>--- a/xen/arch/x86/microcode.c
>+++ b/xen/arch/x86/microcode.c
>@@ -266,10 +266,16 @@ static const struct microcode_patch *nmi_patch = 
>ZERO_BLOCK_PTR;
>  */
> static struct microcode_patch *parse_blob(const char *buf, size_t len)
> {
>+    struct microcode_patch *ret = NULL;
>+
>     if ( likely(!microcode_ops->collect_cpu_info(&this_cpu(cpu_sig))) )
>-        return microcode_ops->cpu_request_microcode(buf, len);
>+    {
>+        spin_lock(&microcode_mutex);
>+        ret = microcode_ops->cpu_request_microcode(buf, len, microcode_cache);
>+        spin_unlock(&microcode_mutex);
>+    }
> 
>-    return NULL;
>+    return ret;
> }
> 
> void microcode_free_patch(struct microcode_patch *microcode_patch)
>@@ -641,6 +647,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;
>     }

... after this if(). Then you needn't modify any vendor-specific code.

Thanks
Chao

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