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

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 on QEMU, tweaked to provide a 1GHz system counter
frequency. The 'bp.refcounter.base_frequency' property does not seem to
have any affect on the 'ARMvA Base RevC AEM FVP', and instead provides a
100MHz counter.

Parent commit: 0c32706dac1b ("arm64: stacktrace: avoid tracing arch_stack_walk()")

 drivers/clocksource/arm_arch_timer.c | 32 ++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index be6d741d404c..f4816b22213c 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c

@@ -52,6 +52,12 @@
 #define CNTV_TVAL	0x38
 #define CNTV_CTL	0x3c
 
+/*
+ * The minimum amount of time a generic counter is guaranteed to not roll over
+ * (40 years)
+ */
+#define MIN_ROLLOVER_SECS	(40ULL * 365 * 24 * 3600)
+
 static unsigned arch_timers_present __initdata;
 
 static void __iomem *arch_counter_base __ro_after_init;

@@ -205,13 +211,11 @@ static struct clocksource clocksource_counter = {
 	.id	= CSID_ARM_ARCH_COUNTER,
 	.rating	= 400,
 	.read	= arch_counter_read,
-	.mask	= CLOCKSOURCE_MASK(56),
 	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
 };
 
 static struct cyclecounter cyclecounter __ro_after_init = {
 	.read	= arch_counter_read_cc,
-	.mask	= CLOCKSOURCE_MASK(56),
 };
 
 struct ate_acpi_oem_info {

@@ -1004,9 +1008,26 @@ struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
 	return &arch_timer_kvm_info;
 }
 
+/*
+ * Makes an educated guess at a valid counter width based on the Generic Timer
+ * specification. Of note:
+ *   1) the system counter is at least 56 bits wide
+ *   2) a roll-over time of not less than 40 years
+ *
+ * See 'ARM DDI 0487G.a D11.1.2 ("The system counter")' for more details.
+ */
+static int __init arch_counter_get_width(void)
+{
+	u64 min_cycles = MIN_ROLLOVER_SECS * arch_timer_rate;
+
+	/* guarantee the returned width is within the valid range */
+	return clamp_val(ilog2(min_cycles), 56, 64);
+}
+
 static void __init arch_counter_register(unsigned type)
 {
 	u64 start_count;
+	int width;
 
 	/* Register the CP15 based counter if we have one */
 	if (type & ARCH_TIMER_TYPE_CP15) {

@@ -1031,6 +1052,10 @@ static void __init arch_counter_register(unsigned type)
 		arch_timer_read_counter = arch_counter_get_cntvct_mem;
 	}
 
+	width = arch_counter_get_width();
+	clocksource_counter.mask = CLOCKSOURCE_MASK(width);
+	cyclecounter.mask = CLOCKSOURCE_MASK(width);
+
 	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)

-- 
2.32.0.605.g8dce9f2422-goog


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

On Sat, Aug 7, 2021 at 9:14 PM Oliver Upton [off-list ref] wrote:

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>

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?

Yours,
Linus Walleij

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

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

On Sat, 07 Aug 2021 23:30:20 +0100,
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>

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);

That's where things stop. The counter is not writable, and for good
reasons (it is shared with all the CPUs in the system).

    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?

The only way to emulate this behaviour is to use CNTVOFF_EL2 at EL2 to
offset a guest view of the counter, and to run minimal guest that will
do the start/stop/compare work. Given that it involves running a guest
at a point where we are unable to do so, and that it cannot work when
booted at EL1, we're left with guesswork.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

On Sun, 08 Aug 2021 02:14:35 +0100,
Oliver Upton [off-list ref] wrote:

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.

It also has the disadvantage that a large number of DT timer nodes are
a mess of cargo-culted, copy-pasted idioms, and that adding another
property would only make it worse. I'm more confident with something
that can be either:

- checked from EL2 using CNTVOFF, which is complicated, doesn't work
  at EL1, and leaves us in a weird state if we have different counter
  width views in the system (BL is such a wonderful concept)

- or computed from first principle based on the requirements of the
  architecture.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

