From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752324AbeAQJUD (ORCPT + 1 other); Wed, 17 Jan 2018 04:20:03 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:47373 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750888AbeAQJUB (ORCPT ); Wed, 17 Jan 2018 04:20:01 -0500 Date: Wed, 17 Jan 2018 10:19:45 +0100 From: Peter Zijlstra To: Josh Poimboeuf Cc: David Woodhouse , linux-kernel@vger.kernel.org, Dave Hansen , Ashok Raj , Thomas Gleixner , Tim Chen , Andy Lutomirski , Linus Torvalds , Greg KH , Andrea Arcangeli , Andi Kleen , Arjan Van De Ven , Dan Williams , Paolo Bonzini , Jun Nakajima , Asit Mallick , Borislav Petkov Subject: Re: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has() Message-ID: <20180117091945.GL2228@hirez.programming.kicks-ass.net> References: <20180116142825.376986833@infradead.org> <20180116143241.236771093@infradead.org> <20180116230256.7uhpc3bzfj7vivmr@treble> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180116230256.7uhpc3bzfj7vivmr@treble> User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: On Tue, Jan 16, 2018 at 05:02:56PM -0600, Josh Poimboeuf wrote: > On Tue, Jan 16, 2018 at 03:28:31PM +0100, Peter Zijlstra wrote: > > + if (special_alt->static_feat) > > s/static_feat/static_cpu_has/ ? done. > > @@ -664,6 +670,21 @@ static int handle_group_alt(struct objto > > insn->sec, insn->offset); > > return -1; > > } > > + > > + if (special_alt->static_feat) { > > + if (insn->type != INSN_JUMP_UNCONDITIONAL) { > > + WARN_FUNC("not an unconditional jump in _static_cpu_has()", > > + insn->sec, insn->offset); > > + } > > So I think this is trying to assert the fact that you're only expecting > a single instruction which is an unconditional jump. Correct. > We already weeded > out non-jumps earlier in the loop, so would it make sense to do this > check before the INSN_JUMP_CONDITIONAL/INSN_JUMP_UNCONDITIONAL check a > little higher up? This way we keep all the special_alt->static_cpu_has bits together. > > + if (insn->jump_dest == fake_jump) { > > + WARN_FUNC("jump inside alternative for _static_cpu_has()", > > + insn->sec, insn->offset); > > + } > > The error message doesn't seem to match the condition, so I'm not sure > which one you're trying to check, or why. > > IIRC, 'insn->jump_dest == fake_jump' means we reached the end of the > alternative code block without hitting a jump. > > But based on the loop exit condition, I don't think it's ever possible > for insn->jump_dest to ever point to the fake_jump at the end. Oof, now what was I thinking again.. So that fake_jump is inserted at the end of the alternative and jumps to the code after where the alternative will be patched in to simulate the code flow. If there is a jump inside the alternative that jumps to the end, it's destination will be set to the fake jump, we have this clause for that: dest_off = insn->offset + insn->len + insn->immediate; if (dest_off == special_alt->new_off + special_alt->new_len) insn->jump_dest = fake_jump; if that happens for static_cpu_has(), bad things happened. So the only way for a jump to have fake_jump as destination is if the jump is inside the alternative (but to the end) and we must assert this didn't happen. Unlikely, yes, but I figured we want to know about it if it ever does happen. > > --- a/tools/objtool/special.c > > +++ b/tools/objtool/special.c > > @@ -40,6 +40,11 @@ > > #define ALT_FEATURE_OFFSET 8 > > #define ALT_ORIG_LEN_OFFSET 10 > > #define ALT_NEW_LEN_OFFSET 11 > > +#define ALT_PADDING_OFFSET 12 > > +#define ALT_TYPE_OFFSET 13 > > We don't need the ALT_PADDING_OFFSET define (notice we have gaps > already, there are only defines for the ones we use). Over zealous I suppose, I've taken it out again. > > + > > +#define ALT_TYPE_DEFAULT 0 > > +#define ALT_TYPE_STATIC_CPU_HAS 1 > > > > #define X86_FEATURE_POPCNT (4*32+23) > > > > @@ -99,10 +104,13 @@ static int get_alt_entry(struct elf *elf > > > > if (entry->feature) { > > Since this block now uses more than entry->feature, and is now just a > general alternatives thing, can you change it to > > if (entry->feature == ALT_FEATURE_OFFSET) > > so it's more clear and slightly more future proof? I've made it entry->group instead. How about the below on top? --- Subject: objtool: Introduce special_type From: Peter Zijlstra Date: Wed Jan 17 10:16:44 CET 2018 Use an enum for the special_alt entries instead of a collection of booleans. Signed-off-by: Peter Zijlstra (Intel) --- tools/objtool/check.c | 11 ++++++++--- tools/objtool/special.c | 21 +++++++++------------ tools/objtool/special.h | 10 ++++++++-- 3 files changed, 25 insertions(+), 17 deletions(-) --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -773,7 +773,7 @@ static int add_special_section_alts(stru continue; new_insn = NULL; - if (!special_alt->group || special_alt->new_len) { + if (special_alt->type != alternative || special_alt->new_len) { new_insn = find_insn(file, special_alt->new_sec, special_alt->new_off); if (!new_insn) { @@ -785,16 +785,21 @@ static int add_special_section_alts(stru } } - if (special_alt->group) { + switch(special_alt->type) { + case alternative: ret = handle_group_alt(file, special_alt, orig_insn, &new_insn); if (ret) goto out; - } else if (special_alt->jump_or_nop) { + break; + + case jump_label: ret = handle_jump_alt(file, special_alt, orig_insn, &new_insn); if (ret) goto out; + + break; } alt = malloc(sizeof(*alt)); --- a/tools/objtool/special.c +++ b/tools/objtool/special.c @@ -49,7 +49,7 @@ struct special_entry { const char *sec; - bool group, jump_or_nop; + enum special_type type; unsigned char size, orig, new; unsigned char orig_len, new_len; /* group only */ unsigned char feature; /* ALTERNATIVE macro CPU feature */ @@ -58,7 +58,7 @@ struct special_entry { struct special_entry entries[] = { { .sec = ".altinstructions", - .group = true, + .type = alternative, .size = ALT_ENTRY_SIZE, .orig = ALT_ORIG_OFFSET, .orig_len = ALT_ORIG_LEN_OFFSET, @@ -68,13 +68,14 @@ struct special_entry entries[] = { }, { .sec = "__jump_table", - .jump_or_nop = true, + .type = jump_label, .size = JUMP_ENTRY_SIZE, .orig = JUMP_ORIG_OFFSET, .new = JUMP_NEW_OFFSET, }, { .sec = "__ex_table", + .type = exception, .size = EX_ENTRY_SIZE, .orig = EX_ORIG_OFFSET, .new = EX_NEW_OFFSET, @@ -92,18 +93,14 @@ static int get_alt_entry(struct elf *elf offset = idx * entry->size; data = sec->data->d_buf + offset; - alt->group = entry->group; - alt->jump_or_nop = entry->jump_or_nop; + alt->type = entry->type; - if (alt->group) { - alt->orig_len = *(unsigned char *)(data + entry->orig_len); - alt->new_len = *(unsigned char *)(data + entry->new_len); - } - - if (entry->group) { + if (alt->type == alternative) { unsigned short feature; unsigned char type; + alt->orig_len = *(unsigned char *)(data + entry->orig_len); + alt->new_len = *(unsigned char *)(data + entry->new_len); feature = *(unsigned short *)(data + ALT_FEATURE_OFFSET); type = *(unsigned char *)(data + ALT_TYPE_OFFSET); @@ -133,7 +130,7 @@ static int get_alt_entry(struct elf *elf alt->orig_sec = orig_rela->sym->sec; alt->orig_off = orig_rela->addend; - if (!entry->group || alt->new_len) { + if (entry->type != alternative || alt->new_len) { new_rela = find_rela_by_dest(sec, offset + entry->new); if (!new_rela) { WARN_FUNC("can't find new rela", --- a/tools/objtool/special.h +++ b/tools/objtool/special.h @@ -21,12 +21,18 @@ #include #include "elf.h" +enum special_type { + alternative, + jump_label, + exception, +}; + struct special_alt { struct list_head list; - bool group; + enum special_type type; + bool skip_orig; - bool jump_or_nop; bool static_cpu_has; struct section *orig_sec;