From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751504AbeAPXDF (ORCPT + 1 other); Tue, 16 Jan 2018 18:03:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51190 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750750AbeAPXDE (ORCPT ); Tue, 16 Jan 2018 18:03:04 -0500 Date: Tue, 16 Jan 2018 17:02:56 -0600 From: Josh Poimboeuf To: Peter Zijlstra 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: <20180116230256.7uhpc3bzfj7vivmr@treble> References: <20180116142825.376986833@infradead.org> <20180116143241.236771093@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180116143241.236771093@infradead.org> User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 16 Jan 2018 23:02:59 +0000 (UTC) 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 03:28:31PM +0100, Peter Zijlstra wrote: > Unlike the jump_label bits, static_cpu_has is implemented with > alternatives. We use the new type field to distinguish them from any > other alternatives > > Like jump_labels, make static_cpu_has set static_jump_dest on the > instructions after the static branch such that we can assert on it. > > Cc: Borislav Petkov > Cc: Thomas Gleixner > Cc: Josh Poimboeuf > Signed-off-by: Peter Zijlstra (Intel) > --- > tools/objtool/check.c | 21 +++++++++++++++++++++ > tools/objtool/special.c | 11 +++++++++++ > tools/objtool/special.h | 1 + > 3 files changed, 33 insertions(+) > > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -636,6 +636,12 @@ static int handle_group_alt(struct objto > fake_jump->ignore = true; > > if (!special_alt->new_len) { > + /* > + * The NOP case for _static_cpu_has() > + */ > + if (special_alt->static_feat) s/static_feat/static_cpu_has/ ? > + fake_jump->jump_dest->static_jump_dest = true; > + > *new_insn = fake_jump; > return 0; > } > @@ -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. 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? > + 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. > + /* > + * The JMP+disp case for _static_cpu_has() > + */ > + insn->jump_dest->static_jump_dest = true; > + } > } > > if (!last_new_insn) { > --- 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). > + > +#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? > unsigned short feature; > + unsigned char type; > > feature = *(unsigned short *)(sec->data->d_buf + offset + > entry->feature); > > + type = *(unsigned char *)(sec->data->d_buf + offset + ALT_TYPE_OFFSET); > + > /* > * It has been requested that we don't validate the !POPCNT > * feature path which is a "very very small percentage of > @@ -110,6 +118,9 @@ static int get_alt_entry(struct elf *elf > */ > if (feature == X86_FEATURE_POPCNT) > alt->skip_orig = true; > + > + if (type == ALT_TYPE_STATIC_CPU_HAS) > + alt->static_feat = true; > } > > orig_rela = find_rela_by_dest(sec, offset + entry->orig); > --- a/tools/objtool/special.h > +++ b/tools/objtool/special.h > @@ -27,6 +27,7 @@ struct special_alt { > bool group; > bool skip_orig; > bool jump_or_nop; > + bool static_feat; > > struct section *orig_sec; > unsigned long orig_off; > > -- Josh