On Wed, Jan 04, 2023 at 01:50:50PM +0100, gregkh@xxxxxxxxxxxxxxxxxxx wrote:
> CAUTION! External Email. Do not click links or open attachments unless you recognize the sender and are sure the content is safe.
> If you think this is a phishing email, please report it by using the "Phish Alert Report" button in Outlook.
>
> The patch below does not apply to the 6.1-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@xxxxxxxxxxxxxxx>.
>
> Possible dependencies:
>
> 45ae272a948a ("clocksource/drivers/arm_arch_timer: Fix XGene-1 TVAL register math error")
>
> thanks,
> greg k-h
Hi Greg,
The patch is already present in linux-6.1.y.
Ref: 839a973988a94 ("clocksource/drivers/arm_arch_timer: Fix XGene-1 TVAL register math error")
Regards,
Joe
>
> ------------------ original commit in Linus's tree ------------------
>
> >From 45ae272a948a03a7d55748bf52d2f47d3b4e1d5a Mon Sep 17 00:00:00 2001
> From: Joe Korty <joe.korty@xxxxxxxxxxxxxxxxx>
> Date: Mon, 21 Nov 2022 14:53:43 +0000
> Subject: [PATCH] clocksource/drivers/arm_arch_timer: Fix XGene-1 TVAL register
> math error
>
> The TVAL register is 32 bit signed. Thus only the lower 31 bits are
> available to specify when an interrupt is to occur at some time in the
> near future. Attempting to specify a larger interval with TVAL results
> in a negative time delta which means the timer fires immediately upon
> being programmed, rather than firing at that expected future time.
>
> The solution is for Linux to declare that TVAL is a 31 bit register rather
> than give its true size of 32 bits. This prevents Linux from programming
> TVAL with a too-large value. Note that, prior to 5.16, this little trick
> was the standard way to handle TVAL in Linux, so there is nothing new
> happening here on that front.
>
> The softlockup detector hides the issue, because it keeps generating
> short timer deadlines that are within the scope of the broken timer.
>
> Disable it, and you start using NO_HZ with much longer timer deadlines,
> which turns into an interrupt flood:
>
> 11: 1124855130 949168462 758009394 76417474 104782230 30210281
> 310890 1734323687 GICv2 29 Level arch_timer
>
> And "much longer" isn't that long: it takes less than 43s to underflow
> TVAL at 50MHz (the frequency of the counter on XGene-1).
>
> Some comments on the v1 version of this patch by Marc Zyngier:
>
> XGene implements CVAL (a 64bit comparator) in terms of TVAL (a countdown
> register) instead of the other way around. TVAL being a 32bit register,
> the width of the counter should equally be 32. However, TVAL is a
> *signed* value, and keeps counting down in the negative range once the
> timer fires.
>
> It means that any TVAL value with bit 31 set will fire immediately,
> as it cannot be distinguished from an already expired timer. Reducing
> the timer range back to a paltry 31 bits papers over the issue.
>
> Another problem cannot be fixed though, which is that the timer interrupt
> *must* be handled within the negative countdown period, or the interrupt
> will be lost (TVAL will rollover to a positive value, indicative of a
> new timer deadline).
>
> Cc: stable@xxxxxxxxxxxxxxx # 5.16+
> Fixes: 012f18850452 ("clocksource/drivers/arm_arch_timer: Work around broken CVAL implementations")
> Signed-off-by: Joe Korty <joe.korty@xxxxxxxxxxxxxxxxx>
> Reviewed-by: Marc Zyngier <maz@xxxxxxxxxx>
> [maz: revamped the commit message]
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/20221024165422.GA51107@xxxxxxxxxxxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/r/20221121145343.896018-1-maz@xxxxxxxxxx
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 9c3420a0d19d..e2920da18ea1 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -806,6 +806,9 @@ static u64 __arch_timer_check_delta(void)
> /*
> * XGene-1 implements CVAL in terms of TVAL, meaning
> * that the maximum timer range is 32bit. Shame on them.
> + *
> + * Note that TVAL is signed, thus has only 31 of its
> + * 32 bits to express magnitude.
> */
> MIDR_ALL_VERSIONS(MIDR_CPU_MODEL(ARM_CPU_IMP_APM,
> APM_CPU_PART_POTENZA)),
> @@ -813,8 +816,8 @@ static u64 __arch_timer_check_delta(void)
> };
>
> if (is_midr_in_range_list(read_cpuid_id(), broken_cval_midrs)) {
> - pr_warn_once("Broken CNTx_CVAL_EL1, limiting width to 32bits");
> - return CLOCKSOURCE_MASK(32);
> + pr_warn_once("Broken CNTx_CVAL_EL1, using 32 bit TVAL instead.\n");
> + return CLOCKSOURCE_MASK(31);
> }
> #endif
> return CLOCKSOURCE_MASK(arch_counter_get_width());
>
