From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:27:52
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/20210807191428.3488948-1-oupton@google.com
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:27:06
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(-)
@@ -64,17 +62,6 @@ struct arch_timer_erratum_workaround {DECLARE_PER_CPU(conststructarch_timer_erratum_workaround*,timer_unstable_counter_workaround);-/* inline sysreg accessors that make erratum_handler() work */-staticinlinenotraceu32arch_timer_read_cntp_tval_el0(void)-{-returnread_sysreg(cntp_tval_el0);-}--staticinlinenotraceu32arch_timer_read_cntv_tval_el0(void)-{-returnread_sysreg(cntv_tval_el0);-}-staticinlinenotraceu64arch_timer_read_cntpct_el0(void){returnread_sysreg(cntpct_el0);
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:27:09
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(-)
@@ -687,10 +693,18 @@ static __always_inline void set_next_event(const int access, unsigned long evt,structclock_event_device*clk){unsignedlongctrl;+u64cnt;+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,return0;}+static__always_inlinevoidset_next_event_mem(constintaccess,unsignedlongevt,+structclock_event_device*clk)+{+unsignedlongctrl;+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);+}+staticintarch_timer_set_next_event_virt_mem(unsignedlongevt,structclock_event_device*clk){-set_next_event(ARCH_TIMER_MEM_VIRT_ACCESS,evt,clk);+set_next_event_mem(ARCH_TIMER_MEM_VIRT_ACCESS,evt,clk);return0;}staticintarch_timer_set_next_event_phys_mem(unsignedlongevt,structclock_event_device*clk){-set_next_event(ARCH_TIMER_MEM_PHYS_ACCESS,evt,clk);+set_next_event_mem(ARCH_TIMER_MEM_PHYS_ACCESS,evt,clk);return0;}
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:27:13
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(-)
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:27:16
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(-)
@@ -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;caseARCH_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,return0;}+staticu64arch_counter_get_cnt_mem(structarch_timer*t,intoffset_lo)+{+u32cnt_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_inlinevoidset_next_event_mem(constintaccess,unsignedlongevt,structclock_event_device*clk){+structarch_timer*timer=to_arch_timer(clk);unsignedlongctrl;+u64cnt;+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);}
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:27:17
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(-)
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:27:24
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(-)
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:27:26
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(-)
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:27:33
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(-)
@@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq){intret;irq_handler_tfunc;-structarch_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;elsefunc=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;}returnret;
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:48:46
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(-)
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:49:15
From: Oliver Upton <redacted>
Unfortunately, the architecture provides no means to determine the bit
width of the system counter. However, we do know the following from the
specification:
- the system counter is at least 56 bits wide
- Roll-over time of not less than 40 years
To date, the arch timer driver has depended on the first property,
assuming any system counter to be 56 bits wide and masking off the rest.
However, combining a narrow clocksource mask with a high frequency
counter could result in prematurely wrapping the system counter by a
significant margin. For example, a 56 bit wide, 1GHz system counter
would wrap in a mere 2.28 years!
This is a problem for two reasons: v8.6+ implementations are required to
provide a 64 bit, 1GHz system counter. Furthermore, before v8.6,
implementers may select a counter frequency of their choosing.
Fix the issue by deriving a valid clock mask based on the second
property from above. Set the floor at 56 bits, since we know no system
counter is narrower than that.
Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <redacted>
Reviewed-by: Linus Walleij <redacted>
[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(-)
@@ -95,6 +101,22 @@ static int __init early_evtstrm_cfg(char *buf)}early_param("clocksource.arm_arch_timer.evtstrm",early_evtstrm_cfg);+/*+*MakesaneducatedguessatavalidcounterwidthbasedontheGenericTimer+*specification.Ofnote:+*1)thesystemcounterisatleast56bitswide+*2)aroll-overtimeofnotlessthan40years+*+*See'ARMDDI0487G.aD11.1.2("The system counter")'formoredetails.+*/+staticintarch_counter_get_width(void)+{+u64min_cycles=MIN_ROLLOVER_SECS*arch_timer_rate;++/* guarantee the returned width is within the valid range */+returnclamp_val(ilog2(min_cycles-1)+1,56,64);+}+/**Architectedsystemtimersupport.*/
@@ -1041,6 +1061,7 @@ struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)staticvoid__initarch_counter_register(unsignedtype){u64start_count;+intwidth;/* Register the CP15 based counter if we have one */if(type&ARCH_TIMER_TYPE_CP15){
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 15:49:22
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(-)
From: Oliver Upton <hidden> Date: 2021-08-09 16:13:10
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier [off-list ref] 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>
From: Oliver Upton <hidden> Date: 2021-08-09 16:17:18
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier [off-list ref] 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>
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 <redacted>
--
Thanks,
Oliver
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?
From: Oliver Upton <hidden> Date: 2021-08-09 16:42:16
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier [off-list ref] 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
quoted hunk
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(-)
From: Oliver Upton <hidden> Date: 2021-08-09 16:45:44
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier [off-list ref] wrote:
quoted hunk
From: Oliver Upton <redacted>
Unfortunately, the architecture provides no means to determine the bit
width of the system counter. However, we do know the following from the
specification:
- the system counter is at least 56 bits wide
- Roll-over time of not less than 40 years
To date, the arch timer driver has depended on the first property,
assuming any system counter to be 56 bits wide and masking off the rest.
However, combining a narrow clocksource mask with a high frequency
counter could result in prematurely wrapping the system counter by a
significant margin. For example, a 56 bit wide, 1GHz system counter
would wrap in a mere 2.28 years!
This is a problem for two reasons: v8.6+ implementations are required to
provide a 64 bit, 1GHz system counter. Furthermore, before v8.6,
implementers may select a counter frequency of their choosing.
Fix the issue by deriving a valid clock mask based on the second
property from above. Set the floor at 56 bits, since we know no system
counter is narrower than that.
Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <redacted>
Reviewed-by: Linus Walleij <redacted>
[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(-)
@@ -95,6 +101,22 @@ static int __init early_evtstrm_cfg(char *buf)}early_param("clocksource.arm_arch_timer.evtstrm",early_evtstrm_cfg);+/*+*MakesaneducatedguessatavalidcounterwidthbasedontheGenericTimer+*specification.Ofnote:+*1)thesystemcounterisatleast56bitswide+*2)aroll-overtimeofnotlessthan40years+*+*See'ARMDDI0487G.aD11.1.2("The system counter")'formoredetails.+*/+staticintarch_counter_get_width(void)+{+u64min_cycles=MIN_ROLLOVER_SECS*arch_timer_rate;++/* guarantee the returned width is within the valid range */+returnclamp_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
@@ -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) {
From: Oliver Upton <hidden> Date: 2021-08-09 16:52:16
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier [off-list ref] 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 <redacted>
@@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq){intret;irq_handler_tfunc;-structarch_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;elsefunc=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;}returnret;
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 <redacted>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-09 18:11:23
On Mon, 09 Aug 2021 17:42:00 +0100,
Oliver Upton [off-list ref] wrote:
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier [off-list ref] wrote:
quoted
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Oliver Upton <hidden> Date: 2021-08-09 18:17:54
On Mon, Aug 9, 2021 at 11:11 AM Marc Zyngier [off-list ref] wrote:
On Mon, 09 Aug 2021 17:42:00 +0100,
Oliver Upton [off-list ref] wrote:
quoted
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier [off-list ref] wrote:
quoted
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.
quoted
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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.
quoted
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
quoted
Besides the typo:
Reviewed-by: Oliver Upton <redacted>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
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.
quoted
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
quoted
quoted
Besides the typo:
Reviewed-by: Oliver Upton <redacted>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-10 07:59:23
On Mon, 09 Aug 2021 19:17:38 +0100,
Oliver Upton [off-list ref] wrote:
On Mon, Aug 9, 2021 at 11:11 AM Marc Zyngier [off-list ref] wrote:
quoted
On Mon, 09 Aug 2021 17:42:00 +0100,
Oliver Upton [off-list ref] wrote:
quoted
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier [off-list ref] wrote:
quoted
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.
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-10 08:27:38
On Mon, 09 Aug 2021 17:52:00 +0100,
Oliver Upton [off-list ref] wrote:
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier [off-list ref] wrote:
quoted
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-10 08:40:42
On Mon, 09 Aug 2021 17:45:28 +0100,
Oliver Upton [off-list ref] wrote:
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier [off-list ref] wrote:
quoted
From: Oliver Upton <redacted>
Unfortunately, the architecture provides no means to determine the bit
width of the system counter. However, we do know the following from the
specification:
- the system counter is at least 56 bits wide
- Roll-over time of not less than 40 years
To date, the arch timer driver has depended on the first property,
assuming any system counter to be 56 bits wide and masking off the rest.
However, combining a narrow clocksource mask with a high frequency
counter could result in prematurely wrapping the system counter by a
significant margin. For example, a 56 bit wide, 1GHz system counter
would wrap in a mere 2.28 years!
This is a problem for two reasons: v8.6+ implementations are required to
provide a 64 bit, 1GHz system counter. Furthermore, before v8.6,
implementers may select a counter frequency of their choosing.
Fix the issue by deriving a valid clock mask based on the second
property from above. Set the floor at 56 bits, since we know no system
counter is narrower than that.
Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <redacted>
Reviewed-by: Linus Walleij <redacted>
[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(-)
@@ -95,6 +101,22 @@ static int __init early_evtstrm_cfg(char *buf)}early_param("clocksource.arm_arch_timer.evtstrm",early_evtstrm_cfg);+/*+*MakesaneducatedguessatavalidcounterwidthbasedontheGenericTimer+*specification.Ofnote:+*1)thesystemcounterisatleast56bitswide+*2)aroll-overtimeofnotlessthan40years+*+*See'ARMDDI0487G.aD11.1.2("The system counter")'formoredetails.+*/+staticintarch_counter_get_width(void)+{+u64min_cycles=MIN_ROLLOVER_SECS*arch_timer_rate;++/* guarantee the returned width is within the valid range */+returnclamp_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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Oliver Upton <hidden> Date: 2021-08-10 09:10:02
On Tue, Aug 10, 2021 at 1:40 AM Marc Zyngier [off-list ref] wrote:
On Mon, 09 Aug 2021 17:45:28 +0100,
Oliver Upton [off-list ref] wrote:
quoted
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier [off-list ref] wrote:
quoted
From: Oliver Upton <redacted>
Unfortunately, the architecture provides no means to determine the bit
width of the system counter. However, we do know the following from the
specification:
- the system counter is at least 56 bits wide
- Roll-over time of not less than 40 years
To date, the arch timer driver has depended on the first property,
assuming any system counter to be 56 bits wide and masking off the rest.
However, combining a narrow clocksource mask with a high frequency
counter could result in prematurely wrapping the system counter by a
significant margin. For example, a 56 bit wide, 1GHz system counter
would wrap in a mere 2.28 years!
This is a problem for two reasons: v8.6+ implementations are required to
provide a 64 bit, 1GHz system counter. Furthermore, before v8.6,
implementers may select a counter frequency of their choosing.
Fix the issue by deriving a valid clock mask based on the second
property from above. Set the floor at 56 bits, since we know no system
counter is narrower than that.
Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Oliver Upton <redacted>
Reviewed-by: Linus Walleij <redacted>
[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(-)
@@ -95,6 +101,22 @@ static int __init early_evtstrm_cfg(char *buf)}early_param("clocksource.arm_arch_timer.evtstrm",early_evtstrm_cfg);+/*+*MakesaneducatedguessatavalidcounterwidthbasedontheGenericTimer+*specification.Ofnote:+*1)thesystemcounterisatleast56bitswide+*2)aroll-overtimeofnotlessthan40years+*+*See'ARMDDI0487G.aD11.1.2("The system counter")'formoredetails.+*/+staticintarch_counter_get_width(void)+{+u64min_cycles=MIN_ROLLOVER_SECS*arch_timer_rate;++/* guarantee the returned width is within the valid range */+returnclamp_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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-08-10 12:34:20
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?
@@ -778,9 +778,42 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,return0;}+staticu64__arch_timer_check_delta(void)+{+#ifdef CONFIG_ARM64+u64tmp;++/*+*XGene-1implementsCVALintermsofTVAL,meaningthatthe+*maximumtimerrangeis32bit.Shameonthem.Detectthe+*issuebysettingatimertonow+(1<<32),whichwill+*immediatelyfireontheduffCPU.+*/+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.
From: Marc Zyngier <maz@kernel.org> Date: 2021-08-10 13:15:07
On Tue, 10 Aug 2021 13:34:07 +0100,
Mark Rutland [off-list ref] wrote:
On Mon, Aug 09, 2021 at 04:26:46PM +0100, Marc Zyngier wrote:
quoted
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?
@@ -778,9 +778,42 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,return0;}+staticu64__arch_timer_check_delta(void)+{+#ifdef CONFIG_ARM64+u64tmp;++/*+*XGene-1implementsCVALintermsofTVAL,meaningthatthe+*maximumtimerrangeis32bit.Shameonthem.Detectthe+*issuebysettingatimertonow+(1<<32),whichwill+*immediatelyfireontheduffCPU.+*/+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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Oliver Upton <hidden> Date: 2021-08-11 07:02:22
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier [off-list ref] 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>
@@ -64,17 +62,6 @@ struct arch_timer_erratum_workaround {DECLARE_PER_CPU(conststructarch_timer_erratum_workaround*,timer_unstable_counter_workaround);-/* inline sysreg accessors that make erratum_handler() work */-staticinlinenotraceu32arch_timer_read_cntp_tval_el0(void)-{-returnread_sysreg(cntp_tval_el0);-}--staticinlinenotraceu32arch_timer_read_cntv_tval_el0(void)-{-returnread_sysreg(cntv_tval_el0);-}-staticinlinenotraceu64arch_timer_read_cntpct_el0(void){returnread_sysreg(cntpct_el0);
From: Oliver Upton <hidden> Date: 2021-08-11 07:16:11
On Mon, Aug 9, 2021 at 8:27 AM Marc Zyngier [off-list ref] 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>
@@ -687,10 +693,18 @@ static __always_inline void set_next_event(const int access, unsigned long evt,structclock_event_device*clk){unsignedlongctrl;+u64cnt;+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,return0;}+static__always_inlinevoidset_next_event_mem(constintaccess,unsignedlongevt,+structclock_event_device*clk)+{+unsignedlongctrl;+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);+}+staticintarch_timer_set_next_event_virt_mem(unsignedlongevt,structclock_event_device*clk){-set_next_event(ARCH_TIMER_MEM_VIRT_ACCESS,evt,clk);+set_next_event_mem(ARCH_TIMER_MEM_VIRT_ACCESS,evt,clk);return0;}staticintarch_timer_set_next_event_phys_mem(unsignedlongevt,structclock_event_device*clk){-set_next_event(ARCH_TIMER_MEM_PHYS_ACCESS,evt,clk);+set_next_event_mem(ARCH_TIMER_MEM_PHYS_ACCESS,evt,clk);return0;}
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-08-24 16:20:53
On Mon, Aug 09, 2021 at 04:26:39PM +0100, Marc Zyngier wrote:
quoted hunk
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(-)
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.
@@ -64,17 +62,6 @@ struct arch_timer_erratum_workaround {DECLARE_PER_CPU(conststructarch_timer_erratum_workaround*,timer_unstable_counter_workaround);-/* inline sysreg accessors that make erratum_handler() work */-staticinlinenotraceu32arch_timer_read_cntp_tval_el0(void)-{-returnread_sysreg(cntp_tval_el0);-}--staticinlinenotraceu32arch_timer_read_cntv_tval_el0(void)-{-returnread_sysreg(cntv_tval_el0);-}-staticinlinenotraceu64arch_timer_read_cntpct_el0(void){returnread_sysreg(cntpct_el0);
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-08-24 16:21:01
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.
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-08-24 16:21:33
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.
@@ -687,10 +693,18 @@ static __always_inline void set_next_event(const int access, unsigned long evt,structclock_event_device*clk){unsignedlongctrl;+u64cnt;+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,return0;}+static__always_inlinevoidset_next_event_mem(constintaccess,unsignedlongevt,+structclock_event_device*clk)+{+unsignedlongctrl;+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);+}+staticintarch_timer_set_next_event_virt_mem(unsignedlongevt,structclock_event_device*clk){-set_next_event(ARCH_TIMER_MEM_VIRT_ACCESS,evt,clk);+set_next_event_mem(ARCH_TIMER_MEM_VIRT_ACCESS,evt,clk);return0;}staticintarch_timer_set_next_event_phys_mem(unsignedlongevt,structclock_event_device*clk){-set_next_event(ARCH_TIMER_MEM_PHYS_ACCESS,evt,clk);+set_next_event_mem(ARCH_TIMER_MEM_PHYS_ACCESS,evt,clk);return0;}
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-08-24 16:30:01
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.
From: Mark Rutland <mark.rutland@arm.com> Date: 2021-08-24 16:44:58
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.
@@ -1168,25 +1168,25 @@ static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq){intret;irq_handler_tfunc;-structarch_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;elsefunc=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;}returnret;