[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On Tue, Jan 29, 2019 at 03:41:09AM -0700, Jan Beulich wrote:
>>>> 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.

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

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


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.