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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.02.2022 18:39, Andrew Cooper wrote:
> On 24/02/2022 11:25, Jan Beulich wrote:
>> On 13.01.2022 14:17, 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. To also cover the "tsc" case, invoke
>>> what read_tsc() resolves to directly. In turn read_tsc() then becomes
>>> unreachable and hence can move to .init.*.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> As this actually amends the IBT work, I would have hoped for it to be
>> viewed as useful.
> 
> Sorry - it fell through the cracks.  Definitely useful.
> 
>>  Of course if accepted in general, it would now want
>> to have __initconst_cf_clobber annotation addition included. Albeit
>> there's a slight complication: Some of the structures are written to,
>> so those couldn't really be "const".
> 
> The .init.cf_clobber section needs to container a pointer to every
> target function.  For the current ops structures, we just put the whole
> ops structure in.
> 
> For individual functions, the best plan I could come up with was a macro
> which emits:
> 
> .pushsection .init.cf_clobber, a, @progbits
> .quad fn
> .popsection
> 
> wrapped up in #define cf_clobber(fn), so the end code result ought to
> look like:
> 
> static void foo(param *bar)
> {
>     ...
> }
> cf_clobber(foo);
> 
> similar to command line parameters.
> 
> 
> That said, in this case, can't we cf_clobber each platform_timesource ? 
> It would require altcall()ing the resume hook too.  (the init hook won't
> matter either way.)

The resume hook is altcall()ed by the patch already. The problem isn't
that the annotation would be wrong when placed on all ops structs, but
that a mix of const and non-const isn't okay. Some of the structs can
be made const (plt_pit, plt_xen_timer, and plt_hyperv_timer),
but others cannot be (plt_hpet, plt_pmtimer, and plt_tsc). (There's
also going to be fallout from making some const, as then the .init
hook needs to change so its parameter is pointer to const, which in
turn requires updates to plt_hpet and plt_pmtimer to be no longer be
made through the function's parameter, but that's merely a cosmetic
and patch size aspect.)

As said in reply to Roger, I think I'd prefer to do this 2nd
transformation in a separate patch anyway, putting in the one here as
is (merely mechanically rebased over the cf_check additions). The two
steps are technically separable, and with the other adjustments needed
it seems better to keep them separate. (And really I'm about to put in
the patch here.)

As to the init hook not mattering: verify_tsc_reliability() is an
initcall, which happens after AP bringup and hence after the 2nd
alternatives patching pass. Therefore plt_tsc cannot be annotated. But
that's fine: It has no resume hook and the plan is to do away with
read_tsc() as a real function.

Jan