Messages in this thread | | | Date | Thu, 5 Jan 2023 12:00:14 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH -v2] x86/alternatives: Add alt_instr.flags |
| |
* Borislav Petkov <bp@alien8.de> 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@alien8.de> > > 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@infradead.org> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > --- > 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@kernel.org>
I minimally boot-tested it as well.
Thanks,
Ingo
| |