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

Hi Linus,

On Sat, Aug 7, 2021 at 3:30 PM Linus Walleij [off-list ref] wrote:

On Sat, Aug 7, 2021 at 9:14 PM 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 looks good to me:
Reviewed-by: Linus Walleij <redacted>

Thanks for the review!

Just a thought that crossed my mind: as this is real hardware we are
talking about mostly, how hard would it be for arch_counter_get_width()
to detect how wide it actually is if nbits > 56?

I would do something like this pseudocode:

nbits = 56;
while (nbits < 64)
    startval = GENMASK(nbits, 0);
    write_counter(startval);
    start_counter;
    nsleep(1);
    stop_counter;
    now = read_counter;
    if (now < startval)
         /* Ooops it wrapped */
         break;
    nbits++

pr_info("counter has %d bits\n", nbits);

Or did you folks already try this approach?

This would be a good idea, although I believe our only means of
offsetting the counter are available in EL2. I had thought we could
use a CVAL register instead, but this quote from the ARM ARM doesn't
imply the CVAL bit width matches that of the system counter:

<quote>
If the Generic counter is implemented at a size less than 64 bits,
then this field is permitted to be implemented at the same width as
the counter, and the upper bits are RES0.
</quote>

The only other sane idea that I could come up with is providing this
information to the kernel through DT, although that would leave ACPI
systems behind.

--
Thanks,
Oliver

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