[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/time: switch platform timer hooks to altcall
On 12.01.2022 10:17, Andrew Cooper wrote: > On 12/01/2022 08:58, Jan Beulich wrote: >> Except in the "clocksource=tsc" case we can replace the indirect calls >> involved in accessing the platform timers by direct ones, as they get >> established once and never changed. >> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> >> --- >> Sort of RFC, for both the whether and the how aspects. >> >> TBD: Overriding X86_FEATURE_ALWAYS is somewhat dangerous; there's only >> no issue with e.g. hvm_set_tsc_offset() used later in time.c >> because that's an inline function which did already "latch" the >> usual value of the macro. But the alternative of introducing an >> alternative_call() variant allowing to specify the controlling >> feature also doesn't look overly nice to me either. Then again the >> .resume hook invocation could be patched unconditionally, as the >> TSC clocksource leaves this hook set to NULL. >> >> --- a/xen/arch/x86/alternative.c >> +++ b/xen/arch/x86/alternative.c >> @@ -268,8 +268,7 @@ static void init_or_livepatch _apply_alt >> * point the branch destination is still NULL, insert "UD2; UD0" >> * (for ease of recognition) instead of CALL/JMP. >> */ >> - if ( a->cpuid == X86_FEATURE_ALWAYS && >> - *(int32_t *)(buf + 1) == -5 && >> + if ( *(int32_t *)(buf + 1) == -5 && > > I'm afraid that this must not become conditional. > > One of the reasons I was hesitant towards the mechanics of altcall in > the first place was that it intentionally broke Spectre v2 protections > by manually writing out a non-retpoline'd indirect call. > > This is made safe in practice because all altcall sites either get > converted to a direct call, or rewritten to be a UD2. Oh, sorry, I really should have realized this. > If you want to make altcalls conversions conditional, then the code gen > must be rearranged to use INDIRECT_CALL first, but that comes with > overheads too because then call callsites would load the function > pointer into a register, just to not use it in the patched case. I don't view this as desirable. > I suspect it will be easier to figure out how to make the TSC case also > invariant after boot. Since switching to "tsc" happens only after bringing up all CPUs, I don't see how this could become possible; in particular I don't fancy an alternatives patching pass with all CPUs online (albeit technically this could be an option). The solution (workaround) I can see for now is using if ( plt_src.read_counter != read_tsc ) count = alternative_call(plt_src.read_counter); else count = rdtsc_ordered(); everywhere. Potentially we'd then want to hide this in a static (inline?) function. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |