Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755007AbbDJLOq (ORCPT ); Fri, 10 Apr 2015 07:14:46 -0400 Received: from mail.skyhub.de ([78.46.96.112]:37276 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751554AbbDJLOd (ORCPT ); Fri, 10 Apr 2015 07:14:33 -0400 Date: Fri, 10 Apr 2015 13:12:18 +0200 From: Borislav Petkov To: Quentin Casasnovas Cc: X86 ML , LKML Subject: Re: [PATCH 07/13] x86/microcode/intel: Rename update_match_revision() Message-ID: <20150410111218.GC28074@pd.tnic> References: <1424774232-5981-1-git-send-email-bp@alien8.de> <1424774232-5981-8-git-send-email-bp@alien8.de> <20150224162318.GG4565@chrystal.uk.oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150224162318.GG4565@chrystal.uk.oracle.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4697 Lines: 156 On Tue, Feb 24, 2015 at 05:23:18PM +0100, Quentin Casasnovas wrote: > Minor nit-pick, if you reverse your inequality, you don't need for the > ternary operator. Yeah, so I started looking at that and it seems the rabbit hole goes deeper. Let's look at the call to revision_is_newer() in _save_mc(): save_mc: new_rev = mc_hdr->rev; ... if (!revision_is_newer(mc_hdr, new_rev)) -> if (!((mc_hdr->rev <= new_rev) ? 0 : 1)) -> if (!((mc_hdr->rev <= mc_hdr->rev) ? 0 : 1)) -> if (!0) -> if (1) continue; So basically @new_rev was wrong to use there in the first place. And it is there since it got committed in 3.13. If anything, it should've been old_rev FAIK, or if (!revision_is_newer(mc_saved_hdr, new_rev)) ... whateva... And to confirm this and so I can stop rubbing my eyes, let's look at the asm: * * Returns: The updated number @num_saved of saved microcode patches. */ static unsigned int _save_mc(struct microcode_intel **mc_saved, u8 *ucode_ptr, unsigned int num_saved) { ffffffff81033f25: 4c 89 65 e0 mov %r12,-0x20(%rbp) ffffffff81033f29: 4c 89 6d e8 mov %r13,-0x18(%rbp) ffffffff81033f2d: 49 89 f4 mov %rsi,%r12 ucode_ptr lands in %r12 ... new_rev = mc_hdr->rev; ffffffff81033f4f: 45 8b 74 24 04 mov 0x4(%r12),%r14d new_rev is the second unsigned int in the struct thus new_rev = %r14d = *(%r12 + 4) ... if (!revision_is_newer(mc_hdr, new_rev)) ffffffff81033f70: 45 3b 74 24 04 cmp 0x4(%r12),%r14d ffffffff81033f75: 73 21 jae ffffffff81033f98 <_save_mc+0x88> So we practically end up doing cmpl 0x4(%r12), 0x4(%r12) and gcc doesn't optimize it away even. Oh well, let's kill this function completely: --- From: Borislav Petkov Date: Fri, 10 Apr 2015 12:50:57 +0200 Subject: [PATCH] x86/microcode/intel: Get rid of revision_is_newer() It is a one-liner for checking microcode header revisions. On top of that, it can be used wrong as it was the case in _save_mc(). Get rid of it. Signed-off-by: Borislav Petkov --- arch/x86/include/asm/microcode_intel.h | 6 ------ arch/x86/kernel/cpu/microcode/intel_early.c | 2 +- arch/x86/kernel/cpu/microcode/intel_lib.c | 6 +++--- 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h index 2b9209c46ca9..a4df6d292228 100644 --- a/arch/x86/include/asm/microcode_intel.h +++ b/arch/x86/include/asm/microcode_intel.h @@ -60,12 +60,6 @@ extern int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc) extern int microcode_sanity_check(void *mc, int print_err); extern int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc); -static inline int -revision_is_newer(struct microcode_header_intel *mc_header, int rev) -{ - return (mc_header->rev <= rev) ? 0 : 1; -} - #ifdef CONFIG_MICROCODE_INTEL_EARLY extern void __init load_ucode_intel_bsp(void); extern void load_ucode_intel_ap(void); diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c index 98d320c25dff..edae46ebdf32 100644 --- a/arch/x86/kernel/cpu/microcode/intel_early.c +++ b/arch/x86/kernel/cpu/microcode/intel_early.c @@ -262,7 +262,7 @@ static unsigned int _save_mc(struct microcode_intel **mc_saved, found = 1; - if (!revision_is_newer(mc_hdr, new_rev)) + if (mc_hdr->rev <= mc_saved_hdr->rev) continue; /* diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c index cd47a510a3f1..63b0a2e059ee 100644 --- a/arch/x86/kernel/cpu/microcode/intel_lib.c +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c @@ -154,13 +154,13 @@ int get_matching_sig(unsigned int csig, int cpf, int rev, void *mc) /* * Returns 1 if update has been found, 0 otherwise. */ -int get_matching_microcode(unsigned int csig, int cpf, int rev, void *mc) +int get_matching_microcode(unsigned int csig, int cpf, int new_rev, void *mc) { struct microcode_header_intel *mc_hdr = mc; - if (!revision_is_newer(mc_hdr, rev)) + if (mc_hdr->rev <= new_rev) return 0; - return get_matching_sig(csig, cpf, rev, mc); + return get_matching_sig(csig, cpf, new_rev, mc); } EXPORT_SYMBOL_GPL(get_matching_microcode); -- 2.3.5 -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/