This small series has been prompted by a discussion with Oliver around
the fact that an ARMv8.6 implementation must have a 1GHz counter,
which leads to a number of things to break in the timer code:
- the counter rollover can come pretty quickly as we only advertise a
56bit counter,
- the maximum timer delta can be remarkably small, as we use the
countdown interface which is limited to 32bit...
Thankfully, there is a way out: we can compute the minimal width of
the counter based on the guarantees that the architecture gives us,
and we can use the 64bit comparator interface instead of the countdown
to program the timer.
Finally, we start making use of the ARMv8.6 ECV features by switching
accesses to the counters to a self-synchronising register, removing
the need for an ISB. Hopefully, implementations will *not* just stick
an invisible ISB there...
A side effect of the switch to CVAL is that XGene-1 breaks. I have
added a workaround to keep it alive.
I have added Oliver's original patch[0] to the series and tweaked a
couple of things. Blame me if I broke anything.
The whole things has been tested on Juno (sysreg + MMIO timers),
XGene-1 (broken sysreg timers), FVP (FEAT_ECV, CNT*CTSS_EL0).
[0] https://lore.kernel.org/r/[email protected]
Marc Zyngier (12):
clocksource/arm_arch_timer: Drop CNT*_TVAL read accessors
clocksource/arm_arch_timer: Extend write side of timer register
accessors to u64
clocksource/arm_arch_timer: Move system register timer programming
over to CVAL
clocksource/arm_arch_timer: Move drop _tval from erratum function
names
clocksource/arm_arch_timer: Fix MMIO base address vs callback ordering
issue
clocksource/arm_arch_timer: Move MMIO timer programming over to CVAL
clocksource/arm_arch_timer: Advertise 56bit timer to the core code
clocksource/arm_arch_timer: Work around broken CVAL implementations
clocksource/arm_arch_timer: Remove any trace of the TVAL programming
interface
clocksource/arm_arch_timer: Drop unnecessary ISB on CVAL programming
arm64: Add a capability for FEAT_EVC
arm64: Add CNT{P,V}CTSS_EL0 alternatives to cnt{p,v}ct_el0
Oliver Upton (1):
clocksource/arm_arch_timer: Fix masking for high freq counters
arch/arm/include/asm/arch_timer.h | 29 ++--
arch/arm64/include/asm/arch_timer.h | 65 +++----
arch/arm64/include/asm/esr.h | 6 +
arch/arm64/include/asm/sysreg.h | 3 +
arch/arm64/kernel/cpufeature.c | 10 ++
arch/arm64/kernel/traps.c | 11 ++
arch/arm64/tools/cpucaps | 1 +
drivers/clocksource/arm_arch_timer.c | 249 ++++++++++++++++-----------
include/clocksource/arm_arch_timer.h | 2 +-
9 files changed, 234 insertions(+), 142 deletions(-)
--
2.30.2
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 <[email protected]>
---
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
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Mon, 09 Aug 2021 17:42:00 +0100,
Oliver Upton <[email protected]> wrote:
>
> On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <[email protected]> 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.
On Mon, Aug 9, 2021 at 11:11 AM Marc Zyngier <[email protected]> wrote:
>
> On Mon, 09 Aug 2021 17:42:00 +0100,
> Oliver Upton <[email protected]> wrote:
> >
> > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <[email protected]> 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
From: Oliver Upton <[email protected]>
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 <[email protected]>
Signed-off-by: Oliver Upton <[email protected]>
Reviewed-by: Linus Walleij <[email protected]>
[maz: fixed width computation not to lose the last bit, added
max delta generation for the timer]
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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
Add a new capability to detect the Enhanced Counter Virtualization
feature (FEAT_EVC).
Signed-off-by: Marc Zyngier <[email protected]>
---
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
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 <[email protected]>
---
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
On Mon, 09 Aug 2021 19:17:38 +0100,
Oliver Upton <[email protected]> wrote:
>
> On Mon, Aug 9, 2021 at 11:11 AM Marc Zyngier <[email protected]> wrote:
> >
> > On Mon, 09 Aug 2021 17:42:00 +0100,
> > Oliver Upton <[email protected]> wrote:
> > >
> > > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <[email protected]> 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.
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier <[email protected]> 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 <[email protected]>
Reviewed-by: Oliver Upton <[email protected]>
> ---
> 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
>
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 <[email protected]>
Looks good, builds cleanly, and boots fine on both arm/arm64:
Reviewed-by: Mark Rutland <[email protected]>
Tested-by: Mark Rutland <[email protected]>
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
>