Re: [PATCH -v2] x86/alternatives: Add alt_instr.flags

[Ingo Molnar]

* Borislav Petkov <bp@xxxxxxxxx> wrote:

> Yeah,
> 
> PeterZ had a much better idea for doing the split and hacking it in, it
> turned out to be the cleanest and straightforwardestest eva.
> 
> So let's do it.
> 
> ---
> From: "Borislav Petkov (AMD)" <bp@xxxxxxxxx>
> 
> Add a struct alt_instr.flags field which will contain different flags
> controlling alternatives patching behavior.
> 
> The initial idea was to be able to specify it as a separate macro
> parameter but that would mean touching all possible invocations of the
> alternatives macros and thus a lot of churn.
> 
> What is more, as PeterZ suggested, being able to say ALT_NOT(feature) is
> very readable and explains exactly what is meant.
> 
> So make the feature field a u32 where the patching flags are the upper
> u16 part of the dword quantity while the lower u16 word is the feature.
> 
> The highest feature number currently is 0x26a (i.e., word 19) so we have
> plenty of space. If that becomes insufficient, the field can be extended
> to u64 which will then make struct alt_instr of the nice size of 16
> bytes (14 bytes currently).
> 
> There should be no functional changes resulting from this.
> 
> Suggested-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Signed-off-by: Borislav Petkov (AMD) <bp@xxxxxxxxx>
> ---
>  arch/x86/include/asm/alternative.h            | 132 ++++++++++--------
>  arch/x86/kernel/alternative.c                 |  14 +-
>  tools/objtool/arch/x86/include/arch/special.h |   6 +-
>  3 files changed, 85 insertions(+), 67 deletions(-)
> 
> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
> index 7659217f4d49..ad17cda8e5d2 100644
> --- a/arch/x86/include/asm/alternative.h
> +++ b/arch/x86/include/asm/alternative.h
> @@ -6,8 +6,10 @@
>  #include <linux/stringify.h>
>  #include <asm/asm.h>
>  
> -#define ALTINSTR_FLAG_INV	(1 << 15)
> -#define ALT_NOT(feat)		((feat) | ALTINSTR_FLAG_INV)
> +#define ALT_FLAGS_SHIFT		16
> +
> +#define ALT_FLAG_NOT		BIT(0)
> +#define ALT_NOT(feature)	((ALT_FLAG_NOT << ALT_FLAGS_SHIFT) | (feature))
>  
>  #ifndef __ASSEMBLY__
>  
> @@ -59,10 +61,27 @@
>  	".long 999b - .\n\t"					\
>  	".popsection\n\t"
>  
> +/*
> + * The patching flags are part of the upper bits of the @ft_flgs parameter when
> + * specifying them. The split is currently like this:
> + *
> + * [31... flags ...16][15... CPUID feature bit ...0]
> + *
> + * but since this is all hidden in the macros argument being split, those fields can be
> + * extended in the future to fit in a u64 or however the need arises.
> + */
>  struct alt_instr {
>  	s32 instr_offset;	/* original instruction */
>  	s32 repl_offset;	/* offset to replacement instruction */
> -	u16 cpuid;		/* cpuid bit set for replacement */
> +
> +	union {
> +		struct {
> +			u32 cpuid: 16;	/* CPUID bit set for replacement */
> +			u32 flags: 16;	/* patching control flags */
> +		};
> +		u32 ft_flgs;
> +	};

Neat - my only nitpick would be s/ft_flgs/ft_flags - it's more readable and 
we haven't run out of a's yet I guess. ;-)

   Reviewed-by: Ingo Molnar <mingo@xxxxxxxxxx>

I minimally boot-tested it as well.

Thanks,

	Ingo