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

[Jan Beulich <jbeulich@xxxxxxxx>]

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