lkml.org 
[lkml]   [2023]   [Nov]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 4/5] x86/paravirt: switch mixed paravirt/alternative calls to alternative_2
From
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]
\
 
 \ /
  Last update: 2023-11-29 11:09    [W:0.137 / U:0.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site