* [PATCH 01/13] clocksource/arm_arch_timer: Drop CNT*_TVAL read accessors
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-11 7:02 ` Oliver Upton
2021-08-24 16:20 ` Mark Rutland
2021-08-09 15:26 ` [PATCH 02/13] clocksource/arm_arch_timer: Extend write side of timer register accessors to u64 Marc Zyngier
` (11 subsequent siblings)
12 siblings, 2 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
The arch timer driver never reads the various TVAL registers, only
writes to them. It is thus pointless to provide accessors
for them and to implement errata workarounds.
Drop these read-side accessors, and add a couple of BUG() statements
for the time being. These statements will be removed further down
the line.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm/include/asm/arch_timer.h | 6 ++--
arch/arm64/include/asm/arch_timer.h | 17 ++---------
drivers/clocksource/arm_arch_timer.c | 44 ++--------------------------
3 files changed, 6 insertions(+), 61 deletions(-)
diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 99175812d903..0c09afaa590d 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -60,8 +60,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
break;
case ARCH_TIMER_REG_TVAL:
- asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
- break;
+ BUG();
}
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
switch (reg) {
@@ -69,8 +68,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
break;
case ARCH_TIMER_REG_TVAL:
- asm volatile("mrc p15, 0, %0, c14, c3, 0" : "=r" (val));
- break;
+ BUG();
}
}
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 88d20f04c64a..8e3b2ac60c30 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -52,8 +52,6 @@ struct arch_timer_erratum_workaround {
enum arch_timer_erratum_match_type match_type;
const void *id;
const char *desc;
- u32 (*read_cntp_tval_el0)(void);
- u32 (*read_cntv_tval_el0)(void);
u64 (*read_cntpct_el0)(void);
u64 (*read_cntvct_el0)(void);
int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
@@ -64,17 +62,6 @@ struct arch_timer_erratum_workaround {
DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
timer_unstable_counter_workaround);
-/* inline sysreg accessors that make erratum_handler() work */
-static inline notrace u32 arch_timer_read_cntp_tval_el0(void)
-{
- return read_sysreg(cntp_tval_el0);
-}
-
-static inline notrace u32 arch_timer_read_cntv_tval_el0(void)
-{
- return read_sysreg(cntv_tval_el0);
-}
-
static inline notrace u64 arch_timer_read_cntpct_el0(void)
{
return read_sysreg(cntpct_el0);
@@ -135,14 +122,14 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
case ARCH_TIMER_REG_CTRL:
return read_sysreg(cntp_ctl_el0);
case ARCH_TIMER_REG_TVAL:
- return arch_timer_reg_read_stable(cntp_tval_el0);
+ break;
}
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
switch (reg) {
case ARCH_TIMER_REG_CTRL:
return read_sysreg(cntv_ctl_el0);
case ARCH_TIMER_REG_TVAL:
- return arch_timer_reg_read_stable(cntv_tval_el0);
+ break;
}
}
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index be6d741d404c..9db5c16e31e7 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -141,8 +141,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
val = readl_relaxed(timer->base + CNTP_CTL);
break;
case ARCH_TIMER_REG_TVAL:
- val = readl_relaxed(timer->base + CNTP_TVAL);
- break;
+ BUG();
}
} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
struct arch_timer *timer = to_arch_timer(clk);
@@ -151,8 +150,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
val = readl_relaxed(timer->base + CNTV_CTL);
break;
case ARCH_TIMER_REG_TVAL:
- val = readl_relaxed(timer->base + CNTV_TVAL);
- break;
+ BUG();
}
} else {
val = arch_timer_reg_read_cp15(access, reg);
@@ -239,16 +237,6 @@ struct ate_acpi_oem_info {
_new; \
})
-static u32 notrace fsl_a008585_read_cntp_tval_el0(void)
-{
- return __fsl_a008585_read_reg(cntp_tval_el0);
-}
-
-static u32 notrace fsl_a008585_read_cntv_tval_el0(void)
-{
- return __fsl_a008585_read_reg(cntv_tval_el0);
-}
-
static u64 notrace fsl_a008585_read_cntpct_el0(void)
{
return __fsl_a008585_read_reg(cntpct_el0);
@@ -285,16 +273,6 @@ static u64 notrace fsl_a008585_read_cntvct_el0(void)
_new; \
})
-static u32 notrace hisi_161010101_read_cntp_tval_el0(void)
-{
- return __hisi_161010101_read_reg(cntp_tval_el0);
-}
-
-static u32 notrace hisi_161010101_read_cntv_tval_el0(void)
-{
- return __hisi_161010101_read_reg(cntv_tval_el0);
-}
-
static u64 notrace hisi_161010101_read_cntpct_el0(void)
{
return __hisi_161010101_read_reg(cntpct_el0);
@@ -379,16 +357,6 @@ static u64 notrace sun50i_a64_read_cntvct_el0(void)
{
return __sun50i_a64_read_reg(cntvct_el0);
}
-
-static u32 notrace sun50i_a64_read_cntp_tval_el0(void)
-{
- return read_sysreg(cntp_cval_el0) - sun50i_a64_read_cntpct_el0();
-}
-
-static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
-{
- return read_sysreg(cntv_cval_el0) - sun50i_a64_read_cntvct_el0();
-}
#endif
#ifdef CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND
@@ -438,8 +406,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
.match_type = ate_match_dt,
.id = "fsl,erratum-a008585",
.desc = "Freescale erratum a005858",
- .read_cntp_tval_el0 = fsl_a008585_read_cntp_tval_el0,
- .read_cntv_tval_el0 = fsl_a008585_read_cntv_tval_el0,
.read_cntpct_el0 = fsl_a008585_read_cntpct_el0,
.read_cntvct_el0 = fsl_a008585_read_cntvct_el0,
.set_next_event_phys = erratum_set_next_event_tval_phys,
@@ -451,8 +417,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
.match_type = ate_match_dt,
.id = "hisilicon,erratum-161010101",
.desc = "HiSilicon erratum 161010101",
- .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0,
- .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0,
.read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
.read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
.set_next_event_phys = erratum_set_next_event_tval_phys,
@@ -462,8 +426,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
.match_type = ate_match_acpi_oem_info,
.id = hisi_161010101_oem_info,
.desc = "HiSilicon erratum 161010101",
- .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0,
- .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0,
.read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
.read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
.set_next_event_phys = erratum_set_next_event_tval_phys,
@@ -484,8 +446,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
.match_type = ate_match_dt,
.id = "allwinner,erratum-unknown1",
.desc = "Allwinner erratum UNKNOWN1",
- .read_cntp_tval_el0 = sun50i_a64_read_cntp_tval_el0,
- .read_cntv_tval_el0 = sun50i_a64_read_cntv_tval_el0,
.read_cntpct_el0 = sun50i_a64_read_cntpct_el0,
.read_cntvct_el0 = sun50i_a64_read_cntvct_el0,
.set_next_event_phys = erratum_set_next_event_tval_phys,
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 01/13] clocksource/arm_arch_timer: Drop CNT*_TVAL read accessors
2021-08-09 15:26 ` [PATCH 01/13] clocksource/arm_arch_timer: Drop CNT*_TVAL read accessors Marc Zyngier
@ 2021-08-11 7:02 ` Oliver Upton
2021-08-24 16:20 ` Mark Rutland
1 sibling, 0 replies; 39+ messages in thread
From: Oliver Upton @ 2021-08-11 7:02 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier <maz@kernel.org> wrote:
>
> The arch timer driver never reads the various TVAL registers, only
> writes to them. It is thus pointless to provide accessors
> for them and to implement errata workarounds.
>
> Drop these read-side accessors, and add a couple of BUG() statements
> for the time being. These statements will be removed further down
> the line.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Reviewed-by: Oliver Upton <oupton@google.com>
> ---
> arch/arm/include/asm/arch_timer.h | 6 ++--
> arch/arm64/include/asm/arch_timer.h | 17 ++---------
> drivers/clocksource/arm_arch_timer.c | 44 ++--------------------------
> 3 files changed, 6 insertions(+), 61 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 99175812d903..0c09afaa590d 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -60,8 +60,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
> break;
> case ARCH_TIMER_REG_TVAL:
> - asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
> - break;
> + BUG();
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> @@ -69,8 +68,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
> break;
> case ARCH_TIMER_REG_TVAL:
> - asm volatile("mrc p15, 0, %0, c14, c3, 0" : "=r" (val));
> - break;
> + BUG();
> }
> }
>
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 88d20f04c64a..8e3b2ac60c30 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -52,8 +52,6 @@ struct arch_timer_erratum_workaround {
> enum arch_timer_erratum_match_type match_type;
> const void *id;
> const char *desc;
> - u32 (*read_cntp_tval_el0)(void);
> - u32 (*read_cntv_tval_el0)(void);
> u64 (*read_cntpct_el0)(void);
> u64 (*read_cntvct_el0)(void);
> int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
> @@ -64,17 +62,6 @@ struct arch_timer_erratum_workaround {
> DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
> timer_unstable_counter_workaround);
>
> -/* inline sysreg accessors that make erratum_handler() work */
> -static inline notrace u32 arch_timer_read_cntp_tval_el0(void)
> -{
> - return read_sysreg(cntp_tval_el0);
> -}
> -
> -static inline notrace u32 arch_timer_read_cntv_tval_el0(void)
> -{
> - return read_sysreg(cntv_tval_el0);
> -}
> -
> static inline notrace u64 arch_timer_read_cntpct_el0(void)
> {
> return read_sysreg(cntpct_el0);
> @@ -135,14 +122,14 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> case ARCH_TIMER_REG_CTRL:
> return read_sysreg(cntp_ctl_el0);
> case ARCH_TIMER_REG_TVAL:
> - return arch_timer_reg_read_stable(cntp_tval_el0);
> + break;
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> return read_sysreg(cntv_ctl_el0);
> case ARCH_TIMER_REG_TVAL:
> - return arch_timer_reg_read_stable(cntv_tval_el0);
> + break;
> }
> }
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index be6d741d404c..9db5c16e31e7 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -141,8 +141,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
> val = readl_relaxed(timer->base + CNTP_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> - val = readl_relaxed(timer->base + CNTP_TVAL);
> - break;
> + BUG();
> }
> } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
> struct arch_timer *timer = to_arch_timer(clk);
> @@ -151,8 +150,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
> val = readl_relaxed(timer->base + CNTV_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> - val = readl_relaxed(timer->base + CNTV_TVAL);
> - break;
> + BUG();
> }
> } else {
> val = arch_timer_reg_read_cp15(access, reg);
> @@ -239,16 +237,6 @@ struct ate_acpi_oem_info {
> _new; \
> })
>
> -static u32 notrace fsl_a008585_read_cntp_tval_el0(void)
> -{
> - return __fsl_a008585_read_reg(cntp_tval_el0);
> -}
> -
> -static u32 notrace fsl_a008585_read_cntv_tval_el0(void)
> -{
> - return __fsl_a008585_read_reg(cntv_tval_el0);
> -}
> -
> static u64 notrace fsl_a008585_read_cntpct_el0(void)
> {
> return __fsl_a008585_read_reg(cntpct_el0);
> @@ -285,16 +273,6 @@ static u64 notrace fsl_a008585_read_cntvct_el0(void)
> _new; \
> })
>
> -static u32 notrace hisi_161010101_read_cntp_tval_el0(void)
> -{
> - return __hisi_161010101_read_reg(cntp_tval_el0);
> -}
> -
> -static u32 notrace hisi_161010101_read_cntv_tval_el0(void)
> -{
> - return __hisi_161010101_read_reg(cntv_tval_el0);
> -}
> -
> static u64 notrace hisi_161010101_read_cntpct_el0(void)
> {
> return __hisi_161010101_read_reg(cntpct_el0);
> @@ -379,16 +357,6 @@ static u64 notrace sun50i_a64_read_cntvct_el0(void)
> {
> return __sun50i_a64_read_reg(cntvct_el0);
> }
> -
> -static u32 notrace sun50i_a64_read_cntp_tval_el0(void)
> -{
> - return read_sysreg(cntp_cval_el0) - sun50i_a64_read_cntpct_el0();
> -}
> -
> -static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
> -{
> - return read_sysreg(cntv_cval_el0) - sun50i_a64_read_cntvct_el0();
> -}
> #endif
>
> #ifdef CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND
> @@ -438,8 +406,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .match_type = ate_match_dt,
> .id = "fsl,erratum-a008585",
> .desc = "Freescale erratum a005858",
> - .read_cntp_tval_el0 = fsl_a008585_read_cntp_tval_el0,
> - .read_cntv_tval_el0 = fsl_a008585_read_cntv_tval_el0,
> .read_cntpct_el0 = fsl_a008585_read_cntpct_el0,
> .read_cntvct_el0 = fsl_a008585_read_cntvct_el0,
> .set_next_event_phys = erratum_set_next_event_tval_phys,
> @@ -451,8 +417,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .match_type = ate_match_dt,
> .id = "hisilicon,erratum-161010101",
> .desc = "HiSilicon erratum 161010101",
> - .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0,
> - .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0,
> .read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
> .read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
> .set_next_event_phys = erratum_set_next_event_tval_phys,
> @@ -462,8 +426,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .match_type = ate_match_acpi_oem_info,
> .id = hisi_161010101_oem_info,
> .desc = "HiSilicon erratum 161010101",
> - .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0,
> - .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0,
> .read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
> .read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
> .set_next_event_phys = erratum_set_next_event_tval_phys,
> @@ -484,8 +446,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .match_type = ate_match_dt,
> .id = "allwinner,erratum-unknown1",
> .desc = "Allwinner erratum UNKNOWN1",
> - .read_cntp_tval_el0 = sun50i_a64_read_cntp_tval_el0,
> - .read_cntv_tval_el0 = sun50i_a64_read_cntv_tval_el0,
> .read_cntpct_el0 = sun50i_a64_read_cntpct_el0,
> .read_cntvct_el0 = sun50i_a64_read_cntvct_el0,
> .set_next_event_phys = erratum_set_next_event_tval_phys,
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 01/13] clocksource/arm_arch_timer: Drop CNT*_TVAL read accessors
2021-08-09 15:26 ` [PATCH 01/13] clocksource/arm_arch_timer: Drop CNT*_TVAL read accessors Marc Zyngier
2021-08-11 7:02 ` Oliver Upton
@ 2021-08-24 16:20 ` Mark Rutland
1 sibling, 0 replies; 39+ messages in thread
From: Mark Rutland @ 2021-08-24 16:20 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Daniel Lezcano, Thomas Gleixner,
Peter Shier, Raghavendra Rao Ananta, Ricardo Koller,
Oliver Upton, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 09, 2021 at 04:26:39PM +0100, Marc Zyngier wrote:
> The arch timer driver never reads the various TVAL registers, only
> writes to them. It is thus pointless to provide accessors
> for them and to implement errata workarounds.
>
> Drop these read-side accessors, and add a couple of BUG() statements
> for the time being. These statements will be removed further down
> the line.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm/include/asm/arch_timer.h | 6 ++--
> arch/arm64/include/asm/arch_timer.h | 17 ++---------
> drivers/clocksource/arm_arch_timer.c | 44 ++--------------------------
> 3 files changed, 6 insertions(+), 61 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 99175812d903..0c09afaa590d 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -60,8 +60,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
> break;
> case ARCH_TIMER_REG_TVAL:
> - asm volatile("mrc p15, 0, %0, c14, c2, 0" : "=r" (val));
> - break;
> + BUG();
It would be nice if we had:
default:
BUG(); // or maybe BUILD_BUG()
... in all these switches to avoid surprises in future.
If we did that as a prep patch, this patch would be a pure deletion.
Either way, this looks good:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Builds cleanly, and boots fine on a fast model (both arm/arm64):
Tested-by: Mark Rutland <mark.rutland@arm.com>
Thanks,
Mark.
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> @@ -69,8 +68,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
> break;
> case ARCH_TIMER_REG_TVAL:
> - asm volatile("mrc p15, 0, %0, c14, c3, 0" : "=r" (val));
> - break;
> + BUG();
> }
> }
>
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 88d20f04c64a..8e3b2ac60c30 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -52,8 +52,6 @@ struct arch_timer_erratum_workaround {
> enum arch_timer_erratum_match_type match_type;
> const void *id;
> const char *desc;
> - u32 (*read_cntp_tval_el0)(void);
> - u32 (*read_cntv_tval_el0)(void);
> u64 (*read_cntpct_el0)(void);
> u64 (*read_cntvct_el0)(void);
> int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
> @@ -64,17 +62,6 @@ struct arch_timer_erratum_workaround {
> DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
> timer_unstable_counter_workaround);
>
> -/* inline sysreg accessors that make erratum_handler() work */
> -static inline notrace u32 arch_timer_read_cntp_tval_el0(void)
> -{
> - return read_sysreg(cntp_tval_el0);
> -}
> -
> -static inline notrace u32 arch_timer_read_cntv_tval_el0(void)
> -{
> - return read_sysreg(cntv_tval_el0);
> -}
> -
> static inline notrace u64 arch_timer_read_cntpct_el0(void)
> {
> return read_sysreg(cntpct_el0);
> @@ -135,14 +122,14 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> case ARCH_TIMER_REG_CTRL:
> return read_sysreg(cntp_ctl_el0);
> case ARCH_TIMER_REG_TVAL:
> - return arch_timer_reg_read_stable(cntp_tval_el0);
> + break;
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> return read_sysreg(cntv_ctl_el0);
> case ARCH_TIMER_REG_TVAL:
> - return arch_timer_reg_read_stable(cntv_tval_el0);
> + break;
> }
> }
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index be6d741d404c..9db5c16e31e7 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -141,8 +141,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
> val = readl_relaxed(timer->base + CNTP_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> - val = readl_relaxed(timer->base + CNTP_TVAL);
> - break;
> + BUG();
> }
> } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
> struct arch_timer *timer = to_arch_timer(clk);
> @@ -151,8 +150,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
> val = readl_relaxed(timer->base + CNTV_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> - val = readl_relaxed(timer->base + CNTV_TVAL);
> - break;
> + BUG();
> }
> } else {
> val = arch_timer_reg_read_cp15(access, reg);
> @@ -239,16 +237,6 @@ struct ate_acpi_oem_info {
> _new; \
> })
>
> -static u32 notrace fsl_a008585_read_cntp_tval_el0(void)
> -{
> - return __fsl_a008585_read_reg(cntp_tval_el0);
> -}
> -
> -static u32 notrace fsl_a008585_read_cntv_tval_el0(void)
> -{
> - return __fsl_a008585_read_reg(cntv_tval_el0);
> -}
> -
> static u64 notrace fsl_a008585_read_cntpct_el0(void)
> {
> return __fsl_a008585_read_reg(cntpct_el0);
> @@ -285,16 +273,6 @@ static u64 notrace fsl_a008585_read_cntvct_el0(void)
> _new; \
> })
>
> -static u32 notrace hisi_161010101_read_cntp_tval_el0(void)
> -{
> - return __hisi_161010101_read_reg(cntp_tval_el0);
> -}
> -
> -static u32 notrace hisi_161010101_read_cntv_tval_el0(void)
> -{
> - return __hisi_161010101_read_reg(cntv_tval_el0);
> -}
> -
> static u64 notrace hisi_161010101_read_cntpct_el0(void)
> {
> return __hisi_161010101_read_reg(cntpct_el0);
> @@ -379,16 +357,6 @@ static u64 notrace sun50i_a64_read_cntvct_el0(void)
> {
> return __sun50i_a64_read_reg(cntvct_el0);
> }
> -
> -static u32 notrace sun50i_a64_read_cntp_tval_el0(void)
> -{
> - return read_sysreg(cntp_cval_el0) - sun50i_a64_read_cntpct_el0();
> -}
> -
> -static u32 notrace sun50i_a64_read_cntv_tval_el0(void)
> -{
> - return read_sysreg(cntv_cval_el0) - sun50i_a64_read_cntvct_el0();
> -}
> #endif
>
> #ifdef CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND
> @@ -438,8 +406,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .match_type = ate_match_dt,
> .id = "fsl,erratum-a008585",
> .desc = "Freescale erratum a005858",
> - .read_cntp_tval_el0 = fsl_a008585_read_cntp_tval_el0,
> - .read_cntv_tval_el0 = fsl_a008585_read_cntv_tval_el0,
> .read_cntpct_el0 = fsl_a008585_read_cntpct_el0,
> .read_cntvct_el0 = fsl_a008585_read_cntvct_el0,
> .set_next_event_phys = erratum_set_next_event_tval_phys,
> @@ -451,8 +417,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .match_type = ate_match_dt,
> .id = "hisilicon,erratum-161010101",
> .desc = "HiSilicon erratum 161010101",
> - .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0,
> - .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0,
> .read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
> .read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
> .set_next_event_phys = erratum_set_next_event_tval_phys,
> @@ -462,8 +426,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .match_type = ate_match_acpi_oem_info,
> .id = hisi_161010101_oem_info,
> .desc = "HiSilicon erratum 161010101",
> - .read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0,
> - .read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0,
> .read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
> .read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
> .set_next_event_phys = erratum_set_next_event_tval_phys,
> @@ -484,8 +446,6 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .match_type = ate_match_dt,
> .id = "allwinner,erratum-unknown1",
> .desc = "Allwinner erratum UNKNOWN1",
> - .read_cntp_tval_el0 = sun50i_a64_read_cntp_tval_el0,
> - .read_cntv_tval_el0 = sun50i_a64_read_cntv_tval_el0,
> .read_cntpct_el0 = sun50i_a64_read_cntpct_el0,
> .read_cntvct_el0 = sun50i_a64_read_cntvct_el0,
> .set_next_event_phys = erratum_set_next_event_tval_phys,
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 02/13] clocksource/arm_arch_timer: Extend write side of timer register accessors to u64
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
2021-08-09 15:26 ` [PATCH 01/13] clocksource/arm_arch_timer: Drop CNT*_TVAL read accessors Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 16:12 ` Oliver Upton
2021-08-24 16:20 ` Mark Rutland
2021-08-09 15:26 ` [PATCH 03/13] clocksource/arm_arch_timer: Move system register timer programming over to CVAL Marc Zyngier
` (10 subsequent siblings)
12 siblings, 2 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
The various accessors for the timer sysreg and MMIO registers are
currently hardwired to 32bit. However, we are about to introduce
the use of the CVAL registers, which require a 64bit access.
Upgrade the write side of the accessors to take a 64bit value
(the read side is left untouched as we don't plan to ever read
back any of these registers).
No functional change expected.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm/include/asm/arch_timer.h | 10 +++++-----
arch/arm64/include/asm/arch_timer.h | 2 +-
drivers/clocksource/arm_arch_timer.c | 10 +++++-----
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 0c09afaa590d..88075c7f4bfd 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -24,24 +24,24 @@ int arch_timer_arch_init(void);
* the code. At least it does so with a recent GCC (4.6.3).
*/
static __always_inline
-void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val)
+void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
{
if (access == ARCH_TIMER_PHYS_ACCESS) {
switch (reg) {
case ARCH_TIMER_REG_CTRL:
- asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" (val));
+ asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" ((u32)val));
break;
case ARCH_TIMER_REG_TVAL:
- asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (val));
+ asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" ((u32)val));
break;
}
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
switch (reg) {
case ARCH_TIMER_REG_CTRL:
- asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" (val));
+ asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" ((u32)val));
break;
case ARCH_TIMER_REG_TVAL:
- asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" (val));
+ asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" ((u32)val));
break;
}
}
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 8e3b2ac60c30..107afb721749 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -89,7 +89,7 @@ static inline notrace u64 arch_timer_read_cntvct_el0(void)
* the code.
*/
static __always_inline
-void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val)
+void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
{
if (access == ARCH_TIMER_PHYS_ACCESS) {
switch (reg) {
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 9db5c16e31e7..0b2bac3ef7ce 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -100,27 +100,27 @@ early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
*/
static __always_inline
-void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val,
+void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
struct clock_event_device *clk)
{
if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
struct arch_timer *timer = to_arch_timer(clk);
switch (reg) {
case ARCH_TIMER_REG_CTRL:
- writel_relaxed(val, timer->base + CNTP_CTL);
+ writel_relaxed((u32)val, timer->base + CNTP_CTL);
break;
case ARCH_TIMER_REG_TVAL:
- writel_relaxed(val, timer->base + CNTP_TVAL);
+ writel_relaxed((u32)val, timer->base + CNTP_TVAL);
break;
}
} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
struct arch_timer *timer = to_arch_timer(clk);
switch (reg) {
case ARCH_TIMER_REG_CTRL:
- writel_relaxed(val, timer->base + CNTV_CTL);
+ writel_relaxed((u32)val, timer->base + CNTV_CTL);
break;
case ARCH_TIMER_REG_TVAL:
- writel_relaxed(val, timer->base + CNTV_TVAL);
+ writel_relaxed((u32)val, timer->base + CNTV_TVAL);
break;
}
} else {
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 02/13] clocksource/arm_arch_timer: Extend write side of timer register accessors to u64
2021-08-09 15:26 ` [PATCH 02/13] clocksource/arm_arch_timer: Extend write side of timer register accessors to u64 Marc Zyngier
@ 2021-08-09 16:12 ` Oliver Upton
2021-08-24 16:20 ` Mark Rutland
1 sibling, 0 replies; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 16:12 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier <maz@kernel.org> wrote:
>
> The various accessors for the timer sysreg and MMIO registers are
> currently hardwired to 32bit. However, we are about to introduce
> the use of the CVAL registers, which require a 64bit access.
>
> Upgrade the write side of the accessors to take a 64bit value
> (the read side is left untouched as we don't plan to ever read
> back any of these registers).
>
> No functional change expected.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Reviewed-by: Oliver Upton <oupton@google.com>
> ---
> arch/arm/include/asm/arch_timer.h | 10 +++++-----
> arch/arm64/include/asm/arch_timer.h | 2 +-
> drivers/clocksource/arm_arch_timer.c | 10 +++++-----
> 3 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 0c09afaa590d..88075c7f4bfd 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -24,24 +24,24 @@ int arch_timer_arch_init(void);
> * the code. At least it does so with a recent GCC (4.6.3).
> */
> static __always_inline
> -void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val)
> +void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> {
> if (access == ARCH_TIMER_PHYS_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> - asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" (val));
> + asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" ((u32)val));
> break;
> case ARCH_TIMER_REG_TVAL:
> - asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (val));
> + asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" ((u32)val));
> break;
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> - asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" (val));
> + asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" ((u32)val));
> break;
> case ARCH_TIMER_REG_TVAL:
> - asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" (val));
> + asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" ((u32)val));
> break;
> }
> }
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 8e3b2ac60c30..107afb721749 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -89,7 +89,7 @@ static inline notrace u64 arch_timer_read_cntvct_el0(void)
> * the code.
> */
> static __always_inline
> -void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val)
> +void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> {
> if (access == ARCH_TIMER_PHYS_ACCESS) {
> switch (reg) {
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 9db5c16e31e7..0b2bac3ef7ce 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -100,27 +100,27 @@ early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
> */
>
> static __always_inline
> -void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val,
> +void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
> struct clock_event_device *clk)
> {
> if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
> struct arch_timer *timer = to_arch_timer(clk);
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> - writel_relaxed(val, timer->base + CNTP_CTL);
> + writel_relaxed((u32)val, timer->base + CNTP_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> - writel_relaxed(val, timer->base + CNTP_TVAL);
> + writel_relaxed((u32)val, timer->base + CNTP_TVAL);
> break;
> }
> } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
> struct arch_timer *timer = to_arch_timer(clk);
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> - writel_relaxed(val, timer->base + CNTV_CTL);
> + writel_relaxed((u32)val, timer->base + CNTV_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> - writel_relaxed(val, timer->base + CNTV_TVAL);
> + writel_relaxed((u32)val, timer->base + CNTV_TVAL);
> break;
> }
> } else {
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 02/13] clocksource/arm_arch_timer: Extend write side of timer register accessors to u64
2021-08-09 15:26 ` [PATCH 02/13] clocksource/arm_arch_timer: Extend write side of timer register accessors to u64 Marc Zyngier
2021-08-09 16:12 ` Oliver Upton
@ 2021-08-24 16:20 ` Mark Rutland
1 sibling, 0 replies; 39+ messages in thread
From: Mark Rutland @ 2021-08-24 16:20 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Daniel Lezcano, Thomas Gleixner,
Peter Shier, Raghavendra Rao Ananta, Ricardo Koller,
Oliver Upton, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 09, 2021 at 04:26:40PM +0100, Marc Zyngier wrote:
> The various accessors for the timer sysreg and MMIO registers are
> currently hardwired to 32bit. However, we are about to introduce
> the use of the CVAL registers, which require a 64bit access.
>
> Upgrade the write side of the accessors to take a 64bit value
> (the read side is left untouched as we don't plan to ever read
> back any of these registers).
>
> No functional change expected.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Looks good, builds cleanly, and boots fine on both arm/arm64:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm/include/asm/arch_timer.h | 10 +++++-----
> arch/arm64/include/asm/arch_timer.h | 2 +-
> drivers/clocksource/arm_arch_timer.c | 10 +++++-----
> 3 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 0c09afaa590d..88075c7f4bfd 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -24,24 +24,24 @@ int arch_timer_arch_init(void);
> * the code. At least it does so with a recent GCC (4.6.3).
> */
> static __always_inline
> -void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val)
> +void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> {
> if (access == ARCH_TIMER_PHYS_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> - asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" (val));
> + asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" ((u32)val));
> break;
> case ARCH_TIMER_REG_TVAL:
> - asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (val));
> + asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" ((u32)val));
> break;
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> - asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" (val));
> + asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" ((u32)val));
> break;
> case ARCH_TIMER_REG_TVAL:
> - asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" (val));
> + asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" ((u32)val));
> break;
> }
> }
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 8e3b2ac60c30..107afb721749 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -89,7 +89,7 @@ static inline notrace u64 arch_timer_read_cntvct_el0(void)
> * the code.
> */
> static __always_inline
> -void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u32 val)
> +void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> {
> if (access == ARCH_TIMER_PHYS_ACCESS) {
> switch (reg) {
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 9db5c16e31e7..0b2bac3ef7ce 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -100,27 +100,27 @@ early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
> */
>
> static __always_inline
> -void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val,
> +void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
> struct clock_event_device *clk)
> {
> if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
> struct arch_timer *timer = to_arch_timer(clk);
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> - writel_relaxed(val, timer->base + CNTP_CTL);
> + writel_relaxed((u32)val, timer->base + CNTP_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> - writel_relaxed(val, timer->base + CNTP_TVAL);
> + writel_relaxed((u32)val, timer->base + CNTP_TVAL);
> break;
> }
> } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
> struct arch_timer *timer = to_arch_timer(clk);
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> - writel_relaxed(val, timer->base + CNTV_CTL);
> + writel_relaxed((u32)val, timer->base + CNTV_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> - writel_relaxed(val, timer->base + CNTV_TVAL);
> + writel_relaxed((u32)val, timer->base + CNTV_TVAL);
> break;
> }
> } else {
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 03/13] clocksource/arm_arch_timer: Move system register timer programming over to CVAL
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
2021-08-09 15:26 ` [PATCH 01/13] clocksource/arm_arch_timer: Drop CNT*_TVAL read accessors Marc Zyngier
2021-08-09 15:26 ` [PATCH 02/13] clocksource/arm_arch_timer: Extend write side of timer register accessors to u64 Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-11 7:15 ` Oliver Upton
2021-08-24 16:21 ` Mark Rutland
2021-08-09 15:26 ` [PATCH 04/13] clocksource/arm_arch_timer: Move drop _tval from erratum function names Marc Zyngier
` (9 subsequent siblings)
12 siblings, 2 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
In order to cope better with high frequency counters, move the
programming of the timers from the countdown timer (TVAL) over
to the comparator (CVAL).
The programming model is slightly different, as we now need to
read the current counter value to have an absolute deadline
instead of a relative one.
There is a small overhead to this change, which we will address
in the following patches.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm/include/asm/arch_timer.h | 14 ++++++++----
arch/arm64/include/asm/arch_timer.h | 16 +++++++++-----
drivers/clocksource/arm_arch_timer.c | 32 +++++++++++++++++++++++++---
include/clocksource/arm_arch_timer.h | 1 +
4 files changed, 51 insertions(+), 12 deletions(-)
diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index 88075c7f4bfd..b48de9d26f27 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -31,18 +31,22 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
case ARCH_TIMER_REG_CTRL:
asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" ((u32)val));
break;
- case ARCH_TIMER_REG_TVAL:
- asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" ((u32)val));
+ case ARCH_TIMER_REG_CVAL:
+ asm volatile("mcrr p15, 2, %Q0, %R0, c14" : : "r" (val));
break;
+ case ARCH_TIMER_REG_TVAL:
+ BUG();
}
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
switch (reg) {
case ARCH_TIMER_REG_CTRL:
asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" ((u32)val));
break;
- case ARCH_TIMER_REG_TVAL:
- asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" ((u32)val));
+ case ARCH_TIMER_REG_CVAL:
+ asm volatile("mcrr p15, 3, %Q0, %R0, c14" : : "r" (val));
break;
+ case ARCH_TIMER_REG_TVAL:
+ BUG();
}
}
@@ -60,6 +64,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
break;
case ARCH_TIMER_REG_TVAL:
+ case ARCH_TIMER_REG_CVAL:
BUG();
}
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
@@ -68,6 +73,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
break;
case ARCH_TIMER_REG_TVAL:
+ case ARCH_TIMER_REG_CVAL:
BUG();
}
}
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 107afb721749..6ceb050ae7b9 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -96,18 +96,22 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
case ARCH_TIMER_REG_CTRL:
write_sysreg(val, cntp_ctl_el0);
break;
- case ARCH_TIMER_REG_TVAL:
- write_sysreg(val, cntp_tval_el0);
+ case ARCH_TIMER_REG_CVAL:
+ write_sysreg(val, cntp_cval_el0);
break;
+ case ARCH_TIMER_REG_TVAL:
+ BUG();
}
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
switch (reg) {
case ARCH_TIMER_REG_CTRL:
write_sysreg(val, cntv_ctl_el0);
break;
- case ARCH_TIMER_REG_TVAL:
- write_sysreg(val, cntv_tval_el0);
+ case ARCH_TIMER_REG_CVAL:
+ write_sysreg(val, cntv_cval_el0);
break;
+ case ARCH_TIMER_REG_TVAL:
+ BUG();
}
}
@@ -115,13 +119,14 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
}
static __always_inline
-u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
+u64 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
{
if (access == ARCH_TIMER_PHYS_ACCESS) {
switch (reg) {
case ARCH_TIMER_REG_CTRL:
return read_sysreg(cntp_ctl_el0);
case ARCH_TIMER_REG_TVAL:
+ case ARCH_TIMER_REG_CVAL:
break;
}
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
@@ -129,6 +134,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
case ARCH_TIMER_REG_CTRL:
return read_sysreg(cntv_ctl_el0);
case ARCH_TIMER_REG_TVAL:
+ case ARCH_TIMER_REG_CVAL:
break;
}
}
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 0b2bac3ef7ce..898a07dc01cd 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -112,6 +112,8 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
case ARCH_TIMER_REG_TVAL:
writel_relaxed((u32)val, timer->base + CNTP_TVAL);
break;
+ case ARCH_TIMER_REG_CVAL:
+ BUG();
}
} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
struct arch_timer *timer = to_arch_timer(clk);
@@ -122,6 +124,8 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
case ARCH_TIMER_REG_TVAL:
writel_relaxed((u32)val, timer->base + CNTV_TVAL);
break;
+ case ARCH_TIMER_REG_CVAL:
+ BUG();
}
} else {
arch_timer_reg_write_cp15(access, reg, val);
@@ -141,6 +145,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
val = readl_relaxed(timer->base + CNTP_CTL);
break;
case ARCH_TIMER_REG_TVAL:
+ case ARCH_TIMER_REG_CVAL:
BUG();
}
} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
@@ -150,6 +155,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
val = readl_relaxed(timer->base + CNTV_CTL);
break;
case ARCH_TIMER_REG_TVAL:
+ case ARCH_TIMER_REG_CVAL:
BUG();
}
} else {
@@ -687,10 +693,18 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
struct clock_event_device *clk)
{
unsigned long ctrl;
+ u64 cnt;
+
ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
ctrl |= ARCH_TIMER_CTRL_ENABLE;
ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
- arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);
+
+ if (access == ARCH_TIMER_PHYS_ACCESS)
+ cnt = __arch_counter_get_cntpct();
+ else
+ cnt = __arch_counter_get_cntvct();
+
+ arch_timer_reg_write(access, ARCH_TIMER_REG_CVAL, evt + cnt, clk);
arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
}
@@ -708,17 +722,29 @@ static int arch_timer_set_next_event_phys(unsigned long evt,
return 0;
}
+static __always_inline void set_next_event_mem(const int access, unsigned long evt,
+ struct clock_event_device *clk)
+{
+ unsigned long ctrl;
+ ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
+ ctrl |= ARCH_TIMER_CTRL_ENABLE;
+ ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
+
+ arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);
+ arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
+}
+
static int arch_timer_set_next_event_virt_mem(unsigned long evt,
struct clock_event_device *clk)
{
- set_next_event(ARCH_TIMER_MEM_VIRT_ACCESS, evt, clk);
+ set_next_event_mem(ARCH_TIMER_MEM_VIRT_ACCESS, evt, clk);
return 0;
}
static int arch_timer_set_next_event_phys_mem(unsigned long evt,
struct clock_event_device *clk)
{
- set_next_event(ARCH_TIMER_MEM_PHYS_ACCESS, evt, clk);
+ set_next_event_mem(ARCH_TIMER_MEM_PHYS_ACCESS, evt, clk);
return 0;
}
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 73c7139c866f..d59537afb29d 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -25,6 +25,7 @@
enum arch_timer_reg {
ARCH_TIMER_REG_CTRL,
ARCH_TIMER_REG_TVAL,
+ ARCH_TIMER_REG_CVAL,
};
enum arch_timer_ppi_nr {
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 03/13] clocksource/arm_arch_timer: Move system register timer programming over to CVAL
2021-08-09 15:26 ` [PATCH 03/13] clocksource/arm_arch_timer: Move system register timer programming over to CVAL Marc Zyngier
@ 2021-08-11 7:15 ` Oliver Upton
2021-08-24 16:21 ` Mark Rutland
1 sibling, 0 replies; 39+ messages in thread
From: Oliver Upton @ 2021-08-11 7:15 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier <maz@kernel.org> wrote:
>
> In order to cope better with high frequency counters, move the
> programming of the timers from the countdown timer (TVAL) over
> to the comparator (CVAL).
>
> The programming model is slightly different, as we now need to
> read the current counter value to have an absolute deadline
> instead of a relative one.
>
> There is a small overhead to this change, which we will address
> in the following patches.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Reviewed-by: Oliver Upton <oupton@google.com>
> ---
> arch/arm/include/asm/arch_timer.h | 14 ++++++++----
> arch/arm64/include/asm/arch_timer.h | 16 +++++++++-----
> drivers/clocksource/arm_arch_timer.c | 32 +++++++++++++++++++++++++---
> include/clocksource/arm_arch_timer.h | 1 +
> 4 files changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 88075c7f4bfd..b48de9d26f27 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -31,18 +31,22 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> case ARCH_TIMER_REG_CTRL:
> asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" ((u32)val));
> break;
> - case ARCH_TIMER_REG_TVAL:
> - asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" ((u32)val));
> + case ARCH_TIMER_REG_CVAL:
> + asm volatile("mcrr p15, 2, %Q0, %R0, c14" : : "r" (val));
> break;
> + case ARCH_TIMER_REG_TVAL:
> + BUG();
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" ((u32)val));
> break;
> - case ARCH_TIMER_REG_TVAL:
> - asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" ((u32)val));
> + case ARCH_TIMER_REG_CVAL:
> + asm volatile("mcrr p15, 3, %Q0, %R0, c14" : : "r" (val));
> break;
> + case ARCH_TIMER_REG_TVAL:
> + BUG();
> }
> }
>
> @@ -60,6 +64,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
> break;
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> BUG();
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> @@ -68,6 +73,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
> break;
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> BUG();
> }
> }
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 107afb721749..6ceb050ae7b9 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -96,18 +96,22 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> case ARCH_TIMER_REG_CTRL:
> write_sysreg(val, cntp_ctl_el0);
> break;
> - case ARCH_TIMER_REG_TVAL:
> - write_sysreg(val, cntp_tval_el0);
> + case ARCH_TIMER_REG_CVAL:
> + write_sysreg(val, cntp_cval_el0);
> break;
> + case ARCH_TIMER_REG_TVAL:
> + BUG();
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> write_sysreg(val, cntv_ctl_el0);
> break;
> - case ARCH_TIMER_REG_TVAL:
> - write_sysreg(val, cntv_tval_el0);
> + case ARCH_TIMER_REG_CVAL:
> + write_sysreg(val, cntv_cval_el0);
> break;
> + case ARCH_TIMER_REG_TVAL:
> + BUG();
> }
> }
>
> @@ -115,13 +119,14 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> }
>
> static __always_inline
> -u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> +u64 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> {
> if (access == ARCH_TIMER_PHYS_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> return read_sysreg(cntp_ctl_el0);
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> break;
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> @@ -129,6 +134,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> case ARCH_TIMER_REG_CTRL:
> return read_sysreg(cntv_ctl_el0);
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> break;
> }
> }
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 0b2bac3ef7ce..898a07dc01cd 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -112,6 +112,8 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
> case ARCH_TIMER_REG_TVAL:
> writel_relaxed((u32)val, timer->base + CNTP_TVAL);
> break;
> + case ARCH_TIMER_REG_CVAL:
> + BUG();
> }
> } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
> struct arch_timer *timer = to_arch_timer(clk);
> @@ -122,6 +124,8 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
> case ARCH_TIMER_REG_TVAL:
> writel_relaxed((u32)val, timer->base + CNTV_TVAL);
> break;
> + case ARCH_TIMER_REG_CVAL:
> + BUG();
> }
> } else {
> arch_timer_reg_write_cp15(access, reg, val);
> @@ -141,6 +145,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
> val = readl_relaxed(timer->base + CNTP_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> BUG();
> }
> } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
> @@ -150,6 +155,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
> val = readl_relaxed(timer->base + CNTV_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> BUG();
> }
> } else {
> @@ -687,10 +693,18 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
> struct clock_event_device *clk)
> {
> unsigned long ctrl;
> + u64 cnt;
> +
> ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
> ctrl |= ARCH_TIMER_CTRL_ENABLE;
> ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
> - arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);
> +
> + if (access == ARCH_TIMER_PHYS_ACCESS)
> + cnt = __arch_counter_get_cntpct();
> + else
> + cnt = __arch_counter_get_cntvct();
> +
> + arch_timer_reg_write(access, ARCH_TIMER_REG_CVAL, evt + cnt, clk);
> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
> }
>
> @@ -708,17 +722,29 @@ static int arch_timer_set_next_event_phys(unsigned long evt,
> return 0;
> }
>
> +static __always_inline void set_next_event_mem(const int access, unsigned long evt,
> + struct clock_event_device *clk)
> +{
> + unsigned long ctrl;
> + ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
> + ctrl |= ARCH_TIMER_CTRL_ENABLE;
> + ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
> +
> + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);
> + arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
> +}
> +
> static int arch_timer_set_next_event_virt_mem(unsigned long evt,
> struct clock_event_device *clk)
> {
> - set_next_event(ARCH_TIMER_MEM_VIRT_ACCESS, evt, clk);
> + set_next_event_mem(ARCH_TIMER_MEM_VIRT_ACCESS, evt, clk);
> return 0;
> }
>
> static int arch_timer_set_next_event_phys_mem(unsigned long evt,
> struct clock_event_device *clk)
> {
> - set_next_event(ARCH_TIMER_MEM_PHYS_ACCESS, evt, clk);
> + set_next_event_mem(ARCH_TIMER_MEM_PHYS_ACCESS, evt, clk);
> return 0;
> }
>
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 73c7139c866f..d59537afb29d 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -25,6 +25,7 @@
> enum arch_timer_reg {
> ARCH_TIMER_REG_CTRL,
> ARCH_TIMER_REG_TVAL,
> + ARCH_TIMER_REG_CVAL,
> };
>
> enum arch_timer_ppi_nr {
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 03/13] clocksource/arm_arch_timer: Move system register timer programming over to CVAL
2021-08-09 15:26 ` [PATCH 03/13] clocksource/arm_arch_timer: Move system register timer programming over to CVAL Marc Zyngier
2021-08-11 7:15 ` Oliver Upton
@ 2021-08-24 16:21 ` Mark Rutland
1 sibling, 0 replies; 39+ messages in thread
From: Mark Rutland @ 2021-08-24 16:21 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Daniel Lezcano, Thomas Gleixner,
Peter Shier, Raghavendra Rao Ananta, Ricardo Koller,
Oliver Upton, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 09, 2021 at 04:26:41PM +0100, Marc Zyngier wrote:
> In order to cope better with high frequency counters, move the
> programming of the timers from the countdown timer (TVAL) over
> to the comparator (CVAL).
>
> The programming model is slightly different, as we now need to
> read the current counter value to have an absolute deadline
> instead of a relative one.
>
> There is a small overhead to this change, which we will address
> in the following patches.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Looks good, builds cleanly, and boots fine on both arm/arm64:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm/include/asm/arch_timer.h | 14 ++++++++----
> arch/arm64/include/asm/arch_timer.h | 16 +++++++++-----
> drivers/clocksource/arm_arch_timer.c | 32 +++++++++++++++++++++++++---
> include/clocksource/arm_arch_timer.h | 1 +
> 4 files changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
> index 88075c7f4bfd..b48de9d26f27 100644
> --- a/arch/arm/include/asm/arch_timer.h
> +++ b/arch/arm/include/asm/arch_timer.h
> @@ -31,18 +31,22 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> case ARCH_TIMER_REG_CTRL:
> asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" ((u32)val));
> break;
> - case ARCH_TIMER_REG_TVAL:
> - asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" ((u32)val));
> + case ARCH_TIMER_REG_CVAL:
> + asm volatile("mcrr p15, 2, %Q0, %R0, c14" : : "r" (val));
> break;
> + case ARCH_TIMER_REG_TVAL:
> + BUG();
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" ((u32)val));
> break;
> - case ARCH_TIMER_REG_TVAL:
> - asm volatile("mcr p15, 0, %0, c14, c3, 0" : : "r" ((u32)val));
> + case ARCH_TIMER_REG_CVAL:
> + asm volatile("mcrr p15, 3, %Q0, %R0, c14" : : "r" (val));
> break;
> + case ARCH_TIMER_REG_TVAL:
> + BUG();
> }
> }
>
> @@ -60,6 +64,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
> break;
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> BUG();
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> @@ -68,6 +73,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
> break;
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> BUG();
> }
> }
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 107afb721749..6ceb050ae7b9 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -96,18 +96,22 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> case ARCH_TIMER_REG_CTRL:
> write_sysreg(val, cntp_ctl_el0);
> break;
> - case ARCH_TIMER_REG_TVAL:
> - write_sysreg(val, cntp_tval_el0);
> + case ARCH_TIMER_REG_CVAL:
> + write_sysreg(val, cntp_cval_el0);
> break;
> + case ARCH_TIMER_REG_TVAL:
> + BUG();
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> write_sysreg(val, cntv_ctl_el0);
> break;
> - case ARCH_TIMER_REG_TVAL:
> - write_sysreg(val, cntv_tval_el0);
> + case ARCH_TIMER_REG_CVAL:
> + write_sysreg(val, cntv_cval_el0);
> break;
> + case ARCH_TIMER_REG_TVAL:
> + BUG();
> }
> }
>
> @@ -115,13 +119,14 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
> }
>
> static __always_inline
> -u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> +u64 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> {
> if (access == ARCH_TIMER_PHYS_ACCESS) {
> switch (reg) {
> case ARCH_TIMER_REG_CTRL:
> return read_sysreg(cntp_ctl_el0);
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> break;
> }
> } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> @@ -129,6 +134,7 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
> case ARCH_TIMER_REG_CTRL:
> return read_sysreg(cntv_ctl_el0);
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> break;
> }
> }
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 0b2bac3ef7ce..898a07dc01cd 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -112,6 +112,8 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
> case ARCH_TIMER_REG_TVAL:
> writel_relaxed((u32)val, timer->base + CNTP_TVAL);
> break;
> + case ARCH_TIMER_REG_CVAL:
> + BUG();
> }
> } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
> struct arch_timer *timer = to_arch_timer(clk);
> @@ -122,6 +124,8 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
> case ARCH_TIMER_REG_TVAL:
> writel_relaxed((u32)val, timer->base + CNTV_TVAL);
> break;
> + case ARCH_TIMER_REG_CVAL:
> + BUG();
> }
> } else {
> arch_timer_reg_write_cp15(access, reg, val);
> @@ -141,6 +145,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
> val = readl_relaxed(timer->base + CNTP_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> BUG();
> }
> } else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
> @@ -150,6 +155,7 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
> val = readl_relaxed(timer->base + CNTV_CTL);
> break;
> case ARCH_TIMER_REG_TVAL:
> + case ARCH_TIMER_REG_CVAL:
> BUG();
> }
> } else {
> @@ -687,10 +693,18 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
> struct clock_event_device *clk)
> {
> unsigned long ctrl;
> + u64 cnt;
> +
> ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
> ctrl |= ARCH_TIMER_CTRL_ENABLE;
> ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
> - arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);
> +
> + if (access == ARCH_TIMER_PHYS_ACCESS)
> + cnt = __arch_counter_get_cntpct();
> + else
> + cnt = __arch_counter_get_cntvct();
> +
> + arch_timer_reg_write(access, ARCH_TIMER_REG_CVAL, evt + cnt, clk);
> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
> }
>
> @@ -708,17 +722,29 @@ static int arch_timer_set_next_event_phys(unsigned long evt,
> return 0;
> }
>
> +static __always_inline void set_next_event_mem(const int access, unsigned long evt,
> + struct clock_event_device *clk)
> +{
> + unsigned long ctrl;
> + ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
> + ctrl |= ARCH_TIMER_CTRL_ENABLE;
> + ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
> +
> + arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);
> + arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
> +}
> +
> static int arch_timer_set_next_event_virt_mem(unsigned long evt,
> struct clock_event_device *clk)
> {
> - set_next_event(ARCH_TIMER_MEM_VIRT_ACCESS, evt, clk);
> + set_next_event_mem(ARCH_TIMER_MEM_VIRT_ACCESS, evt, clk);
> return 0;
> }
>
> static int arch_timer_set_next_event_phys_mem(unsigned long evt,
> struct clock_event_device *clk)
> {
> - set_next_event(ARCH_TIMER_MEM_PHYS_ACCESS, evt, clk);
> + set_next_event_mem(ARCH_TIMER_MEM_PHYS_ACCESS, evt, clk);
> return 0;
> }
>
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 73c7139c866f..d59537afb29d 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -25,6 +25,7 @@
> enum arch_timer_reg {
> ARCH_TIMER_REG_CTRL,
> ARCH_TIMER_REG_TVAL,
> + ARCH_TIMER_REG_CVAL,
> };
>
> enum arch_timer_ppi_nr {
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 04/13] clocksource/arm_arch_timer: Move drop _tval from erratum function names
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (2 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 03/13] clocksource/arm_arch_timer: Move system register timer programming over to CVAL Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 16:16 ` Oliver Upton
2021-08-24 16:29 ` Mark Rutland
2021-08-09 15:26 ` [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue Marc Zyngier
` (8 subsequent siblings)
12 siblings, 2 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
The '_tval' name in the erratum handling function names doesn't
make much sense anymore (and they were using CVAL the first place).
Drop the _tval tag.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
drivers/clocksource/arm_arch_timer.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 898a07dc01cd..160464f75017 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -371,7 +371,7 @@ EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
-static void erratum_set_next_event_tval_generic(const int access, unsigned long evt,
+static void erratum_set_next_event_generic(const int access, unsigned long evt,
struct clock_event_device *clk)
{
unsigned long ctrl;
@@ -392,17 +392,17 @@ static void erratum_set_next_event_tval_generic(const int access, unsigned long
arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
}
-static __maybe_unused int erratum_set_next_event_tval_virt(unsigned long evt,
+static __maybe_unused int erratum_set_next_event_virt(unsigned long evt,
struct clock_event_device *clk)
{
- erratum_set_next_event_tval_generic(ARCH_TIMER_VIRT_ACCESS, evt, clk);
+ erratum_set_next_event_generic(ARCH_TIMER_VIRT_ACCESS, evt, clk);
return 0;
}
-static __maybe_unused int erratum_set_next_event_tval_phys(unsigned long evt,
+static __maybe_unused int erratum_set_next_event_phys(unsigned long evt,
struct clock_event_device *clk)
{
- erratum_set_next_event_tval_generic(ARCH_TIMER_PHYS_ACCESS, evt, clk);
+ erratum_set_next_event_generic(ARCH_TIMER_PHYS_ACCESS, evt, clk);
return 0;
}
@@ -414,8 +414,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
.desc = "Freescale erratum a005858",
.read_cntpct_el0 = fsl_a008585_read_cntpct_el0,
.read_cntvct_el0 = fsl_a008585_read_cntvct_el0,
- .set_next_event_phys = erratum_set_next_event_tval_phys,
- .set_next_event_virt = erratum_set_next_event_tval_virt,
+ .set_next_event_phys = erratum_set_next_event_phys,
+ .set_next_event_virt = erratum_set_next_event_virt,
},
#endif
#ifdef CONFIG_HISILICON_ERRATUM_161010101
@@ -425,8 +425,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
.desc = "HiSilicon erratum 161010101",
.read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
.read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
- .set_next_event_phys = erratum_set_next_event_tval_phys,
- .set_next_event_virt = erratum_set_next_event_tval_virt,
+ .set_next_event_phys = erratum_set_next_event_phys,
+ .set_next_event_virt = erratum_set_next_event_virt,
},
{
.match_type = ate_match_acpi_oem_info,
@@ -434,8 +434,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
.desc = "HiSilicon erratum 161010101",
.read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
.read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
- .set_next_event_phys = erratum_set_next_event_tval_phys,
- .set_next_event_virt = erratum_set_next_event_tval_virt,
+ .set_next_event_phys = erratum_set_next_event_phys,
+ .set_next_event_virt = erratum_set_next_event_virt,
},
#endif
#ifdef CONFIG_ARM64_ERRATUM_858921
@@ -454,8 +454,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
.desc = "Allwinner erratum UNKNOWN1",
.read_cntpct_el0 = sun50i_a64_read_cntpct_el0,
.read_cntvct_el0 = sun50i_a64_read_cntvct_el0,
- .set_next_event_phys = erratum_set_next_event_tval_phys,
- .set_next_event_virt = erratum_set_next_event_tval_virt,
+ .set_next_event_phys = erratum_set_next_event_phys,
+ .set_next_event_virt = erratum_set_next_event_virt,
},
#endif
#ifdef CONFIG_ARM64_ERRATUM_1418040
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 04/13] clocksource/arm_arch_timer: Move drop _tval from erratum function names
2021-08-09 15:26 ` [PATCH 04/13] clocksource/arm_arch_timer: Move drop _tval from erratum function names Marc Zyngier
@ 2021-08-09 16:16 ` Oliver Upton
2021-08-24 16:29 ` Mark Rutland
1 sibling, 0 replies; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 16:16 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier <maz@kernel.org> wrote:
>
> The '_tval' name in the erratum handling function names doesn't
> make much sense anymore (and they were using CVAL the first place).
>
> Drop the _tval tag.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Reviewed-by: Oliver Upton <oupton@google.com>
> ---
> drivers/clocksource/arm_arch_timer.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 898a07dc01cd..160464f75017 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -371,7 +371,7 @@ EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>
> static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
>
> -static void erratum_set_next_event_tval_generic(const int access, unsigned long evt,
> +static void erratum_set_next_event_generic(const int access, unsigned long evt,
> struct clock_event_device *clk)
> {
> unsigned long ctrl;
> @@ -392,17 +392,17 @@ static void erratum_set_next_event_tval_generic(const int access, unsigned long
> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
> }
>
> -static __maybe_unused int erratum_set_next_event_tval_virt(unsigned long evt,
> +static __maybe_unused int erratum_set_next_event_virt(unsigned long evt,
> struct clock_event_device *clk)
> {
> - erratum_set_next_event_tval_generic(ARCH_TIMER_VIRT_ACCESS, evt, clk);
> + erratum_set_next_event_generic(ARCH_TIMER_VIRT_ACCESS, evt, clk);
> return 0;
> }
>
> -static __maybe_unused int erratum_set_next_event_tval_phys(unsigned long evt,
> +static __maybe_unused int erratum_set_next_event_phys(unsigned long evt,
> struct clock_event_device *clk)
> {
> - erratum_set_next_event_tval_generic(ARCH_TIMER_PHYS_ACCESS, evt, clk);
> + erratum_set_next_event_generic(ARCH_TIMER_PHYS_ACCESS, evt, clk);
> return 0;
> }
>
> @@ -414,8 +414,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .desc = "Freescale erratum a005858",
> .read_cntpct_el0 = fsl_a008585_read_cntpct_el0,
> .read_cntvct_el0 = fsl_a008585_read_cntvct_el0,
> - .set_next_event_phys = erratum_set_next_event_tval_phys,
> - .set_next_event_virt = erratum_set_next_event_tval_virt,
> + .set_next_event_phys = erratum_set_next_event_phys,
> + .set_next_event_virt = erratum_set_next_event_virt,
> },
> #endif
> #ifdef CONFIG_HISILICON_ERRATUM_161010101
> @@ -425,8 +425,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .desc = "HiSilicon erratum 161010101",
> .read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
> .read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
> - .set_next_event_phys = erratum_set_next_event_tval_phys,
> - .set_next_event_virt = erratum_set_next_event_tval_virt,
> + .set_next_event_phys = erratum_set_next_event_phys,
> + .set_next_event_virt = erratum_set_next_event_virt,
> },
> {
> .match_type = ate_match_acpi_oem_info,
> @@ -434,8 +434,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .desc = "HiSilicon erratum 161010101",
> .read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
> .read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
> - .set_next_event_phys = erratum_set_next_event_tval_phys,
> - .set_next_event_virt = erratum_set_next_event_tval_virt,
> + .set_next_event_phys = erratum_set_next_event_phys,
> + .set_next_event_virt = erratum_set_next_event_virt,
> },
> #endif
> #ifdef CONFIG_ARM64_ERRATUM_858921
> @@ -454,8 +454,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .desc = "Allwinner erratum UNKNOWN1",
> .read_cntpct_el0 = sun50i_a64_read_cntpct_el0,
> .read_cntvct_el0 = sun50i_a64_read_cntvct_el0,
> - .set_next_event_phys = erratum_set_next_event_tval_phys,
> - .set_next_event_virt = erratum_set_next_event_tval_virt,
> + .set_next_event_phys = erratum_set_next_event_phys,
> + .set_next_event_virt = erratum_set_next_event_virt,
> },
> #endif
> #ifdef CONFIG_ARM64_ERRATUM_1418040
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 04/13] clocksource/arm_arch_timer: Move drop _tval from erratum function names
2021-08-09 15:26 ` [PATCH 04/13] clocksource/arm_arch_timer: Move drop _tval from erratum function names Marc Zyngier
2021-08-09 16:16 ` Oliver Upton
@ 2021-08-24 16:29 ` Mark Rutland
1 sibling, 0 replies; 39+ messages in thread
From: Mark Rutland @ 2021-08-24 16:29 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Daniel Lezcano, Thomas Gleixner,
Peter Shier, Raghavendra Rao Ananta, Ricardo Koller,
Oliver Upton, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 09, 2021 at 04:26:42PM +0100, Marc Zyngier wrote:
> The '_tval' name in the erratum handling function names doesn't
> make much sense anymore (and they were using CVAL the first place).
>
> Drop the _tval tag.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Looks good, builds cleanly, and boots fine on both arm/arm64:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> drivers/clocksource/arm_arch_timer.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 898a07dc01cd..160464f75017 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -371,7 +371,7 @@ EXPORT_SYMBOL_GPL(timer_unstable_counter_workaround);
>
> static atomic_t timer_unstable_counter_workaround_in_use = ATOMIC_INIT(0);
>
> -static void erratum_set_next_event_tval_generic(const int access, unsigned long evt,
> +static void erratum_set_next_event_generic(const int access, unsigned long evt,
> struct clock_event_device *clk)
> {
> unsigned long ctrl;
> @@ -392,17 +392,17 @@ static void erratum_set_next_event_tval_generic(const int access, unsigned long
> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
> }
>
> -static __maybe_unused int erratum_set_next_event_tval_virt(unsigned long evt,
> +static __maybe_unused int erratum_set_next_event_virt(unsigned long evt,
> struct clock_event_device *clk)
> {
> - erratum_set_next_event_tval_generic(ARCH_TIMER_VIRT_ACCESS, evt, clk);
> + erratum_set_next_event_generic(ARCH_TIMER_VIRT_ACCESS, evt, clk);
> return 0;
> }
>
> -static __maybe_unused int erratum_set_next_event_tval_phys(unsigned long evt,
> +static __maybe_unused int erratum_set_next_event_phys(unsigned long evt,
> struct clock_event_device *clk)
> {
> - erratum_set_next_event_tval_generic(ARCH_TIMER_PHYS_ACCESS, evt, clk);
> + erratum_set_next_event_generic(ARCH_TIMER_PHYS_ACCESS, evt, clk);
> return 0;
> }
>
> @@ -414,8 +414,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .desc = "Freescale erratum a005858",
> .read_cntpct_el0 = fsl_a008585_read_cntpct_el0,
> .read_cntvct_el0 = fsl_a008585_read_cntvct_el0,
> - .set_next_event_phys = erratum_set_next_event_tval_phys,
> - .set_next_event_virt = erratum_set_next_event_tval_virt,
> + .set_next_event_phys = erratum_set_next_event_phys,
> + .set_next_event_virt = erratum_set_next_event_virt,
> },
> #endif
> #ifdef CONFIG_HISILICON_ERRATUM_161010101
> @@ -425,8 +425,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .desc = "HiSilicon erratum 161010101",
> .read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
> .read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
> - .set_next_event_phys = erratum_set_next_event_tval_phys,
> - .set_next_event_virt = erratum_set_next_event_tval_virt,
> + .set_next_event_phys = erratum_set_next_event_phys,
> + .set_next_event_virt = erratum_set_next_event_virt,
> },
> {
> .match_type = ate_match_acpi_oem_info,
> @@ -434,8 +434,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .desc = "HiSilicon erratum 161010101",
> .read_cntpct_el0 = hisi_161010101_read_cntpct_el0,
> .read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
> - .set_next_event_phys = erratum_set_next_event_tval_phys,
> - .set_next_event_virt = erratum_set_next_event_tval_virt,
> + .set_next_event_phys = erratum_set_next_event_phys,
> + .set_next_event_virt = erratum_set_next_event_virt,
> },
> #endif
> #ifdef CONFIG_ARM64_ERRATUM_858921
> @@ -454,8 +454,8 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
> .desc = "Allwinner erratum UNKNOWN1",
> .read_cntpct_el0 = sun50i_a64_read_cntpct_el0,
> .read_cntvct_el0 = sun50i_a64_read_cntvct_el0,
> - .set_next_event_phys = erratum_set_next_event_tval_phys,
> - .set_next_event_virt = erratum_set_next_event_tval_virt,
> + .set_next_event_phys = erratum_set_next_event_phys,
> + .set_next_event_virt = erratum_set_next_event_virt,
> },
> #endif
> #ifdef CONFIG_ARM64_ERRATUM_1418040
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (3 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 04/13] clocksource/arm_arch_timer: Move drop _tval from erratum function names Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 16:52 ` Oliver Upton
2021-08-24 16:44 ` Mark Rutland
2021-08-09 15:26 ` [PATCH 06/13] clocksource/arm_arch_timer: Move MMIO timer programming over to CVAL Marc Zyngier
` (7 subsequent siblings)
12 siblings, 2 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
The MMIO timer base address gets published after we have registered
the callbacks and the interrupt handler, which is... a bit dangerous.
Fix this by moving the base address publication to the point where
we register the timer, and expose a pointer to the timer structure
itself rather than a naked value.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
drivers/clocksource/arm_arch_timer.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 160464f75017..ca7761d8459a 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -54,13 +54,13 @@
static unsigned arch_timers_present __initdata;
-static void __iomem *arch_counter_base __ro_after_init;
-
struct arch_timer {
void __iomem *base;
struct clock_event_device evt;
};
+static struct arch_timer *arch_timer_mem __ro_after_init;
+
#define to_arch_timer(e) container_of(e, struct arch_timer, evt)
static u32 arch_timer_rate __ro_after_init;
@@ -975,9 +975,9 @@ static u64 arch_counter_get_cntvct_mem(void)
u32 vct_lo, vct_hi, tmp_hi;
do {
- vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
- vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO);
- tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
+ vct_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
+ vct_lo = readl_relaxed(arch_timer_mem->base + CNTVCT_LO);
+ tmp_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
} while (vct_hi != tmp_hi);
return ((u64) vct_hi << 32) | vct_lo;
@@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
{
int ret;
irq_handler_t func;
- struct arch_timer *t;
- t = kzalloc(sizeof(*t), GFP_KERNEL);
- if (!t)
+ arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL);
+ if (!arch_timer_mem)
return -ENOMEM;
- t->base = base;
- t->evt.irq = irq;
- __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &t->evt);
+ arch_timer_mem->base = base;
+ arch_timer_mem->evt.irq = irq;
+ __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &arch_timer_mem->evt);
if (arch_timer_mem_use_virtual)
func = arch_timer_handler_virt_mem;
else
func = arch_timer_handler_phys_mem;
- ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &t->evt);
+ ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt);
if (ret) {
pr_err("Failed to request mem timer irq\n");
- kfree(t);
+ kfree(arch_timer_mem);
+ arch_timer_mem = NULL;
}
return ret;
@@ -1444,7 +1444,6 @@ arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame)
return ret;
}
- arch_counter_base = base;
arch_timers_present |= ARCH_TIMER_TYPE_MEM;
return 0;
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue
2021-08-09 15:26 ` [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue Marc Zyngier
@ 2021-08-09 16:52 ` Oliver Upton
2021-08-10 8:27 ` Marc Zyngier
2021-08-24 16:44 ` Mark Rutland
1 sibling, 1 reply; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 16:52 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier <maz@kernel.org> wrote:
>
> The MMIO timer base address gets published after we have registered
> the callbacks and the interrupt handler, which is... a bit dangerous.
>
> Fix this by moving the base address publication to the point where
> we register the timer, and expose a pointer to the timer structure
> itself rather than a naked value.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Is this patch stable-worthy? I take it there haven't been any reports
of issues, though this seems rather perilous.
Reviewed-by: Oliver Upton <oupton@google.com>
> ---
> drivers/clocksource/arm_arch_timer.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 160464f75017..ca7761d8459a 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -54,13 +54,13 @@
>
> static unsigned arch_timers_present __initdata;
>
> -static void __iomem *arch_counter_base __ro_after_init;
> -
> struct arch_timer {
> void __iomem *base;
> struct clock_event_device evt;
> };
>
> +static struct arch_timer *arch_timer_mem __ro_after_init;
> +
> #define to_arch_timer(e) container_of(e, struct arch_timer, evt)
>
> static u32 arch_timer_rate __ro_after_init;
> @@ -975,9 +975,9 @@ static u64 arch_counter_get_cntvct_mem(void)
> u32 vct_lo, vct_hi, tmp_hi;
>
> do {
> - vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> - vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO);
> - tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> + vct_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
> + vct_lo = readl_relaxed(arch_timer_mem->base + CNTVCT_LO);
> + tmp_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
> } while (vct_hi != tmp_hi);
>
> return ((u64) vct_hi << 32) | vct_lo;
> @@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
> {
> int ret;
> irq_handler_t func;
> - struct arch_timer *t;
>
> - t = kzalloc(sizeof(*t), GFP_KERNEL);
> - if (!t)
> + arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL);
> + if (!arch_timer_mem)
> return -ENOMEM;
>
> - t->base = base;
> - t->evt.irq = irq;
> - __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &t->evt);
> + arch_timer_mem->base = base;
> + arch_timer_mem->evt.irq = irq;
> + __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &arch_timer_mem->evt);
>
> if (arch_timer_mem_use_virtual)
> func = arch_timer_handler_virt_mem;
> else
> func = arch_timer_handler_phys_mem;
>
> - ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &t->evt);
> + ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt);
> if (ret) {
> pr_err("Failed to request mem timer irq\n");
> - kfree(t);
> + kfree(arch_timer_mem);
> + arch_timer_mem = NULL;
> }
>
> return ret;
> @@ -1444,7 +1444,6 @@ arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame)
> return ret;
> }
>
> - arch_counter_base = base;
> arch_timers_present |= ARCH_TIMER_TYPE_MEM;
>
> return 0;
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue
2021-08-09 16:52 ` Oliver Upton
@ 2021-08-10 8:27 ` Marc Zyngier
0 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-10 8:27 UTC (permalink / raw)
To: Oliver Upton
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, 09 Aug 2021 17:52:00 +0100,
Oliver Upton <oupton@google.com> wrote:
>
> On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > The MMIO timer base address gets published after we have registered
> > the callbacks and the interrupt handler, which is... a bit dangerous.
> >
> > Fix this by moving the base address publication to the point where
> > we register the timer, and expose a pointer to the timer structure
> > itself rather than a naked value.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
>
> Is this patch stable-worthy? I take it there haven't been any reports
> of issues, though this seems rather perilous.
It *could* deserve a Cc stable, although I suspect it doesn't easily
fall over with the current code:
- When programming a timer, the driver uses the base contained in
struct arch_timer, and derived from the clock_event_device.
- As long as you don't need to read the counter, you are good (the
whole point of using TVAL is that you avoid reading the counter).
It is only if someone called into the standalone counter accessor that
there would be a firework, and that's rather unlikely.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue
2021-08-09 15:26 ` [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue Marc Zyngier
2021-08-09 16:52 ` Oliver Upton
@ 2021-08-24 16:44 ` Mark Rutland
1 sibling, 0 replies; 39+ messages in thread
From: Mark Rutland @ 2021-08-24 16:44 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Daniel Lezcano, Thomas Gleixner,
Peter Shier, Raghavendra Rao Ananta, Ricardo Koller,
Oliver Upton, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 09, 2021 at 04:26:43PM +0100, Marc Zyngier wrote:
> The MMIO timer base address gets published after we have registered
> the callbacks and the interrupt handler, which is... a bit dangerous.
>
> Fix this by moving the base address publication to the point where
> we register the timer, and expose a pointer to the timer structure
> itself rather than a naked value.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
I don't have agood setup to test this with, but this looks good to me,
and builds cleanly for arm/arm64, so:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> drivers/clocksource/arm_arch_timer.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 160464f75017..ca7761d8459a 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -54,13 +54,13 @@
>
> static unsigned arch_timers_present __initdata;
>
> -static void __iomem *arch_counter_base __ro_after_init;
> -
> struct arch_timer {
> void __iomem *base;
> struct clock_event_device evt;
> };
>
> +static struct arch_timer *arch_timer_mem __ro_after_init;
> +
> #define to_arch_timer(e) container_of(e, struct arch_timer, evt)
>
> static u32 arch_timer_rate __ro_after_init;
> @@ -975,9 +975,9 @@ static u64 arch_counter_get_cntvct_mem(void)
> u32 vct_lo, vct_hi, tmp_hi;
>
> do {
> - vct_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> - vct_lo = readl_relaxed(arch_counter_base + CNTVCT_LO);
> - tmp_hi = readl_relaxed(arch_counter_base + CNTVCT_HI);
> + vct_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
> + vct_lo = readl_relaxed(arch_timer_mem->base + CNTVCT_LO);
> + tmp_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
> } while (vct_hi != tmp_hi);
>
> return ((u64) vct_hi << 32) | vct_lo;
> @@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
> {
> int ret;
> irq_handler_t func;
> - struct arch_timer *t;
>
> - t = kzalloc(sizeof(*t), GFP_KERNEL);
> - if (!t)
> + arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL);
> + if (!arch_timer_mem)
> return -ENOMEM;
>
> - t->base = base;
> - t->evt.irq = irq;
> - __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &t->evt);
> + arch_timer_mem->base = base;
> + arch_timer_mem->evt.irq = irq;
> + __arch_timer_setup(ARCH_TIMER_TYPE_MEM, &arch_timer_mem->evt);
>
> if (arch_timer_mem_use_virtual)
> func = arch_timer_handler_virt_mem;
> else
> func = arch_timer_handler_phys_mem;
>
> - ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &t->evt);
> + ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt);
> if (ret) {
> pr_err("Failed to request mem timer irq\n");
> - kfree(t);
> + kfree(arch_timer_mem);
> + arch_timer_mem = NULL;
> }
>
> return ret;
> @@ -1444,7 +1444,6 @@ arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame)
> return ret;
> }
>
> - arch_counter_base = base;
> arch_timers_present |= ARCH_TIMER_TYPE_MEM;
>
> return 0;
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 06/13] clocksource/arm_arch_timer: Move MMIO timer programming over to CVAL
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (4 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 05/13] clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering issue Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 15:26 ` [PATCH 07/13] clocksource/arm_arch_timer: Advertise 56bit timer to the core code Marc Zyngier
` (6 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
Similarily to the sysreg-based timer, move the MMIO over to using
the CVAL registers instead of TVAL. Note that there is no warranty
that the 64bit MMIO access will be atomic, but the timer is always
disabled at the point where we program CVAL.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm/include/asm/arch_timer.h | 1 +
drivers/clocksource/arm_arch_timer.c | 50 ++++++++++++++++++++--------
2 files changed, 37 insertions(+), 14 deletions(-)
diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index b48de9d26f27..d21a7f788d7b 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -7,6 +7,7 @@
#include <asm/hwcap.h>
#include <linux/clocksource.h>
#include <linux/init.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
#include <linux/types.h>
#include <clocksource/arm_arch_timer.h>
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index ca7761d8459a..29544b16edf3 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -44,11 +44,13 @@
#define CNTACR_RWVT BIT(4)
#define CNTACR_RWPT BIT(5)
-#define CNTVCT_LO 0x08
-#define CNTVCT_HI 0x0c
+#define CNTVCT_LO 0x00
+#define CNTPCT_LO 0x08
#define CNTFRQ 0x10
+#define CNTP_CVAL_LO 0x20
#define CNTP_TVAL 0x28
#define CNTP_CTL 0x2c
+#define CNTV_CVAL_LO 0x30
#define CNTV_TVAL 0x38
#define CNTV_CTL 0x3c
@@ -113,7 +115,12 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
writel_relaxed((u32)val, timer->base + CNTP_TVAL);
break;
case ARCH_TIMER_REG_CVAL:
- BUG();
+ /*
+ * Not guaranteed to be atomic, so the timer
+ * must be disabled at this point.
+ */
+ writeq_relaxed(val, timer->base + CNTP_CVAL_LO);
+ break;
}
} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
struct arch_timer *timer = to_arch_timer(clk);
@@ -125,7 +132,9 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
writel_relaxed((u32)val, timer->base + CNTV_TVAL);
break;
case ARCH_TIMER_REG_CVAL:
- BUG();
+ /* Same restriction as above */
+ writeq_relaxed(val, timer->base + CNTV_CVAL_LO);
+ break;
}
} else {
arch_timer_reg_write_cp15(access, reg, val);
@@ -722,15 +731,36 @@ static int arch_timer_set_next_event_phys(unsigned long evt,
return 0;
}
+static u64 arch_counter_get_cnt_mem(struct arch_timer *t, int offset_lo)
+{
+ u32 cnt_lo, cnt_hi, tmp_hi;
+
+ do {
+ cnt_hi = readl_relaxed(t->base + offset_lo + 4);
+ cnt_lo = readl_relaxed(t->base + offset_lo);
+ tmp_hi = readl_relaxed(t->base + offset_lo + 4);
+ } while (cnt_hi != tmp_hi);
+
+ return ((u64) cnt_hi << 32) | cnt_lo;
+}
+
static __always_inline void set_next_event_mem(const int access, unsigned long evt,
struct clock_event_device *clk)
{
+ struct arch_timer *timer = to_arch_timer(clk);
unsigned long ctrl;
+ u64 cnt;
+
ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
ctrl |= ARCH_TIMER_CTRL_ENABLE;
ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
- arch_timer_reg_write(access, ARCH_TIMER_REG_TVAL, evt, clk);
+ if (access == ARCH_TIMER_MEM_VIRT_ACCESS)
+ cnt = arch_counter_get_cnt_mem(timer, CNTVCT_LO);
+ else
+ cnt = arch_counter_get_cnt_mem(timer, CNTPCT_LO);
+
+ arch_timer_reg_write(access, ARCH_TIMER_REG_CVAL, evt + cnt, clk);
arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
}
@@ -972,15 +1002,7 @@ bool arch_timer_evtstrm_available(void)
static u64 arch_counter_get_cntvct_mem(void)
{
- u32 vct_lo, vct_hi, tmp_hi;
-
- do {
- vct_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
- vct_lo = readl_relaxed(arch_timer_mem->base + CNTVCT_LO);
- tmp_hi = readl_relaxed(arch_timer_mem->base + CNTVCT_HI);
- } while (vct_hi != tmp_hi);
-
- return ((u64) vct_hi << 32) | vct_lo;
+ return arch_counter_get_cnt_mem(arch_timer_mem, CNTVCT_LO);
}
static struct arch_timer_kvm_info arch_timer_kvm_info;
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 07/13] clocksource/arm_arch_timer: Advertise 56bit timer to the core code
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (5 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 06/13] clocksource/arm_arch_timer: Move MMIO timer programming over to CVAL Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 15:26 ` [PATCH 08/13] clocksource/arm_arch_timer: Work around broken CVAL implementations Marc Zyngier
` (5 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
Proudly tell the code code that we have a timer able to handle
56 bits deltas.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
drivers/clocksource/arm_arch_timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 29544b16edf3..895844c33351 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -832,7 +832,7 @@ static void __arch_timer_setup(unsigned type,
clk->set_state_shutdown(clk);
- clockevents_config_and_register(clk, arch_timer_rate, 0xf, 0x7fffffff);
+ clockevents_config_and_register(clk, arch_timer_rate, 0xf, CLOCKSOURCE_MASK(56));
}
static void arch_timer_evtstrm_enable(int divider)
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 08/13] clocksource/arm_arch_timer: Work around broken CVAL implementations
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (6 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 07/13] clocksource/arm_arch_timer: Advertise 56bit timer to the core code Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-10 12:34 ` Mark Rutland
2021-08-09 15:26 ` [PATCH 09/13] clocksource/arm_arch_timer: Remove any trace of the TVAL programming interface Marc Zyngier
` (4 subsequent siblings)
12 siblings, 1 reply; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
The Applied Micro XGene-1 SoC has a busted implementation of the
CVAL register: it looks like it is based on TVAL instead of the
other way around. The net effect of this implementation blunder
is that the maximum deadline you can program in the timer is
32bit wide.
Detect the problematic case and limit the timer to 32bit deltas.
Note that we don't tie this bug to XGene specifically, as it may
also catch similar defects on other high-quality implementations.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
drivers/clocksource/arm_arch_timer.c | 38 +++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 895844c33351..1c596cd3cc5c 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -778,9 +778,42 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,
return 0;
}
+static u64 __arch_timer_check_delta(void)
+{
+#ifdef CONFIG_ARM64
+ u64 tmp;
+
+ /*
+ * XGene-1 implements CVAL in terms of TVAL, meaning that the
+ * maximum timer range is 32bit. Shame on them. Detect the
+ * issue by setting a timer to now+(1<<32), which will
+ * immediately fire on the duff CPU.
+ */
+ write_sysreg(0, cntv_ctl_el0);
+ isb();
+ tmp = read_sysreg(cntvct_el0) | BIT(32);
+ write_sysreg(tmp, cntv_cval_el0);
+ write_sysreg(ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_MASK,
+ cntv_ctl_el0);
+ isb();
+
+ tmp = read_sysreg(cntv_ctl_el0);
+ write_sysreg(0, cntv_ctl_el0);
+ isb();
+
+ if (tmp & ARCH_TIMER_CTRL_IT_STAT) {
+ pr_warn_once("Detected broken implementation, limiting width to 32bits");
+ return CLOCKSOURCE_MASK(32);
+ }
+#endif
+ return CLOCKSOURCE_MASK(56);
+}
+
static void __arch_timer_setup(unsigned type,
struct clock_event_device *clk)
{
+ u64 max_delta;
+
clk->features = CLOCK_EVT_FEAT_ONESHOT;
if (type == ARCH_TIMER_TYPE_CP15) {
@@ -812,6 +845,7 @@ static void __arch_timer_setup(unsigned type,
}
clk->set_next_event = sne;
+ max_delta = __arch_timer_check_delta();
} else {
clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
clk->name = "arch_mem_timer";
@@ -828,11 +862,13 @@ static void __arch_timer_setup(unsigned type,
clk->set_next_event =
arch_timer_set_next_event_phys_mem;
}
+
+ max_delta = CLOCKSOURCE_MASK(56);
}
clk->set_state_shutdown(clk);
- clockevents_config_and_register(clk, arch_timer_rate, 0xf, CLOCKSOURCE_MASK(56));
+ clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
}
static void arch_timer_evtstrm_enable(int divider)
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 08/13] clocksource/arm_arch_timer: Work around broken CVAL implementations
2021-08-09 15:26 ` [PATCH 08/13] clocksource/arm_arch_timer: Work around broken CVAL implementations Marc Zyngier
@ 2021-08-10 12:34 ` Mark Rutland
2021-08-10 13:15 ` Marc Zyngier
0 siblings, 1 reply; 39+ messages in thread
From: Mark Rutland @ 2021-08-10 12:34 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Daniel Lezcano, Thomas Gleixner,
Peter Shier, Raghavendra Rao Ananta, Ricardo Koller,
Oliver Upton, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 09, 2021 at 04:26:46PM +0100, Marc Zyngier wrote:
> The Applied Micro XGene-1 SoC has a busted implementation of the
> CVAL register: it looks like it is based on TVAL instead of the
> other way around. The net effect of this implementation blunder
> is that the maximum deadline you can program in the timer is
> 32bit wide.
>
> Detect the problematic case and limit the timer to 32bit deltas.
> Note that we don't tie this bug to XGene specifically, as it may
> also catch similar defects on other high-quality implementations.
Do we know of any other implementations that have a similar bug?
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> drivers/clocksource/arm_arch_timer.c | 38 +++++++++++++++++++++++++++-
> 1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 895844c33351..1c596cd3cc5c 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -778,9 +778,42 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,
> return 0;
> }
>
> +static u64 __arch_timer_check_delta(void)
> +{
> +#ifdef CONFIG_ARM64
> + u64 tmp;
> +
> + /*
> + * XGene-1 implements CVAL in terms of TVAL, meaning that the
> + * maximum timer range is 32bit. Shame on them. Detect the
> + * issue by setting a timer to now+(1<<32), which will
> + * immediately fire on the duff CPU.
> + */
> + write_sysreg(0, cntv_ctl_el0);
> + isb();
> + tmp = read_sysreg(cntvct_el0) | BIT(32);
> + write_sysreg(tmp, cntv_cval_el0);
This will fire on legitimate implementations fairly often. Consider if
we enter this function at a time where CNTCVT_EL0[32] == 1, where:
* At 100MHz, bit 32 flips every ~42.95
* At 200MHz, bit 32 flips every ~21.47
* At 1GHz, bit 32 flips every ~4.29s
... and ThunderX2 has a 200MHz frequency today, with SBSA recommending
100MHz.
What does XGene-1 return upon a read of CVAL? If it always returns 0 for
the high bits, we could do a timing-insensitive check for truncation of
CVAL, e.g.
| /* CVAL must be at least 56 bits wide, as with CNT */
| u64 mask = GENMASK(55, 0);
| u64 val;
|
| write_sysreg(mask, cntv_cval_el0);
| val = read_sysread(cnt_cval_el0);
|
| if (val != mask) {
| /* What a great CPU */
| }
Thanks,
Mark.
> + write_sysreg(ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_MASK,
> + cntv_ctl_el0);
> + isb();
> +
> + tmp = read_sysreg(cntv_ctl_el0);
> + write_sysreg(0, cntv_ctl_el0);
> + isb();
> +
> + if (tmp & ARCH_TIMER_CTRL_IT_STAT) {
> + pr_warn_once("Detected broken implementation, limiting width to 32bits");
> + return CLOCKSOURCE_MASK(32);
> + }
> +#endif
> + return CLOCKSOURCE_MASK(56);
> +}
> +
> static void __arch_timer_setup(unsigned type,
> struct clock_event_device *clk)
> {
> + u64 max_delta;
> +
> clk->features = CLOCK_EVT_FEAT_ONESHOT;
>
> if (type == ARCH_TIMER_TYPE_CP15) {
> @@ -812,6 +845,7 @@ static void __arch_timer_setup(unsigned type,
> }
>
> clk->set_next_event = sne;
> + max_delta = __arch_timer_check_delta();
> } else {
> clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
> clk->name = "arch_mem_timer";
> @@ -828,11 +862,13 @@ static void __arch_timer_setup(unsigned type,
> clk->set_next_event =
> arch_timer_set_next_event_phys_mem;
> }
> +
> + max_delta = CLOCKSOURCE_MASK(56);
> }
>
> clk->set_state_shutdown(clk);
>
> - clockevents_config_and_register(clk, arch_timer_rate, 0xf, CLOCKSOURCE_MASK(56));
> + clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
> }
>
> static void arch_timer_evtstrm_enable(int divider)
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 08/13] clocksource/arm_arch_timer: Work around broken CVAL implementations
2021-08-10 12:34 ` Mark Rutland
@ 2021-08-10 13:15 ` Marc Zyngier
0 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-10 13:15 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-arm-kernel, linux-kernel, Daniel Lezcano, Thomas Gleixner,
Peter Shier, Raghavendra Rao Ananta, Ricardo Koller,
Oliver Upton, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Tue, 10 Aug 2021 13:34:07 +0100,
Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Mon, Aug 09, 2021 at 04:26:46PM +0100, Marc Zyngier wrote:
> > The Applied Micro XGene-1 SoC has a busted implementation of the
> > CVAL register: it looks like it is based on TVAL instead of the
> > other way around. The net effect of this implementation blunder
> > is that the maximum deadline you can program in the timer is
> > 32bit wide.
> >
> > Detect the problematic case and limit the timer to 32bit deltas.
> > Note that we don't tie this bug to XGene specifically, as it may
> > also catch similar defects on other high-quality implementations.
>
> Do we know of any other implementations that have a similar bug?
>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > drivers/clocksource/arm_arch_timer.c | 38 +++++++++++++++++++++++++++-
> > 1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > index 895844c33351..1c596cd3cc5c 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -778,9 +778,42 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,
> > return 0;
> > }
> >
> > +static u64 __arch_timer_check_delta(void)
> > +{
> > +#ifdef CONFIG_ARM64
> > + u64 tmp;
> > +
> > + /*
> > + * XGene-1 implements CVAL in terms of TVAL, meaning that the
> > + * maximum timer range is 32bit. Shame on them. Detect the
> > + * issue by setting a timer to now+(1<<32), which will
> > + * immediately fire on the duff CPU.
> > + */
> > + write_sysreg(0, cntv_ctl_el0);
> > + isb();
> > + tmp = read_sysreg(cntvct_el0) | BIT(32);
> > + write_sysreg(tmp, cntv_cval_el0);
>
> This will fire on legitimate implementations fairly often. Consider if
> we enter this function at a time where CNTCVT_EL0[32] == 1, where:
>
> * At 100MHz, bit 32 flips every ~42.95
> * At 200MHz, bit 32 flips every ~21.47
> * At 1GHz, bit 32 flips every ~4.29s
>
> ... and ThunderX2 has a 200MHz frequency today, with SBSA recommending
> 100MHz.
Yup, you're right, this is silly. Orr-ing the bit is a bad enough bug
(it really should be a +), but also preemption in a guest will add
another set of false positives.
> What does XGene-1 return upon a read of CVAL? If it always returns 0 for
> the high bits, we could do a timing-insensitive check for truncation of
> CVAL, e.g.
>
> | /* CVAL must be at least 56 bits wide, as with CNT */
> | u64 mask = GENMASK(55, 0);
> | u64 val;
> |
> | write_sysreg(mask, cntv_cval_el0);
> | val = read_sysread(cnt_cval_el0);
> |
> | if (val != mask) {
> | /* What a great CPU */
> | }
No, the register itself returns what has been written. But only the
low 32bits of the delta trickle into TVAL on write, which is then used
as a countdown. I guess I could play the same trick as above with a
higher bit, but it still is pretty unreliable, as it could then wrap
through 0.
Maybe I'll just check the MIDR in the end...
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 09/13] clocksource/arm_arch_timer: Remove any trace of the TVAL programming interface
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (7 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 08/13] clocksource/arm_arch_timer: Work around broken CVAL implementations Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 15:26 ` [PATCH 10/13] clocksource/arm_arch_timer: Drop unnecessary ISB on CVAL programming Marc Zyngier
` (3 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
TVAL usage is now long gone, get rid of the leftovers.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm/include/asm/arch_timer.h | 6 ------
arch/arm64/include/asm/arch_timer.h | 6 ------
drivers/clocksource/arm_arch_timer.c | 10 ----------
include/clocksource/arm_arch_timer.h | 1 -
4 files changed, 23 deletions(-)
diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index d21a7f788d7b..f014630259cb 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -35,8 +35,6 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
case ARCH_TIMER_REG_CVAL:
asm volatile("mcrr p15, 2, %Q0, %R0, c14" : : "r" (val));
break;
- case ARCH_TIMER_REG_TVAL:
- BUG();
}
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
switch (reg) {
@@ -46,8 +44,6 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
case ARCH_TIMER_REG_CVAL:
asm volatile("mcrr p15, 3, %Q0, %R0, c14" : : "r" (val));
break;
- case ARCH_TIMER_REG_TVAL:
- BUG();
}
}
@@ -64,7 +60,6 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
case ARCH_TIMER_REG_CTRL:
asm volatile("mrc p15, 0, %0, c14, c2, 1" : "=r" (val));
break;
- case ARCH_TIMER_REG_TVAL:
case ARCH_TIMER_REG_CVAL:
BUG();
}
@@ -73,7 +68,6 @@ u32 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
case ARCH_TIMER_REG_CTRL:
asm volatile("mrc p15, 0, %0, c14, c3, 1" : "=r" (val));
break;
- case ARCH_TIMER_REG_TVAL:
case ARCH_TIMER_REG_CVAL:
BUG();
}
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 6ceb050ae7b9..861cafc4aca5 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -99,8 +99,6 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
case ARCH_TIMER_REG_CVAL:
write_sysreg(val, cntp_cval_el0);
break;
- case ARCH_TIMER_REG_TVAL:
- BUG();
}
} else if (access == ARCH_TIMER_VIRT_ACCESS) {
switch (reg) {
@@ -110,8 +108,6 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
case ARCH_TIMER_REG_CVAL:
write_sysreg(val, cntv_cval_el0);
break;
- case ARCH_TIMER_REG_TVAL:
- BUG();
}
}
@@ -125,7 +121,6 @@ u64 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
switch (reg) {
case ARCH_TIMER_REG_CTRL:
return read_sysreg(cntp_ctl_el0);
- case ARCH_TIMER_REG_TVAL:
case ARCH_TIMER_REG_CVAL:
break;
}
@@ -133,7 +128,6 @@ u64 arch_timer_reg_read_cp15(int access, enum arch_timer_reg reg)
switch (reg) {
case ARCH_TIMER_REG_CTRL:
return read_sysreg(cntv_ctl_el0);
- case ARCH_TIMER_REG_TVAL:
case ARCH_TIMER_REG_CVAL:
break;
}
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 1c596cd3cc5c..fa09952b94bf 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -48,10 +48,8 @@
#define CNTPCT_LO 0x08
#define CNTFRQ 0x10
#define CNTP_CVAL_LO 0x20
-#define CNTP_TVAL 0x28
#define CNTP_CTL 0x2c
#define CNTV_CVAL_LO 0x30
-#define CNTV_TVAL 0x38
#define CNTV_CTL 0x3c
static unsigned arch_timers_present __initdata;
@@ -111,9 +109,6 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
case ARCH_TIMER_REG_CTRL:
writel_relaxed((u32)val, timer->base + CNTP_CTL);
break;
- case ARCH_TIMER_REG_TVAL:
- writel_relaxed((u32)val, timer->base + CNTP_TVAL);
- break;
case ARCH_TIMER_REG_CVAL:
/*
* Not guaranteed to be atomic, so the timer
@@ -128,9 +123,6 @@ void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
case ARCH_TIMER_REG_CTRL:
writel_relaxed((u32)val, timer->base + CNTV_CTL);
break;
- case ARCH_TIMER_REG_TVAL:
- writel_relaxed((u32)val, timer->base + CNTV_TVAL);
- break;
case ARCH_TIMER_REG_CVAL:
/* Same restriction as above */
writeq_relaxed(val, timer->base + CNTV_CVAL_LO);
@@ -153,7 +145,6 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
case ARCH_TIMER_REG_CTRL:
val = readl_relaxed(timer->base + CNTP_CTL);
break;
- case ARCH_TIMER_REG_TVAL:
case ARCH_TIMER_REG_CVAL:
BUG();
}
@@ -163,7 +154,6 @@ u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
case ARCH_TIMER_REG_CTRL:
val = readl_relaxed(timer->base + CNTV_CTL);
break;
- case ARCH_TIMER_REG_TVAL:
case ARCH_TIMER_REG_CVAL:
BUG();
}
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index d59537afb29d..e715bdb720d5 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -24,7 +24,6 @@
enum arch_timer_reg {
ARCH_TIMER_REG_CTRL,
- ARCH_TIMER_REG_TVAL,
ARCH_TIMER_REG_CVAL,
};
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 10/13] clocksource/arm_arch_timer: Drop unnecessary ISB on CVAL programming
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (8 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 09/13] clocksource/arm_arch_timer: Remove any trace of the TVAL programming interface Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 15:26 ` [PATCH 11/13] clocksource/arm_arch_timer: Fix masking for high freq counters Marc Zyngier
` (2 subsequent siblings)
12 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
Switching from TVAL to CVAL has a small drawback: we need an ISB
before reading the counter. We cannot get rid of it, but we can
instead remove the one that comes just after writing to CVAL.
This reduces the number of ISBs from 3 to 2 when programming
the timer.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm/include/asm/arch_timer.h | 4 ++--
arch/arm64/include/asm/arch_timer.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/include/asm/arch_timer.h b/arch/arm/include/asm/arch_timer.h
index f014630259cb..787a98ed0716 100644
--- a/arch/arm/include/asm/arch_timer.h
+++ b/arch/arm/include/asm/arch_timer.h
@@ -31,6 +31,7 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
switch (reg) {
case ARCH_TIMER_REG_CTRL:
asm volatile("mcr p15, 0, %0, c14, c2, 1" : : "r" ((u32)val));
+ isb();
break;
case ARCH_TIMER_REG_CVAL:
asm volatile("mcrr p15, 2, %Q0, %R0, c14" : : "r" (val));
@@ -40,14 +41,13 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
switch (reg) {
case ARCH_TIMER_REG_CTRL:
asm volatile("mcr p15, 0, %0, c14, c3, 1" : : "r" ((u32)val));
+ isb();
break;
case ARCH_TIMER_REG_CVAL:
asm volatile("mcrr p15, 3, %Q0, %R0, c14" : : "r" (val));
break;
}
}
-
- isb();
}
static __always_inline
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 861cafc4aca5..b2f056db1225 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -95,6 +95,7 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
switch (reg) {
case ARCH_TIMER_REG_CTRL:
write_sysreg(val, cntp_ctl_el0);
+ isb();
break;
case ARCH_TIMER_REG_CVAL:
write_sysreg(val, cntp_cval_el0);
@@ -104,14 +105,13 @@ void arch_timer_reg_write_cp15(int access, enum arch_timer_reg reg, u64 val)
switch (reg) {
case ARCH_TIMER_REG_CTRL:
write_sysreg(val, cntv_ctl_el0);
+ isb();
break;
case ARCH_TIMER_REG_CVAL:
write_sysreg(val, cntv_cval_el0);
break;
}
}
-
- isb();
}
static __always_inline
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH 11/13] clocksource/arm_arch_timer: Fix masking for high freq counters
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (9 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 10/13] clocksource/arm_arch_timer: Drop unnecessary ISB on CVAL programming Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 16:45 ` Oliver Upton
2021-08-09 15:26 ` [PATCH 12/13] arm64: Add a capability for FEAT_EVC Marc Zyngier
2021-08-09 15:26 ` [PATCH 13/13] arm64: Add CNT{P,V}CTSS_EL0 alternatives to cnt{p,v}ct_el0 Marc Zyngier
12 siblings, 1 reply; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
From: Oliver Upton <oupton@google.com>
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 <oupton@google.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
[maz: fixed width computation not to lose the last bit, added
max delta generation for the timer]
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20210807191428.3488948-1-oupton@google.com
---
drivers/clocksource/arm_arch_timer.c | 34 ++++++++++++++++++++++++----
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index fa09952b94bf..74eca831d0d9 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -52,6 +52,12 @@
#define CNTV_CVAL_LO 0x30
#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;
struct arch_timer {
@@ -95,6 +101,22 @@ static int __init early_evtstrm_cfg(char *buf)
}
early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
+/*
+ * 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 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 - 1) + 1, 56, 64);
+}
+
/*
* Architected system timer support.
*/
@@ -208,13 +230,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 {
@@ -796,7 +816,7 @@ static u64 __arch_timer_check_delta(void)
return CLOCKSOURCE_MASK(32);
}
#endif
- return CLOCKSOURCE_MASK(56);
+ return CLOCKSOURCE_MASK(arch_counter_get_width());
}
static void __arch_timer_setup(unsigned type,
@@ -1041,6 +1061,7 @@ struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
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) {
@@ -1065,6 +1086,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();
@@ -1074,8 +1099,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.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 11/13] clocksource/arm_arch_timer: Fix masking for high freq counters
2021-08-09 15:26 ` [PATCH 11/13] clocksource/arm_arch_timer: Fix masking for high freq counters Marc Zyngier
@ 2021-08-09 16:45 ` Oliver Upton
2021-08-10 8:40 ` Marc Zyngier
0 siblings, 1 reply; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 16:45 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
>
> From: Oliver Upton <oupton@google.com>
>
> 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 <oupton@google.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> [maz: fixed width computation not to lose the last bit, added
> max delta generation for the timer]
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Link: https://lore.kernel.org/r/20210807191428.3488948-1-oupton@google.com
> ---
> drivers/clocksource/arm_arch_timer.c | 34 ++++++++++++++++++++++++----
> 1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index fa09952b94bf..74eca831d0d9 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -52,6 +52,12 @@
> #define CNTV_CVAL_LO 0x30
> #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;
>
> struct arch_timer {
> @@ -95,6 +101,22 @@ static int __init early_evtstrm_cfg(char *buf)
> }
> early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
>
> +/*
> + * 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 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 - 1) + 1, 56, 64);
> +}
Reposting thoughts from the original patch:
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 (40 years) than assume the
sanity of the system counter implementation.
--
Thanks,
Oliver
> +
> /*
> * Architected system timer support.
> */
> @@ -208,13 +230,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 {
> @@ -796,7 +816,7 @@ static u64 __arch_timer_check_delta(void)
> return CLOCKSOURCE_MASK(32);
> }
> #endif
> - return CLOCKSOURCE_MASK(56);
> + return CLOCKSOURCE_MASK(arch_counter_get_width());
> }
>
> static void __arch_timer_setup(unsigned type,
> @@ -1041,6 +1061,7 @@ struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
> 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) {
> @@ -1065,6 +1086,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();
> @@ -1074,8 +1099,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.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 11/13] clocksource/arm_arch_timer: Fix masking for high freq counters
2021-08-09 16:45 ` Oliver Upton
@ 2021-08-10 8:40 ` Marc Zyngier
2021-08-10 9:09 ` Oliver Upton
0 siblings, 1 reply; 39+ messages in thread
From: Marc Zyngier @ 2021-08-10 8:40 UTC (permalink / raw)
To: Oliver Upton
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, 09 Aug 2021 17:45:28 +0100,
Oliver Upton <oupton@google.com> wrote:
>
> On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > From: Oliver Upton <oupton@google.com>
> >
> > 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 <oupton@google.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > [maz: fixed width computation not to lose the last bit, added
> > max delta generation for the timer]
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > Link: https://lore.kernel.org/r/20210807191428.3488948-1-oupton@google.com
> > ---
> > drivers/clocksource/arm_arch_timer.c | 34 ++++++++++++++++++++++++----
> > 1 file changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > index fa09952b94bf..74eca831d0d9 100644
> > --- a/drivers/clocksource/arm_arch_timer.c
> > +++ b/drivers/clocksource/arm_arch_timer.c
> > @@ -52,6 +52,12 @@
> > #define CNTV_CVAL_LO 0x30
> > #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;
> >
> > struct arch_timer {
> > @@ -95,6 +101,22 @@ static int __init early_evtstrm_cfg(char *buf)
> > }
> > early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
> >
> > +/*
> > + * 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 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 - 1) + 1, 56, 64);
> > +}
>
> Reposting thoughts from the original patch:
>
> 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 (40 years) than assume the
> sanity of the system counter implementation.
I really don't think that would be a likely implementation. The fact
that the ARM ARM only talks about the width of the counter makes it a
strong case that there is no 'ceiling' other than the natural
saturation of the counter, IMO. If a rollover was allowed to occur
before, it would definitely be mentioned.
Think about it: you'd need to implement an extra comparator to drive
the reset of the counter. It would also make the implementation of
CVAL stupidly complicated: how do you handle the set of values that
fit in the counter width, but are out of the counter range?
Even though the architecture is not the clearest thing, I'm expecting
the CPU designers to try and save gates, rather than trying to
implement a GOTCHA, expensive counter... ;-)
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 11/13] clocksource/arm_arch_timer: Fix masking for high freq counters
2021-08-10 8:40 ` Marc Zyngier
@ 2021-08-10 9:09 ` Oliver Upton
0 siblings, 0 replies; 39+ messages in thread
From: Oliver Upton @ 2021-08-10 9:09 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Tue, Aug 10, 2021 at 1:40 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 09 Aug 2021 17:45:28 +0100,
> Oliver Upton <oupton@google.com> wrote:
> >
> > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > From: Oliver Upton <oupton@google.com>
> > >
> > > 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 <oupton@google.com>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > [maz: fixed width computation not to lose the last bit, added
> > > max delta generation for the timer]
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > Link: https://lore.kernel.org/r/20210807191428.3488948-1-oupton@google.com
> > > ---
> > > drivers/clocksource/arm_arch_timer.c | 34 ++++++++++++++++++++++++----
> > > 1 file changed, 29 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> > > index fa09952b94bf..74eca831d0d9 100644
> > > --- a/drivers/clocksource/arm_arch_timer.c
> > > +++ b/drivers/clocksource/arm_arch_timer.c
> > > @@ -52,6 +52,12 @@
> > > #define CNTV_CVAL_LO 0x30
> > > #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;
> > >
> > > struct arch_timer {
> > > @@ -95,6 +101,22 @@ static int __init early_evtstrm_cfg(char *buf)
> > > }
> > > early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
> > >
> > > +/*
> > > + * 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 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 - 1) + 1, 56, 64);
> > > +}
> >
> > Reposting thoughts from the original patch:
> >
> > 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 (40 years) than assume the
> > sanity of the system counter implementation.
>
> I really don't think that would be a likely implementation. The fact
> that the ARM ARM only talks about the width of the counter makes it a
> strong case that there is no 'ceiling' other than the natural
> saturation of the counter, IMO. If a rollover was allowed to occur
> before, it would definitely be mentioned.
>
> Think about it: you'd need to implement an extra comparator to drive
> the reset of the counter. It would also make the implementation of
> CVAL stupidly complicated: how do you handle the set of values that
> fit in the counter width, but are out of the counter range?
>
> Even though the architecture is not the clearest thing, I'm expecting
> the CPU designers to try and save gates, rather than trying to
> implement a GOTCHA, expensive counter... ;-)
Fair, I'll put the tinfoil away then :) Just wanted to avoid reading
between the lines, but it would be rather stunning to encounter
hardware in the wild that does this. Your additions to the patch LGTM.
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 12/13] arm64: Add a capability for FEAT_EVC
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (10 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 11/13] clocksource/arm_arch_timer: Fix masking for high freq counters Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 16:30 ` Oliver Upton
2021-08-09 15:26 ` [PATCH 13/13] arm64: Add CNT{P,V}CTSS_EL0 alternatives to cnt{p,v}ct_el0 Marc Zyngier
12 siblings, 1 reply; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
Add a new capability to detect the Enhanced Counter Virtualization
feature (FEAT_EVC).
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kernel/cpufeature.c | 10 ++++++++++
arch/arm64/tools/cpucaps | 1 +
2 files changed, 11 insertions(+)
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 0ead8bfedf20..9c2ce5408811 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1899,6 +1899,16 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
.sign = FTR_UNSIGNED,
.min_field_value = 1,
},
+ {
+ .desc = "Enhanced counter virtualization",
+ .capability = ARM64_HAS_ECV,
+ .type = ARM64_CPUCAP_SYSTEM_FEATURE,
+ .matches = has_cpuid_feature,
+ .sys_reg = SYS_ID_AA64MMFR0_EL1,
+ .field_pos = ID_AA64MMFR0_ECV_SHIFT,
+ .sign = FTR_UNSIGNED,
+ .min_field_value = 1,
+ },
#ifdef CONFIG_ARM64_PAN
{
.desc = "Privileged Access Never",
diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
index 49305c2e6dfd..7a7c58acd8f0 100644
--- a/arch/arm64/tools/cpucaps
+++ b/arch/arm64/tools/cpucaps
@@ -18,6 +18,7 @@ HAS_CRC32
HAS_DCPODP
HAS_DCPOP
HAS_E0PD
+HAS_ECV
HAS_EPAN
HAS_GENERIC_AUTH
HAS_GENERIC_AUTH_ARCH
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 12/13] arm64: Add a capability for FEAT_EVC
2021-08-09 15:26 ` [PATCH 12/13] arm64: Add a capability for FEAT_EVC Marc Zyngier
@ 2021-08-09 16:30 ` Oliver Upton
2021-08-09 16:34 ` Oliver Upton
2021-08-09 18:02 ` Marc Zyngier
0 siblings, 2 replies; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 16:30 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
Hi Marc,
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Add a new capability to detect the Enhanced Counter Virtualization
> feature (FEAT_EVC).
>
s/FEAT_EVC/FEAT_ECV/g
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kernel/cpufeature.c | 10 ++++++++++
> arch/arm64/tools/cpucaps | 1 +
> 2 files changed, 11 insertions(+)
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 0ead8bfedf20..9c2ce5408811 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1899,6 +1899,16 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> .sign = FTR_UNSIGNED,
> .min_field_value = 1,
> },
> + {
> + .desc = "Enhanced counter virtualization",
> + .capability = ARM64_HAS_ECV,
> + .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> + .matches = has_cpuid_feature,
> + .sys_reg = SYS_ID_AA64MMFR0_EL1,
> + .field_pos = ID_AA64MMFR0_ECV_SHIFT,
> + .sign = FTR_UNSIGNED,
> + .min_field_value = 1,
> + },
Per one of your other patches in the series, it sounds like userspace
access to the self-synchronized registers hasn't been settled yet.
However, if/when available to userspace, should this cpufeature map to
an ELF HWCAP?
Also, w.r.t. my series I have out for ECV in KVM. All the controls
used in EL2 depend on ECV=0x2. I agree that ECV=0x1 needs a cpufeature
bit, but what about EL2's use case?
Besides the typo:
Reviewed-by: Oliver Upton <oupton@google.com>
--
Thanks,
Oliver
> #ifdef CONFIG_ARM64_PAN
> {
> .desc = "Privileged Access Never",
> diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
> index 49305c2e6dfd..7a7c58acd8f0 100644
> --- a/arch/arm64/tools/cpucaps
> +++ b/arch/arm64/tools/cpucaps
> @@ -18,6 +18,7 @@ HAS_CRC32
> HAS_DCPODP
> HAS_DCPOP
> HAS_E0PD
> +HAS_ECV
> HAS_EPAN
> HAS_GENERIC_AUTH
> HAS_GENERIC_AUTH_ARCH
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 12/13] arm64: Add a capability for FEAT_EVC
2021-08-09 16:30 ` Oliver Upton
@ 2021-08-09 16:34 ` Oliver Upton
2021-08-09 18:02 ` Marc Zyngier
1 sibling, 0 replies; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 16:34 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 9:30 AM Oliver Upton <oupton@google.com> wrote:
>
> Hi Marc,
>
> On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Add a new capability to detect the Enhanced Counter Virtualization
> > feature (FEAT_EVC).
> >
>
> s/FEAT_EVC/FEAT_ECV/g
>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > arch/arm64/kernel/cpufeature.c | 10 ++++++++++
> > arch/arm64/tools/cpucaps | 1 +
> > 2 files changed, 11 insertions(+)
> >
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 0ead8bfedf20..9c2ce5408811 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -1899,6 +1899,16 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> > .sign = FTR_UNSIGNED,
> > .min_field_value = 1,
> > },
> > + {
> > + .desc = "Enhanced counter virtualization",
> > + .capability = ARM64_HAS_ECV,
> > + .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> > + .matches = has_cpuid_feature,
> > + .sys_reg = SYS_ID_AA64MMFR0_EL1,
> > + .field_pos = ID_AA64MMFR0_ECV_SHIFT,
> > + .sign = FTR_UNSIGNED,
> > + .min_field_value = 1,
> > + },
>
> Per one of your other patches in the series, it sounds like userspace
> access to the self-synchronized registers hasn't been settled yet.
> However, if/when available to userspace, should this cpufeature map to
> an ELF HWCAP?
>
> Also, w.r.t. my series I have out for ECV in KVM. All the controls
> used in EL2 depend on ECV=0x2. I agree that ECV=0x1 needs a cpufeature
> bit, but what about EL2's use case?
Forgot to link the series:
http://lore.kernel.org/r/20210804085819.846610-1-oupton@google.com
>
> Besides the typo:
>
> Reviewed-by: Oliver Upton <oupton@google.com>
>
> --
> Thanks,
> Oliver
>
> > #ifdef CONFIG_ARM64_PAN
> > {
> > .desc = "Privileged Access Never",
> > diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
> > index 49305c2e6dfd..7a7c58acd8f0 100644
> > --- a/arch/arm64/tools/cpucaps
> > +++ b/arch/arm64/tools/cpucaps
> > @@ -18,6 +18,7 @@ HAS_CRC32
> > HAS_DCPODP
> > HAS_DCPOP
> > HAS_E0PD
> > +HAS_ECV
> > HAS_EPAN
> > HAS_GENERIC_AUTH
> > HAS_GENERIC_AUTH_ARCH
> > --
> > 2.30.2
> >
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 12/13] arm64: Add a capability for FEAT_EVC
2021-08-09 16:30 ` Oliver Upton
2021-08-09 16:34 ` Oliver Upton
@ 2021-08-09 18:02 ` Marc Zyngier
2021-08-09 18:21 ` Oliver Upton
1 sibling, 1 reply; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 18:02 UTC (permalink / raw)
To: Oliver Upton
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
Hi Oliver,
Thanks for having a look.
On Mon, 09 Aug 2021 17:30:45 +0100,
Oliver Upton <oupton@google.com> wrote:
>
> Hi Marc,
>
> On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Add a new capability to detect the Enhanced Counter Virtualization
> > feature (FEAT_EVC).
> >
>
> s/FEAT_EVC/FEAT_ECV/g
I'm the knig fo tpyoes :).
>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > arch/arm64/kernel/cpufeature.c | 10 ++++++++++
> > arch/arm64/tools/cpucaps | 1 +
> > 2 files changed, 11 insertions(+)
> >
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 0ead8bfedf20..9c2ce5408811 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -1899,6 +1899,16 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> > .sign = FTR_UNSIGNED,
> > .min_field_value = 1,
> > },
> > + {
> > + .desc = "Enhanced counter virtualization",
> > + .capability = ARM64_HAS_ECV,
> > + .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> > + .matches = has_cpuid_feature,
> > + .sys_reg = SYS_ID_AA64MMFR0_EL1,
> > + .field_pos = ID_AA64MMFR0_ECV_SHIFT,
> > + .sign = FTR_UNSIGNED,
> > + .min_field_value = 1,
> > + },
>
> Per one of your other patches in the series, it sounds like userspace
> access to the self-synchronized registers hasn't been settled yet.
> However, if/when available to userspace, should this cpufeature map to
> an ELF HWCAP?
We can't prevent the access to userspace, unless we also trap
cntvct_el0 and cntfreq_el0. Which we try not to do. But you are indeed
correct, we probably have a HWCAP if we decide to advertise it to
userspace.
> Also, w.r.t. my series I have out for ECV in KVM. All the controls
> used in EL2 depend on ECV=0x2. I agree that ECV=0x1 needs a cpufeature
> bit, but what about EL2's use case?
My idea was to have a ARM64_HAS_ECV2 to capture the EL2 extensions
with min_field_value=2.
> Besides the typo:
>
> Reviewed-by: Oliver Upton <oupton@google.com>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 12/13] arm64: Add a capability for FEAT_EVC
2021-08-09 18:02 ` Marc Zyngier
@ 2021-08-09 18:21 ` Oliver Upton
2021-08-09 18:23 ` Oliver Upton
0 siblings, 1 reply; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 18:21 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 11:02 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Oliver,
>
> Thanks for having a look.
>
> On Mon, 09 Aug 2021 17:30:45 +0100,
> Oliver Upton <oupton@google.com> wrote:
> >
> > Hi Marc,
> >
> > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > Add a new capability to detect the Enhanced Counter Virtualization
> > > feature (FEAT_EVC).
> > >
> >
> > s/FEAT_EVC/FEAT_ECV/g
>
> I'm the knig fo tpyoes :).
>
> >
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > > arch/arm64/kernel/cpufeature.c | 10 ++++++++++
> > > arch/arm64/tools/cpucaps | 1 +
> > > 2 files changed, 11 insertions(+)
> > >
> > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > index 0ead8bfedf20..9c2ce5408811 100644
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@ -1899,6 +1899,16 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> > > .sign = FTR_UNSIGNED,
> > > .min_field_value = 1,
> > > },
> > > + {
> > > + .desc = "Enhanced counter virtualization",
> > > + .capability = ARM64_HAS_ECV,
> > > + .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> > > + .matches = has_cpuid_feature,
> > > + .sys_reg = SYS_ID_AA64MMFR0_EL1,
> > > + .field_pos = ID_AA64MMFR0_ECV_SHIFT,
> > > + .sign = FTR_UNSIGNED,
> > > + .min_field_value = 1,
> > > + },
> >
> > Per one of your other patches in the series, it sounds like userspace
> > access to the self-synchronized registers hasn't been settled yet.
> > However, if/when available to userspace, should this cpufeature map to
> > an ELF HWCAP?
>
> We can't prevent the access to userspace, unless we also trap
> cntvct_el0 and cntfreq_el0. Which we try not to do. But you are indeed
> correct, we probably have a HWCAP if we decide to advertise it to
> userspace.
>
> > Also, w.r.t. my series I have out for ECV in KVM. All the controls
> > used in EL2 depend on ECV=0x2. I agree that ECV=0x1 needs a cpufeature
> > bit, but what about EL2's use case?
>
> My idea was to have a ARM64_HAS_ECV2 to capture the EL2 extensions
> with min_field_value=2.
This SGTM. I imagine with your HWCAP patch you will be passing through
ID_AA64MMFR0_EL1.ECV to userspace too. Dunno if we should clamp to 1
or let userspace see ECV=2 when we enumerate the second cpufeature.
Definitely not worthy of a HWCAP, though.
--
Thanks,
Oliver
> > Besides the typo:
> >
> > Reviewed-by: Oliver Upton <oupton@google.com>
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 12/13] arm64: Add a capability for FEAT_EVC
2021-08-09 18:21 ` Oliver Upton
@ 2021-08-09 18:23 ` Oliver Upton
0 siblings, 0 replies; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 18:23 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
Oh, one more thing,
On Mon, Aug 9, 2021 at 11:21 AM Oliver Upton <oupton@google.com> wrote:
>
> On Mon, Aug 9, 2021 at 11:02 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Hi Oliver,
> >
> > Thanks for having a look.
> >
> > On Mon, 09 Aug 2021 17:30:45 +0100,
> > Oliver Upton <oupton@google.com> wrote:
> > >
> > > Hi Marc,
> > >
> > > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > Add a new capability to detect the Enhanced Counter Virtualization
> > > > feature (FEAT_EVC).
> > > >
> > >
> > > s/FEAT_EVC/FEAT_ECV/g
> >
> > I'm the knig fo tpyoes :).
> >
> > >
> > > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > > ---
> > > > arch/arm64/kernel/cpufeature.c | 10 ++++++++++
> > > > arch/arm64/tools/cpucaps | 1 +
> > > > 2 files changed, 11 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > index 0ead8bfedf20..9c2ce5408811 100644
> > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > @@ -1899,6 +1899,16 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> > > > .sign = FTR_UNSIGNED,
> > > > .min_field_value = 1,
> > > > },
> > > > + {
> > > > + .desc = "Enhanced counter virtualization",
Pesky nit: "Enhanced Counter Virtualization"
> > > > + .capability = ARM64_HAS_ECV,
> > > > + .type = ARM64_CPUCAP_SYSTEM_FEATURE,
> > > > + .matches = has_cpuid_feature,
> > > > + .sys_reg = SYS_ID_AA64MMFR0_EL1,
> > > > + .field_pos = ID_AA64MMFR0_ECV_SHIFT,
> > > > + .sign = FTR_UNSIGNED,
> > > > + .min_field_value = 1,
> > > > + },
> > >
> > > Per one of your other patches in the series, it sounds like userspace
> > > access to the self-synchronized registers hasn't been settled yet.
> > > However, if/when available to userspace, should this cpufeature map to
> > > an ELF HWCAP?
> >
> > We can't prevent the access to userspace, unless we also trap
> > cntvct_el0 and cntfreq_el0. Which we try not to do. But you are indeed
> > correct, we probably have a HWCAP if we decide to advertise it to
> > userspace.
> >
> > > Also, w.r.t. my series I have out for ECV in KVM. All the controls
> > > used in EL2 depend on ECV=0x2. I agree that ECV=0x1 needs a cpufeature
> > > bit, but what about EL2's use case?
> >
> > My idea was to have a ARM64_HAS_ECV2 to capture the EL2 extensions
> > with min_field_value=2.
>
> This SGTM. I imagine with your HWCAP patch you will be passing through
> ID_AA64MMFR0_EL1.ECV to userspace too. Dunno if we should clamp to 1
> or let userspace see ECV=2 when we enumerate the second cpufeature.
> Definitely not worthy of a HWCAP, though.
>
> --
> Thanks,
> Oliver
>
>
> > > Besides the typo:
> > >
> > > Reviewed-by: Oliver Upton <oupton@google.com>
> >
> > Thanks,
> >
> > M.
> >
> > --
> > Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH 13/13] arm64: Add CNT{P,V}CTSS_EL0 alternatives to cnt{p,v}ct_el0
2021-08-09 15:26 [PATCH 00/13] clocksource/arm_arch_timer: Add basic ARMv8.6 support Marc Zyngier
` (11 preceding siblings ...)
2021-08-09 15:26 ` [PATCH 12/13] arm64: Add a capability for FEAT_EVC Marc Zyngier
@ 2021-08-09 15:26 ` Marc Zyngier
2021-08-09 16:42 ` Oliver Upton
12 siblings, 1 reply; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 15:26 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel
Cc: Mark Rutland, Daniel Lezcano, Thomas Gleixner, Peter Shier,
Raghavendra Rao Ananta, Ricardo Koller, Oliver Upton,
Will Deacon, Catalin Marinas, Linus Walleij, kernel-team
CNTPCTSS_EL0 and CNTVCTSS_EL0 are alternatives to the usual
CNTPCT_EL0 and CNTVCT_EL0 that do not require a previous ISB
to be synchronised (SS stands for Self-Synchronising).
Use the ARM64_HAS_ECV capability to control alternative sequences
that switch to these low(er)-cost primitives. Note that the
counter access in the VDSO is for now left alone until we decide
whether we want to allow this.
For a good measure, wire the cntvct hooks to also handle CNTVCTSS.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/include/asm/arch_timer.h | 30 +++++++++++++++++++++++------
arch/arm64/include/asm/esr.h | 6 ++++++
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kernel/traps.c | 11 +++++++++++
4 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index b2f056db1225..785411a48512 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -64,12 +64,26 @@ DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
static inline notrace u64 arch_timer_read_cntpct_el0(void)
{
- return read_sysreg(cntpct_el0);
+ u64 cnt;
+
+ asm volatile(ALTERNATIVE("isb\n mrs %x0, cntpct_el0",
+ "nop\n" __mrs_s("%x0", SYS_CNTPCTSS_EL0),
+ ARM64_HAS_ECV)
+ : "=r" (cnt));
+
+ return cnt;
}
static inline notrace u64 arch_timer_read_cntvct_el0(void)
{
- return read_sysreg(cntvct_el0);
+ u64 cnt;
+
+ asm volatile(ALTERNATIVE("isb\n mrs %x0, cntvct_el0",
+ "nop\n" __mrs_s("%x0", SYS_CNTVCTSS_EL0),
+ ARM64_HAS_ECV)
+ : "=r" (cnt));
+
+ return cnt;
}
#define arch_timer_reg_read_stable(reg) \
@@ -166,8 +180,10 @@ static __always_inline u64 __arch_counter_get_cntpct(void)
{
u64 cnt;
- isb();
- cnt = read_sysreg(cntpct_el0);
+ asm volatile(ALTERNATIVE("isb\n mrs %x0, cntpct_el0",
+ "nop\n" __mrs_s("%x0", SYS_CNTPCTSS_EL0),
+ ARM64_HAS_ECV)
+ : "=r" (cnt));
arch_counter_enforce_ordering(cnt);
return cnt;
}
@@ -186,8 +202,10 @@ static __always_inline u64 __arch_counter_get_cntvct(void)
{
u64 cnt;
- isb();
- cnt = read_sysreg(cntvct_el0);
+ asm volatile(ALTERNATIVE("isb\n mrs %x0, cntvct_el0",
+ "nop\n" __mrs_s("%x0", SYS_CNTVCTSS_EL0),
+ ARM64_HAS_ECV)
+ : "=r" (cnt));
arch_counter_enforce_ordering(cnt);
return cnt;
}
diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 29f97eb3dad4..a305ce256090 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -227,6 +227,9 @@
#define ESR_ELx_SYS64_ISS_SYS_CNTVCT (ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 2, 14, 0) | \
ESR_ELx_SYS64_ISS_DIR_READ)
+#define ESR_ELx_SYS64_ISS_SYS_CNTVCTSS (ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 6, 14, 0) | \
+ ESR_ELx_SYS64_ISS_DIR_READ)
+
#define ESR_ELx_SYS64_ISS_SYS_CNTFRQ (ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 0, 14, 0) | \
ESR_ELx_SYS64_ISS_DIR_READ)
@@ -317,6 +320,9 @@
#define ESR_ELx_CP15_64_ISS_SYS_CNTVCT (ESR_ELx_CP15_64_ISS_SYS_VAL(1, 14) | \
ESR_ELx_CP15_64_ISS_DIR_READ)
+#define ESR_ELx_CP15_64_ISS_SYS_CNTVCTSS (ESR_ELx_CP15_64_ISS_SYS_VAL(9, 14) | \
+ ESR_ELx_CP15_64_ISS_DIR_READ)
+
#define ESR_ELx_CP15_32_ISS_SYS_CNTFRQ (ESR_ELx_CP15_32_ISS_SYS_VAL(0, 0, 14, 0) |\
ESR_ELx_CP15_32_ISS_DIR_READ)
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 7b9c3acba684..897f9c882895 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -506,6 +506,9 @@
#define SYS_CNTFRQ_EL0 sys_reg(3, 3, 14, 0, 0)
+#define SYS_CNTPCTSS_EL0 sys_reg(3, 3, 14, 0, 5)
+#define SYS_CNTVCTSS_EL0 sys_reg(3, 3, 14, 0, 6)
+
#define SYS_CNTP_TVAL_EL0 sys_reg(3, 3, 14, 2, 0)
#define SYS_CNTP_CTL_EL0 sys_reg(3, 3, 14, 2, 1)
#define SYS_CNTP_CVAL_EL0 sys_reg(3, 3, 14, 2, 2)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index b03e383d944a..16710ca55fbb 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -653,6 +653,12 @@ static const struct sys64_hook sys64_hooks[] = {
.esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCT,
.handler = cntvct_read_handler,
},
+ {
+ /* Trap read access to CNTVCTSS_EL0 */
+ .esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
+ .esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCTSS,
+ .handler = cntvct_read_handler,
+ },
{
/* Trap read access to CNTFRQ_EL0 */
.esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
@@ -729,6 +735,11 @@ static const struct sys64_hook cp15_64_hooks[] = {
.esr_val = ESR_ELx_CP15_64_ISS_SYS_CNTVCT,
.handler = compat_cntvct_read_handler,
},
+ {
+ .esr_mask = ESR_ELx_CP15_64_ISS_SYS_MASK,
+ .esr_val = ESR_ELx_CP15_64_ISS_SYS_CNTVCTSS,
+ .handler = compat_cntvct_read_handler,
+ },
{},
};
--
2.30.2
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH 13/13] arm64: Add CNT{P,V}CTSS_EL0 alternatives to cnt{p,v}ct_el0
2021-08-09 15:26 ` [PATCH 13/13] arm64: Add CNT{P,V}CTSS_EL0 alternatives to cnt{p,v}ct_el0 Marc Zyngier
@ 2021-08-09 16:42 ` Oliver Upton
2021-08-09 18:11 ` Marc Zyngier
0 siblings, 1 reply; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 16:42 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
>
> CNTPCTSS_EL0 and CNTVCTSS_EL0 are alternatives to the usual
> CNTPCT_EL0 and CNTVCT_EL0 that do not require a previous ISB
> to be synchronised (SS stands for Self-Synchronising).
>
> Use the ARM64_HAS_ECV capability to control alternative sequences
> that switch to these low(er)-cost primitives. Note that the
> counter access in the VDSO is for now left alone until we decide
> whether we want to allow this.
What remains to be figured out before we add this to the vDSO (and
presumably advertise to userspace through some standard convention)?
It would be nice to skip the trap handler altogether, unless there's a
can of worms lurking that I'm not aware of.
--
Thanks,
Oliver
> For a good measure, wire the cntvct hooks to also handle CNTVCTSS.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/include/asm/arch_timer.h | 30 +++++++++++++++++++++++------
> arch/arm64/include/asm/esr.h | 6 ++++++
> arch/arm64/include/asm/sysreg.h | 3 +++
> arch/arm64/kernel/traps.c | 11 +++++++++++
> 4 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index b2f056db1225..785411a48512 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -64,12 +64,26 @@ DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
>
> static inline notrace u64 arch_timer_read_cntpct_el0(void)
> {
> - return read_sysreg(cntpct_el0);
> + u64 cnt;
> +
> + asm volatile(ALTERNATIVE("isb\n mrs %x0, cntpct_el0",
> + "nop\n" __mrs_s("%x0", SYS_CNTPCTSS_EL0),
> + ARM64_HAS_ECV)
> + : "=r" (cnt));
> +
> + return cnt;
> }
>
> static inline notrace u64 arch_timer_read_cntvct_el0(void)
> {
> - return read_sysreg(cntvct_el0);
> + u64 cnt;
> +
> + asm volatile(ALTERNATIVE("isb\n mrs %x0, cntvct_el0",
> + "nop\n" __mrs_s("%x0", SYS_CNTVCTSS_EL0),
> + ARM64_HAS_ECV)
> + : "=r" (cnt));
> +
> + return cnt;
> }
>
> #define arch_timer_reg_read_stable(reg) \
> @@ -166,8 +180,10 @@ static __always_inline u64 __arch_counter_get_cntpct(void)
> {
> u64 cnt;
>
> - isb();
> - cnt = read_sysreg(cntpct_el0);
> + asm volatile(ALTERNATIVE("isb\n mrs %x0, cntpct_el0",
> + "nop\n" __mrs_s("%x0", SYS_CNTPCTSS_EL0),
> + ARM64_HAS_ECV)
> + : "=r" (cnt));
> arch_counter_enforce_ordering(cnt);
> return cnt;
> }
> @@ -186,8 +202,10 @@ static __always_inline u64 __arch_counter_get_cntvct(void)
> {
> u64 cnt;
>
> - isb();
> - cnt = read_sysreg(cntvct_el0);
> + asm volatile(ALTERNATIVE("isb\n mrs %x0, cntvct_el0",
> + "nop\n" __mrs_s("%x0", SYS_CNTVCTSS_EL0),
> + ARM64_HAS_ECV)
> + : "=r" (cnt));
> arch_counter_enforce_ordering(cnt);
> return cnt;
> }
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 29f97eb3dad4..a305ce256090 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -227,6 +227,9 @@
> #define ESR_ELx_SYS64_ISS_SYS_CNTVCT (ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 2, 14, 0) | \
> ESR_ELx_SYS64_ISS_DIR_READ)
>
> +#define ESR_ELx_SYS64_ISS_SYS_CNTVCTSS (ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 6, 14, 0) | \
> + ESR_ELx_SYS64_ISS_DIR_READ)
> +
> #define ESR_ELx_SYS64_ISS_SYS_CNTFRQ (ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 0, 14, 0) | \
> ESR_ELx_SYS64_ISS_DIR_READ)
>
> @@ -317,6 +320,9 @@
> #define ESR_ELx_CP15_64_ISS_SYS_CNTVCT (ESR_ELx_CP15_64_ISS_SYS_VAL(1, 14) | \
> ESR_ELx_CP15_64_ISS_DIR_READ)
>
> +#define ESR_ELx_CP15_64_ISS_SYS_CNTVCTSS (ESR_ELx_CP15_64_ISS_SYS_VAL(9, 14) | \
> + ESR_ELx_CP15_64_ISS_DIR_READ)
> +
> #define ESR_ELx_CP15_32_ISS_SYS_CNTFRQ (ESR_ELx_CP15_32_ISS_SYS_VAL(0, 0, 14, 0) |\
> ESR_ELx_CP15_32_ISS_DIR_READ)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 7b9c3acba684..897f9c882895 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -506,6 +506,9 @@
>
> #define SYS_CNTFRQ_EL0 sys_reg(3, 3, 14, 0, 0)
>
> +#define SYS_CNTPCTSS_EL0 sys_reg(3, 3, 14, 0, 5)
> +#define SYS_CNTVCTSS_EL0 sys_reg(3, 3, 14, 0, 6)
> +
> #define SYS_CNTP_TVAL_EL0 sys_reg(3, 3, 14, 2, 0)
> #define SYS_CNTP_CTL_EL0 sys_reg(3, 3, 14, 2, 1)
> #define SYS_CNTP_CVAL_EL0 sys_reg(3, 3, 14, 2, 2)
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index b03e383d944a..16710ca55fbb 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -653,6 +653,12 @@ static const struct sys64_hook sys64_hooks[] = {
> .esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCT,
> .handler = cntvct_read_handler,
> },
> + {
> + /* Trap read access to CNTVCTSS_EL0 */
> + .esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
> + .esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCTSS,
> + .handler = cntvct_read_handler,
> + },
> {
> /* Trap read access to CNTFRQ_EL0 */
> .esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
> @@ -729,6 +735,11 @@ static const struct sys64_hook cp15_64_hooks[] = {
> .esr_val = ESR_ELx_CP15_64_ISS_SYS_CNTVCT,
> .handler = compat_cntvct_read_handler,
> },
> + {
> + .esr_mask = ESR_ELx_CP15_64_ISS_SYS_MASK,
> + .esr_val = ESR_ELx_CP15_64_ISS_SYS_CNTVCTSS,
> + .handler = compat_cntvct_read_handler,
> + },
> {},
> };
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 13/13] arm64: Add CNT{P,V}CTSS_EL0 alternatives to cnt{p,v}ct_el0
2021-08-09 16:42 ` Oliver Upton
@ 2021-08-09 18:11 ` Marc Zyngier
2021-08-09 18:17 ` Oliver Upton
0 siblings, 1 reply; 39+ messages in thread
From: Marc Zyngier @ 2021-08-09 18:11 UTC (permalink / raw)
To: Oliver Upton
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, 09 Aug 2021 17:42:00 +0100,
Oliver Upton <oupton@google.com> wrote:
>
> On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > CNTPCTSS_EL0 and CNTVCTSS_EL0 are alternatives to the usual
> > CNTPCT_EL0 and CNTVCT_EL0 that do not require a previous ISB
> > to be synchronised (SS stands for Self-Synchronising).
> >
> > Use the ARM64_HAS_ECV capability to control alternative sequences
> > that switch to these low(er)-cost primitives. Note that the
> > counter access in the VDSO is for now left alone until we decide
> > whether we want to allow this.
>
> What remains to be figured out before we add this to the vDSO (and
> presumably advertise to userspace through some standard convention)?
We need to understand what breaks if we runtime-patch the VDSO just
like we do with the rest of the kernel. To start with, the debug
version of the shared object is not the same as the object presented
to the process. Maybe that's not a problem, but I would tend to err on
the side of caution.
An alternative suggested by Ard was to have a separate function
altogether for the counter access and an ifunc mapping to pick the
right one.
> It would be nice to skip the trap handler altogether, unless there's a
> can of worms lurking that I'm not aware of.
The trap handlers are only there to work around errata. If you look at
the arch timer code, you will notice that there is a bunch of SoCs and
CPUs that do not have a reliable counter, and for which we have to
trap the virtual counter accesses from userspace (as well as the
VDSO).
On sane platforms, userspace is free to use the virtual counter
without any trap.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 13/13] arm64: Add CNT{P,V}CTSS_EL0 alternatives to cnt{p,v}ct_el0
2021-08-09 18:11 ` Marc Zyngier
@ 2021-08-09 18:17 ` Oliver Upton
2021-08-10 7:59 ` Marc Zyngier
0 siblings, 1 reply; 39+ messages in thread
From: Oliver Upton @ 2021-08-09 18:17 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, Aug 9, 2021 at 11:11 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 09 Aug 2021 17:42:00 +0100,
> Oliver Upton <oupton@google.com> wrote:
> >
> > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > CNTPCTSS_EL0 and CNTVCTSS_EL0 are alternatives to the usual
> > > CNTPCT_EL0 and CNTVCT_EL0 that do not require a previous ISB
> > > to be synchronised (SS stands for Self-Synchronising).
> > >
> > > Use the ARM64_HAS_ECV capability to control alternative sequences
> > > that switch to these low(er)-cost primitives. Note that the
> > > counter access in the VDSO is for now left alone until we decide
> > > whether we want to allow this.
> >
> > What remains to be figured out before we add this to the vDSO (and
> > presumably advertise to userspace through some standard convention)?
>
> We need to understand what breaks if we runtime-patch the VDSO just
> like we do with the rest of the kernel. To start with, the debug
> version of the shared object is not the same as the object presented
> to the process. Maybe that's not a problem, but I would tend to err on
> the side of caution.
I would too, but there sadly are instances of Linux patching *user*
memory already (go look at how KVM/x86 handles the VMCALL/VMMCALL
instruction). But yes, I would much prefer the debug vDSO correspond
to the actual instructions.
> An alternative suggested by Ard was to have a separate function
> altogether for the counter access and an ifunc mapping to pick the
> right one.
>
Hmm, this does sound promising.
> > It would be nice to skip the trap handler altogether, unless there's a
> > can of worms lurking that I'm not aware of.
>
> The trap handlers are only there to work around errata. If you look at
> the arch timer code, you will notice that there is a bunch of SoCs and
> CPUs that do not have a reliable counter, and for which we have to
> trap the virtual counter accesses from userspace (as well as the
> VDSO).
>
> On sane platforms, userspace is free to use the virtual counter
> without any trap.
/facepalm I was about 2 cups of coffee short when writing this :) Thanks!
--
Oliver
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH 13/13] arm64: Add CNT{P,V}CTSS_EL0 alternatives to cnt{p,v}ct_el0
2021-08-09 18:17 ` Oliver Upton
@ 2021-08-10 7:59 ` Marc Zyngier
0 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2021-08-10 7:59 UTC (permalink / raw)
To: Oliver Upton
Cc: linux-arm-kernel, linux-kernel, Mark Rutland, Daniel Lezcano,
Thomas Gleixner, Peter Shier, Raghavendra Rao Ananta,
Ricardo Koller, Will Deacon, Catalin Marinas, Linus Walleij,
kernel-team
On Mon, 09 Aug 2021 19:17:38 +0100,
Oliver Upton <oupton@google.com> wrote:
>
> On Mon, Aug 9, 2021 at 11:11 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Mon, 09 Aug 2021 17:42:00 +0100,
> > Oliver Upton <oupton@google.com> wrote:
> > >
> > > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > CNTPCTSS_EL0 and CNTVCTSS_EL0 are alternatives to the usual
> > > > CNTPCT_EL0 and CNTVCT_EL0 that do not require a previous ISB
> > > > to be synchronised (SS stands for Self-Synchronising).
> > > >
> > > > Use the ARM64_HAS_ECV capability to control alternative sequences
> > > > that switch to these low(er)-cost primitives. Note that the
> > > > counter access in the VDSO is for now left alone until we decide
> > > > whether we want to allow this.
> > >
> > > What remains to be figured out before we add this to the vDSO (and
> > > presumably advertise to userspace through some standard convention)?
> >
> > We need to understand what breaks if we runtime-patch the VDSO just
> > like we do with the rest of the kernel. To start with, the debug
> > version of the shared object is not the same as the object presented
> > to the process. Maybe that's not a problem, but I would tend to err on
> > the side of caution.
>
> I would too, but there sadly are instances of Linux patching *user*
> memory already (go look at how KVM/x86 handles the VMCALL/VMMCALL
> instruction). But yes, I would much prefer the debug vDSO correspond
> to the actual instructions.
Urghhh... This reminds me of
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=arm/netwinder&id=72797818a31d37a7ec28db659afcab0a56d47968
which I never tried to get merged for this exact reason. I'd rather
not replicate this sort of braindamage^Wthing if I can avoid it.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 39+ messages in thread