lkml.org 
[lkml]   [2018]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2 06/10] objtool: Implement jump_assert for _static_cpu_has()
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 <bp@alien8.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> 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

\
 
 \ /
  Last update: 2018-01-17 00:03    [W:1.008 / U:0.632 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site