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@suse.com>
---
TBD: Instead of adding __init to read_tsc() we could also ditch the
function altogether, using a dedicated (non-canonical) pointer
constant instead for the .read_counter initializer and the two
comparisons done on plt_src.read_counter.
---
v2: Avoid altcall patching becoming conditional.
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -66,6 +66,7 @@ struct platform_timesource {
char *id;
char *name;
u64 frequency;
+ /* This hook may only be invoked via the read_counter() wrapper! */
u64 (*read_counter)(void);
s64 (*init)(struct platform_timesource *);
void (*resume)(struct platform_timesource *);
@@ -578,7 +579,7 @@ static s64 __init init_tsc(struct platfo
return ret;
}
-static u64 read_tsc(void)
+static uint64_t __init read_tsc(void)
{
return rdtsc_ordered();
}
@@ -810,6 +811,18 @@ static s_time_t __read_platform_stime(u6
return (stime_platform_stamp + scale_delta(diff, &plt_scale));
}
+static uint64_t read_counter(void)
+{
+ /*
+ * plt_tsc is put in use only after alternatives patching has occurred,
+ * hence we can't invoke read_tsc() that way. Special case it here, open-
+ * coding the function call at the same time.
+ */
+ return plt_src.read_counter != read_tsc
+ ? alternative_call(plt_src.read_counter)
+ : rdtsc_ordered();
+}
+
static void plt_overflow(void *unused)
{
int i;
@@ -818,7 +831,7 @@ static void plt_overflow(void *unused)
spin_lock_irq(&platform_timer_lock);
- count = plt_src.read_counter();
+ count = read_counter();
plt_stamp64 += (count - plt_stamp) & plt_mask;
plt_stamp = count;
@@ -854,7 +867,7 @@ static s_time_t read_platform_stime(u64
ASSERT(!local_irq_is_enabled());
spin_lock(&platform_timer_lock);
- plt_counter = plt_src.read_counter();
+ plt_counter = read_counter();
count = plt_stamp64 + ((plt_counter - plt_stamp) & plt_mask);
stime = __read_platform_stime(count);
spin_unlock(&platform_timer_lock);
@@ -872,7 +885,7 @@ static void platform_time_calibration(vo
unsigned long flags;
spin_lock_irqsave(&platform_timer_lock, flags);
- count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
+ count = plt_stamp64 + ((read_counter() - plt_stamp) & plt_mask);
stamp = __read_platform_stime(count);
stime_platform_stamp = stamp;
platform_timer_stamp = count;
@@ -883,10 +896,10 @@ static void resume_platform_timer(void)
{
/* Timer source can be reset when backing from S3 to S0 */
if ( plt_src.resume )
- plt_src.resume(&plt_src);
+ alternative_vcall(plt_src.resume, &plt_src);
plt_stamp64 = platform_timer_stamp;
- plt_stamp = plt_src.read_counter();
+ plt_stamp = read_counter();
}
static void __init reset_platform_timer(void)
On Thu, Jan 13, 2022 at 02:17:18PM +0100, 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@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > --- > TBD: Instead of adding __init to read_tsc() we could also ditch the > function altogether, using a dedicated (non-canonical) pointer > constant instead for the .read_counter initializer and the two > comparisons done on plt_src.read_counter. I was going to suggest adding an ASSERT_UNREACHABLE, but not sure it makes much sense if the function is init only. I would be fine with using a non-canonical pointer. Thanks, Roger.
On 24.02.2022 15:09, Roger Pau Monné wrote: > On Thu, Jan 13, 2022 at 02:17:18PM +0100, 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@suse.com> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. >> --- >> TBD: Instead of adding __init to read_tsc() we could also ditch the >> function altogether, using a dedicated (non-canonical) pointer >> constant instead for the .read_counter initializer and the two >> comparisons done on plt_src.read_counter. > > I was going to suggest adding an ASSERT_UNREACHABLE, but not sure it > makes much sense if the function is init only. > > I would be fine with using a non-canonical pointer. I guess I'll put in the patch as is and do the conversion in a follow-up change adding __initconst_cf_clobber (once it's clear what to do about structures which are written to). Jan
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@suse.com>
As this actually amends the IBT work, I would have hoped for it to be
viewed as 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".
Jan
> ---
> v2: Avoid altcall patching becoming conditional.
>
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -66,6 +66,7 @@ struct platform_timesource {
> char *id;
> char *name;
> u64 frequency;
> + /* This hook may only be invoked via the read_counter() wrapper! */
> u64 (*read_counter)(void);
> s64 (*init)(struct platform_timesource *);
> void (*resume)(struct platform_timesource *);
> @@ -578,7 +579,7 @@ static s64 __init init_tsc(struct platfo
> return ret;
> }
>
> -static u64 read_tsc(void)
> +static uint64_t __init read_tsc(void)
> {
> return rdtsc_ordered();
> }
> @@ -810,6 +811,18 @@ static s_time_t __read_platform_stime(u6
> return (stime_platform_stamp + scale_delta(diff, &plt_scale));
> }
>
> +static uint64_t read_counter(void)
> +{
> + /*
> + * plt_tsc is put in use only after alternatives patching has occurred,
> + * hence we can't invoke read_tsc() that way. Special case it here, open-
> + * coding the function call at the same time.
> + */
> + return plt_src.read_counter != read_tsc
> + ? alternative_call(plt_src.read_counter)
> + : rdtsc_ordered();
> +}
> +
> static void plt_overflow(void *unused)
> {
> int i;
> @@ -818,7 +831,7 @@ static void plt_overflow(void *unused)
>
> spin_lock_irq(&platform_timer_lock);
>
> - count = plt_src.read_counter();
> + count = read_counter();
> plt_stamp64 += (count - plt_stamp) & plt_mask;
> plt_stamp = count;
>
> @@ -854,7 +867,7 @@ static s_time_t read_platform_stime(u64
> ASSERT(!local_irq_is_enabled());
>
> spin_lock(&platform_timer_lock);
> - plt_counter = plt_src.read_counter();
> + plt_counter = read_counter();
> count = plt_stamp64 + ((plt_counter - plt_stamp) & plt_mask);
> stime = __read_platform_stime(count);
> spin_unlock(&platform_timer_lock);
> @@ -872,7 +885,7 @@ static void platform_time_calibration(vo
> unsigned long flags;
>
> spin_lock_irqsave(&platform_timer_lock, flags);
> - count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
> + count = plt_stamp64 + ((read_counter() - plt_stamp) & plt_mask);
> stamp = __read_platform_stime(count);
> stime_platform_stamp = stamp;
> platform_timer_stamp = count;
> @@ -883,10 +896,10 @@ static void resume_platform_timer(void)
> {
> /* Timer source can be reset when backing from S3 to S0 */
> if ( plt_src.resume )
> - plt_src.resume(&plt_src);
> + alternative_vcall(plt_src.resume, &plt_src);
>
> plt_stamp64 = platform_timer_stamp;
> - plt_stamp = plt_src.read_counter();
> + plt_stamp = read_counter();
> }
>
> static void __init reset_platform_timer(void)
>
>
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@suse.com>
> 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.)
~Andrew
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@suse.com>
>> 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
© 2016 - 2024 Red Hat, Inc.