On Sun, Aug 8, 2021 at 3:40 AM Marc Zyngier [off-list ref] wrote:

On Sun, 08 Aug 2021 02:14:35 +0100,
Oliver Upton [off-list ref] wrote:

quoted

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.

It also has the disadvantage that a large number of DT timer nodes are
a mess of cargo-culted, copy-pasted idioms, and that adding another
property would only make it worse.

Agreed, this does seem like the best solution, short of the
architecture actually providing something to determine the counter
width.

On that note, I wonder how (if ever) we will be able to move away from
unnecessarily masking a 64 bit counter, i.e. a v8.6 or above
implementation. With this patch, one such counter would wrap after
36.56 years, short of the 40 year guarantee we have from the
architecture for < v8.6 implementations. Getting it to 64 bits would
squarely make it someone else's problem ~585 years from now :)

--
Thanks,
Oliver

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

On Sun, 08 Aug 2021 20:01:10 +0100,
Oliver Upton [off-list ref] wrote:

On Sun, Aug 8, 2021 at 3:40 AM Marc Zyngier [off-list ref] wrote:

quoted

On Sun, 08 Aug 2021 02:14:35 +0100,
Oliver Upton [off-list ref] wrote:

quoted

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.

It also has the disadvantage that a large number of DT timer nodes are
a mess of cargo-culted, copy-pasted idioms, and that adding another
property would only make it worse.

Agreed, this does seem like the best solution, short of the
architecture actually providing something to determine the counter
width.

On that note, I wonder how (if ever) we will be able to move away from
unnecessarily masking a 64 bit counter, i.e. a v8.6 or above
implementation. With this patch, one such counter would wrap after
36.56 years, short of the 40 year guarantee we have from the
architecture for < v8.6 implementations. Getting it to 64 bits would
squarely make it someone else's problem ~585 years from now :)

Hmmm. If you end-up with something that falls short of 40 years, then
I suspect something is wrong in the way you compute the required
width.

40 years @1GHz (which we shall call FY1G from now on) fits comfortably
in 61 bits, and I fear that your use of ilog2() gives you one less bit
than what it should be:

log2(FY1G) ~= 60.13

What you are after is probably (ilog2(FY1G - 1) + 1), similar to the
way roundup_pow_of_two() works.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

On Sat, 07 Aug 2021 20:14:28 +0100,
Oliver Upton [off-list ref] wrote:

See my comment somewhere else in the thread about the potential wasted
bit.

For the record, there is one spot where the clockevent gets registered
and configured that also needs addressing (there is a mask harcoded to
31 bits there, which is pretty odd). It cannot be fixed directly in
this patch though. I'll probably take this patch on top of my series
and adjust the relevant bits.

	M.

-- 
Without deviation from the norm, progress is not possible.

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

On Mon, Aug 9, 2021 at 3:45 AM Marc Zyngier [off-list ref] wrote:

quoted

On that note, I wonder how (if ever) we will be able to move away from
unnecessarily masking a 64 bit counter, i.e. a v8.6 or above
implementation. With this patch, one such counter would wrap after
36.56 years, short of the 40 year guarantee we have from the
architecture for < v8.6 implementations. Getting it to 64 bits would
squarely make it someone else's problem ~585 years from now :)

Hmmm. If you end-up with something that falls short of 40 years, then
I suspect something is wrong in the way you compute the required
width.

40 years @1GHz (which we shall call FY1G from now on) fits comfortably
in 61 bits, and I fear that your use of ilog2() gives you one less bit
than what it should be:

log2(FY1G) ~= 60.13

Right, this is round-down behavior was deliberate. Reading the ARM ARM
D11.1.2 'The system counter', I did not find any language that
suggested the counter saturates the register width before rolling
over. So, it may be paranoid, but I presumed it to be safer to wrap
within the guaranteed interval rather than assume the sanity of the
system counter implementation. That being said, fine with rounding up
instead, so long as we don't believe there's any chance of hardware
doing something crazy.

--
Thanks,
Oliver

Thanks,

        M.

--
Without deviation from the norm, progress is not possible.

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