Re: [Xen-devel] [PATCH v5 2/8] microcode/intel: extend microcode_update_match()

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Chao Gao <chao.gao@xxxxxxxxx> 01/28/19 8:10 AM >>>
>--- a/xen/arch/x86/microcode_intel.c
>+++ b/xen/arch/x86/microcode_intel.c
>@@ -127,14 +127,24 @@ static int collect_cpu_info(unsigned int cpu_num, struct 
>cpu_signature *csig)
>return 0;
>}
 >
>-static inline int microcode_update_match(
>-    unsigned int cpu_num, const struct microcode_header_intel *mc_header,
>-    int sig, int pf)
>+static enum microcode_match_result microcode_update_match(
>+    const void *mc, unsigned int sig, unsigned int pf, unsigned int rev)
>{
>-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>+    const struct microcode_header_intel *mc_header = mc;
>+    const struct extended_sigtable *ext_header;
>+    const struct extended_signature *ext_sig;
>+    unsigned int i;
>+
>+    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>+        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
 
You may want a tristate return here: I know there are systems where
firmware updates ucode only on core 0 of every socket, in which case we'd
very much like to apply the same microcode on the other cores in case we
find the blob matching what is currently installed. IOW depending how later
patches actually work, you may also want a SAME_UCODE return case.

>+    ext_header = mc + get_datasize(mc_header) + MC_HEADER_SIZE;

On top of what Roger has said, isn't mc + MC_HEADER_SIZE the same
as mc_header + 1?

>+    ext_sig = (const void *)ext_header + EXT_HEADER_SIZE;

And (const void *)ext_header + EXT_HEADER_SIZE the same as
(const void *)(ext_header + 1)?

In both cases this would eliminate unnecessary implications of certain
two sub-terms to refer to the same types, i.e. also make the casts less
scary / dangerous.


>--- a/xen/include/asm-x86/microcode.h
>+++ b/xen/include/asm-x86/microcode.h
>@@ -3,6 +3,12 @@
 >
>#include <xen/percpu.h>
 >
>+enum microcode_match_result {
>+    OLD_UCODE, /* signature matched, but revision id isn't newer */
>+    NEW_UCODE, /* signature matched, but revision id is newer */
>+    MIS_UCODE, /* signature mismatched */
>+};

It's not clear at this point of the series or from the commit message whether
this is to be used by AMD code as well. If not, it would better move into
microcode_intel.c.

Jan



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