Re: [PATCH] x86/time: switch platform timer hooks to altcall

[Andrew Cooper <amc96@xxxxxxxx>]

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.


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 suspect it will be easier to figure out how to make the TSC case also
invariant after boot.

~Andrew