On Wed, Nov 28, 2018 at 11:58:06AM +0100, Roger Pau Monné wrote: >On Wed, Nov 28, 2018 at 01:34:11PM +0800, Chao Gao wrote: >> to a more generic function. The benefit is that this function can be >> used to check whether a microcode is newer than another as well. We >> rely on this function to decide to perform a replacement or an add when >> updating the global microcode cache (introduced by later patches in >> this series). >> >> Signed-off-by: Chao Gao <chao....@intel.com> >> --- >> xen/arch/x86/microcode_intel.c | 57 >> +++++++++++++++++++++++------------------- >> 1 file changed, 31 insertions(+), 26 deletions(-) >> >> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c >> index 9657575..8d9a3b2 100644 >> --- a/xen/arch/x86/microcode_intel.c >> +++ b/xen/arch/x86/microcode_intel.c >> @@ -127,14 +127,37 @@ 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) >> +enum { >> + OLD_UCODE, /* signature matched, but revision id isn't newer */ >> + NEW_UCODE, /* signature matched, but revision id is newer */ >> + MIS_UCODE, /* signature mismatched */ >> +}; > >Shouldn't you give a name to this type ... > >> +static int microcode_update_match(const void *mc, > >... so that this function can return it instead of int? > >> + 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; >> + unsigned long total_size = get_totalsize(mc_header); > >size_t might be more appropriate here. > >> + int ext_sigcount, i; > >unsigned int. > >> + struct extended_signature *ext_sig; > >const? > >> >> - return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) && >> - (mc_header->rev > uci->cpu_sig.rev)); >> + if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) ) >> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE; >> + >> + if ( total_size <= (get_datasize(mc_header) + MC_HEADER_SIZE) ) >> + return MIS_UCODE; > >Shouldn't you perform this check before the signature check?
I think this check shouldn't be here. Given that this check is also done in microcode_sanity_check(), I will remove this check here. > >> + >> + ext_header = mc + get_datasize(mc_header) + MC_HEADER_SIZE; >> + ext_sigcount = ext_header->count; >> + ext_sig = (void *)ext_header + EXT_HEADER_SIZE; > >You are dropping the const here AFAICT by casting to void *. > >> + for ( i = 0; i < ext_sigcount; i++ ) >> + { >> + if ( sigmatch(sig, ext_sig->sig, pf, ext_sig->pf) ) >> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE; >> + ext_sig++; >> + } > >I would add a newline here for readability. > >> + return MIS_UCODE; >> } >> >> static int microcode_sanity_check(void *mc) >> @@ -236,31 +259,13 @@ static int get_matching_microcode(const void *mc, >> unsigned int cpu) >> { >> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu); >> const struct microcode_header_intel *mc_header = mc; >> - const struct extended_sigtable *ext_header; >> unsigned long total_size = get_totalsize(mc_header); >> - int ext_sigcount, i; >> - struct extended_signature *ext_sig; >> void *new_mc; >> >> - if ( microcode_update_match(cpu, mc_header, >> - mc_header->sig, mc_header->pf) ) >> - goto find; >> - >> - if ( total_size <= (get_datasize(mc_header) + MC_HEADER_SIZE) ) >> + if ( microcode_update_match(mc, uci->cpu_sig.sig, uci->cpu_sig.pf, >> + uci->cpu_sig.rev) != NEW_UCODE ) >> return 0; > >Shouldn't you differentiate between the function returning OLD_UCODE >or MIS_UCODE? I would expect that trying to load a mismatched UCODE >would trigger some kind of message from Xen. I don't differentiate these two cases. For both of them, we do nothing. Actually, I add a message "No newer or matched microcode found" in patch 4 for them (Currently each cpu parses the file locally, if we add an error message, it will show up many times). However, if you are to load a corrupted file, another error will be prompted. Other comments are fine with me. Thanks Chao _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel