Re: [RESEND PATCH] clocksource/arm_arch_timer: Fix masking for high freq counters

Hi Marc,

On Sat, Aug 7, 2021 at 3:52 AM Marc Zyngier [off-list ref] wrote:

Hi Oliver,

On Fri, 06 Aug 2021 19:21:26 +0100,
Oliver Upton [off-list ref] wrote:

quoted

Unfortunately, the architecture provides no means to determine the bit
width of the system counter. However, we do know the following from the
specification:

 - the system counter is at least 56 bits wide
 - Roll-over time of not less than 40 years

To date, the arch timer driver has depended on the first property,
assuming any system counter to be 56 bits wide and masking off the rest.
However, combining a narrow clocksource mask with a high frequency
counter could result in prematurely wrapping the system counter by a
significant margin. For example, a 56 bit wide, 1GHz system counter
would wrap in a mere 2.28 years!

This is a problem for two reasons: v8.6+ implementations are required to
provide a 64 bit, 1GHz system counter. Furthermore, before v8.6,
implementers may select a counter frequency of their choosing.

Fix the issue by deriving a valid clock mask based on the second
property from above. Set the floor at 56 bits, since we know no system
counter is narrower than that.

Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <redacted>
---
This patch was tested with QEMU, tweaked to provide a 1GHz system
counter frequency, as I could not easily figure out how to tweak the
base FVP to provide a 1GHz counter.

<FVP>
"bp.refcounter.base_frequency" is the property you are looking for. In
general, passing --list-params to the model reveals a treasure trove
of weird and wonderful options that can be used to configure the model
to your liking.
</FVP>

Ah! Thanks for the tip. I didn't find the fast models documentation to
be too terribly helpful.

quoted

+/*
+ * The minimum amount of time a generic timer is guaranteed to not roll over

nit: s/timer/counter/

Ack.

quoted

+ * (40 years)

For later reference, could you add the section of the ARMv8 ARM where
this is mentioned? Something like 'ARM DDI 0487G.a D11.1.2 ("The
system counter")', either here or in the comment further down.

Sure thing. I think I'll drop it on the function below since we depend
on two properties of the system counter there.

quoted

+static int __init arch_counter_get_width(void)
+{
+     u64 min_cycles = MIN_ROLLOVER_SECS * arch_timer_get_cntfrq();

That's unfortunately wishful thinking. We have stupidly broken systems
out there that do not set CNTFRQ_EL0, and instead rely on DT
properties to describe the counter frequency. You're likely to end up
with a glorious zero as a result, with interesting consequences...

Blech. I didn't read the DT hooks for the driver.  That's awful.

Use arch_timer_rate instead, which will be set as by the time you
register the counter.

Ack.

quoted

+
+     /* guarantee the returned width is within the valid range */
+     return max(56, min(64, ilog2(min_cycles)));

Maybe better written as "clamp_val(ilog2(min_cycles), 56, 64);".

Agreed.

quoted

+     width = arch_counter_get_width();
+     clocksource_counter.mask = CLOCKSOURCE_MASK(width);
+     cyclecounter.mask = CLOCKSOURCE_MASK(width);

Since you move this to be computed at runtime, how about dropping the
static initialisation of the mask fields?

Yep, already made the change locally after mailing out :)

quoted

+
      if (!arch_counter_suspend_stop)
              clocksource_counter.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP;
      start_count = arch_timer_read_counter();

@@ -1040,8 +1065,7 @@ static void __init arch_counter_register(unsigned type)
      timecounter_init(&arch_timer_kvm_info.timecounter,
                       &cyclecounter, start_count);

-     /* 56 bits minimum, so we assume worst case rollover */
-     sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate);
+     sched_clock_register(arch_timer_read_counter, width, arch_timer_rate);
 }

 static void arch_timer_stop(struct clock_event_device *clk)

Another thing that needs addressing for high frequency counter support
is to move away from TVAL programming and switch to CVAL, as the
maximum deadline we can currently program is 4.2s at 1GHz.

Fun fact: it has the interesting consequence of breaking XGene-1,
which implemented CVAL in terms of TVAL instead of the other way
around (what were these guys thinking?)

Yikes!

, though I don't think anyone
will notice in practice. I have a preliminary patch on a branch
somewhere that I'll try to dust up and post in the coming days.

Sounds good, I'll clean this one up and send it out too.

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel