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

[Chao Gao <chao.gao@intel.com>]

On Tue, Jan 29, 2019 at 03:41:09AM -0700, Jan Beulich wrote:
>>>> Chao Gao <chao.gao@intel.com> 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.

It seems we don't need it. For CPU hot-plug, we would also meet the same
issue you described. To resolve it, a ucode patch is added to the global
cache without checking against the revision on current CPU even if the
signature is matched.

>
>>+    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.
>

Will do.

>
>>--- 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.

It will be used by AMD code. Will mention this in commit message.

Thanks
Chao

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