Messages in this thread | | | Date | Wed, 29 Nov 2023 11:08:14 +0100 | Subject | Re: [PATCH v4 4/5] x86/paravirt: switch mixed paravirt/alternative calls to alternative_2 | From | Juergen Gross <> |
| |
On 21.11.23 19:45, Borislav Petkov wrote: > On Mon, Oct 30, 2023 at 03:25:07PM +0100, Juergen Gross wrote: >> Instead of stacking alternative and paravirt patching, use the new >> ALT_FLAG_CALL flag to switch those mixed calls to pure alternative >> handling. >> >> This eliminates the need to be careful regarding the sequence of >> alternative and paravirt patching. >> >> For call depth tracking callthunks_setup() needs to be adapted to patch >> calls at alternative patching sites instead of paravirt calls. > > Why is this important so that it is called out explicitly in the commit > message? Is callthunks_setup() special somehow?
IMHO it is a non-obvious change, so I spelled it out explicitly. I can drop that paragraph if you want.
> >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> >> --- >> arch/x86/include/asm/alternative.h | 5 +++-- >> arch/x86/include/asm/paravirt.h | 9 ++++++--- >> arch/x86/include/asm/paravirt_types.h | 26 +++++++++----------------- >> arch/x86/kernel/callthunks.c | 17 ++++++++--------- >> arch/x86/kernel/module.c | 20 +++++--------------- >> 5 files changed, 31 insertions(+), 46 deletions(-) >> >> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h >> index 2a74a94bd569..07b17bc615a0 100644 >> --- a/arch/x86/include/asm/alternative.h >> +++ b/arch/x86/include/asm/alternative.h >> @@ -89,6 +89,8 @@ struct alt_instr { >> u8 replacementlen; /* length of new instruction */ >> } __packed; >> >> +extern struct alt_instr __alt_instructions[], __alt_instructions_end[]; >> + > > arch/x86/include/asm/alternative.h:92:extern struct alt_instr __alt_instructions[], __alt_instructions_end[]; > arch/x86/kernel/alternative.c:163:extern struct alt_instr __alt_instructions[], __alt_instructions_end[]; > > Zap the declaration from the .c file pls.
Okay.
> >> /* >> * Debug flag that can be tested to see whether alternative >> * instructions were patched in already: >> @@ -104,11 +106,10 @@ extern void apply_fineibt(s32 *start_retpoline, s32 *end_retpoine, >> s32 *start_cfi, s32 *end_cfi); >> >> struct module; >> -struct paravirt_patch_site; >> >> struct callthunk_sites { >> s32 *call_start, *call_end; >> - struct paravirt_patch_site *pv_start, *pv_end; >> + struct alt_instr *alt_start, *alt_end; >> }; >> >> #ifdef CONFIG_CALL_THUNKS >> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h >> index 3749311d51c3..9c6c5cfa9fe2 100644 >> --- a/arch/x86/include/asm/paravirt.h >> +++ b/arch/x86/include/asm/paravirt.h >> @@ -740,20 +740,23 @@ void native_pv_lock_init(void) __init; >> >> #ifdef CONFIG_X86_64 >> #ifdef CONFIG_PARAVIRT_XXL >> +#ifdef CONFIG_DEBUG_ENTRY >> >> #define PARA_PATCH(off) ((off) / 8) >> #define PARA_SITE(ptype, ops) _PVSITE(ptype, ops, .quad, 8) >> #define PARA_INDIRECT(addr) *addr(%rip) >> >> -#ifdef CONFIG_DEBUG_ENTRY >> .macro PARA_IRQ_save_fl >> PARA_SITE(PARA_PATCH(PV_IRQ_save_fl), >> ANNOTATE_RETPOLINE_SAFE; >> call PARA_INDIRECT(pv_ops+PV_IRQ_save_fl);) >> + ANNOTATE_RETPOLINE_SAFE; >> + call PARA_INDIRECT(pv_ops+PV_IRQ_save_fl); >> .endm >> >> -#define SAVE_FLAGS ALTERNATIVE "PARA_IRQ_save_fl;", "pushf; pop %rax;", \ >> - ALT_NOT_XEN >> +#define SAVE_FLAGS ALTERNATIVE_2 "PARA_IRQ_save_fl;", \ >> + ALT_CALL_INSTR, ALT_CALL_ALWAYS, \ >> + "pushf; pop %rax;", ALT_NOT_XEN > > How is that supposed to work? > > At build time for a PARAVIRT_XXL build it'll have that PARA_IRQ_save_fl > macro in there which issues a .parainstructions section and an indirect > call to > > call *pv_ops+240(%rip); > > then it'll always patch in "call BUG_func" due to X86_FEATURE_ALWAYS. > > I guess this is your way of saying "this should always be patched, one > way or the other, depending on X86_FEATURE_XENPV, and this is a way to > catch unpatched locations... > > Then on a pv build which doesn't set X86_FEATURE_XENPV during boot, > it'll replace the "call BUG_func" thing with the pushf;pop. > > And if it does set X86_FEATURE_XENPV, it'll patch in the direct call to > .... /me greps tree ... pv_native_save_fl I guess. > > If anything, how those ALT_CALL_ALWAYS things are supposed to work, > should be documented there, over the macro definition and what the > intent is.
I can do that, but OTOH the existing comments are quite clear:
* If CPU has feature2, newinstr2 is used. * Otherwise, if CPU has feature1, newinstr1 is used. * Otherwise, oldinstr is used.
> > Because otherwise we'll have to swap in the whole machinery back into > our L1s each time we need to touch it. > > And btw, this whole patching stuff becomes insanely non-trivial slowly.
Not worse than today. It just replaces the paravirt patching with an alternative patching.
> > :-\ > >> diff --git a/arch/x86/kernel/callthunks.c b/arch/x86/kernel/callthunks.c >> index faa9f2299848..200ea8087ddb 100644 >> --- a/arch/x86/kernel/callthunks.c >> +++ b/arch/x86/kernel/callthunks.c >> @@ -238,14 +238,13 @@ patch_call_sites(s32 *start, s32 *end, const struct core_text *ct) >> } >> >> static __init_or_module void >> -patch_paravirt_call_sites(struct paravirt_patch_site *start, >> - struct paravirt_patch_site *end, >> - const struct core_text *ct) >> +patch_alt_call_sites(struct alt_instr *start, struct alt_instr *end, >> + const struct core_text *ct) >> { >> - struct paravirt_patch_site *p; >> + struct alt_instr *a; >> >> - for (p = start; p < end; p++) >> - patch_call(p->instr, ct); >> + for (a = start; a < end; a++) >> + patch_call((u8 *)&a->instr_offset + a->instr_offset, ct); > > tip:x86/paravirt has: > > 5c22c4726e4a ("x86/paravirt: Use relative reference for the original instruction offset") > > Perhaps redo yours ontop of tip/master:
Yes, of course.
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |