| Message ID | 20210809152651.2297337-14-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clocksource/arm_arch_timer: Add basic ARMv8.6 support | expand |
On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote: > > CNTPCTSS_EL0 and CNTVCTSS_EL0 are alternatives to the usual > CNTPCT_EL0 and CNTVCT_EL0 that do not require a previous ISB > to be synchronised (SS stands for Self-Synchronising). > > Use the ARM64_HAS_ECV capability to control alternative sequences > that switch to these low(er)-cost primitives. Note that the > counter access in the VDSO is for now left alone until we decide > whether we want to allow this. What remains to be figured out before we add this to the vDSO (and presumably advertise to userspace through some standard convention)? It would be nice to skip the trap handler altogether, unless there's a can of worms lurking that I'm not aware of. -- Thanks, Oliver > For a good measure, wire the cntvct hooks to also handle CNTVCTSS. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/arch_timer.h | 30 +++++++++++++++++++++++------ > arch/arm64/include/asm/esr.h | 6 ++++++ > arch/arm64/include/asm/sysreg.h | 3 +++ > arch/arm64/kernel/traps.c | 11 +++++++++++ > 4 files changed, 44 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h > index b2f056db1225..785411a48512 100644 > --- a/arch/arm64/include/asm/arch_timer.h > +++ b/arch/arm64/include/asm/arch_timer.h > @@ -64,12 +64,26 @@ DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *, > > static inline notrace u64 arch_timer_read_cntpct_el0(void) > { > - return read_sysreg(cntpct_el0); > + u64 cnt; > + > + asm volatile(ALTERNATIVE("isb\n mrs %x0, cntpct_el0", > + "nop\n" __mrs_s("%x0", SYS_CNTPCTSS_EL0), > + ARM64_HAS_ECV) > + : "=r" (cnt)); > + > + return cnt; > } > > static inline notrace u64 arch_timer_read_cntvct_el0(void) > { > - return read_sysreg(cntvct_el0); > + u64 cnt; > + > + asm volatile(ALTERNATIVE("isb\n mrs %x0, cntvct_el0", > + "nop\n" __mrs_s("%x0", SYS_CNTVCTSS_EL0), > + ARM64_HAS_ECV) > + : "=r" (cnt)); > + > + return cnt; > } > > #define arch_timer_reg_read_stable(reg) \ > @@ -166,8 +180,10 @@ static __always_inline u64 __arch_counter_get_cntpct(void) > { > u64 cnt; > > - isb(); > - cnt = read_sysreg(cntpct_el0); > + asm volatile(ALTERNATIVE("isb\n mrs %x0, cntpct_el0", > + "nop\n" __mrs_s("%x0", SYS_CNTPCTSS_EL0), > + ARM64_HAS_ECV) > + : "=r" (cnt)); > arch_counter_enforce_ordering(cnt); > return cnt; > } > @@ -186,8 +202,10 @@ static __always_inline u64 __arch_counter_get_cntvct(void) > { > u64 cnt; > > - isb(); > - cnt = read_sysreg(cntvct_el0); > + asm volatile(ALTERNATIVE("isb\n mrs %x0, cntvct_el0", > + "nop\n" __mrs_s("%x0", SYS_CNTVCTSS_EL0), > + ARM64_HAS_ECV) > + : "=r" (cnt)); > arch_counter_enforce_ordering(cnt); > return cnt; > } > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h > index 29f97eb3dad4..a305ce256090 100644 > --- a/arch/arm64/include/asm/esr.h > +++ b/arch/arm64/include/asm/esr.h > @@ -227,6 +227,9 @@ > #define ESR_ELx_SYS64_ISS_SYS_CNTVCT (ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 2, 14, 0) | \ > ESR_ELx_SYS64_ISS_DIR_READ) > > +#define ESR_ELx_SYS64_ISS_SYS_CNTVCTSS (ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 6, 14, 0) | \ > + ESR_ELx_SYS64_ISS_DIR_READ) > + > #define ESR_ELx_SYS64_ISS_SYS_CNTFRQ (ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 0, 14, 0) | \ > ESR_ELx_SYS64_ISS_DIR_READ) > > @@ -317,6 +320,9 @@ > #define ESR_ELx_CP15_64_ISS_SYS_CNTVCT (ESR_ELx_CP15_64_ISS_SYS_VAL(1, 14) | \ > ESR_ELx_CP15_64_ISS_DIR_READ) > > +#define ESR_ELx_CP15_64_ISS_SYS_CNTVCTSS (ESR_ELx_CP15_64_ISS_SYS_VAL(9, 14) | \ > + ESR_ELx_CP15_64_ISS_DIR_READ) > + > #define ESR_ELx_CP15_32_ISS_SYS_CNTFRQ (ESR_ELx_CP15_32_ISS_SYS_VAL(0, 0, 14, 0) |\ > ESR_ELx_CP15_32_ISS_DIR_READ) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 7b9c3acba684..897f9c882895 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -506,6 +506,9 @@ > > #define SYS_CNTFRQ_EL0 sys_reg(3, 3, 14, 0, 0) > > +#define SYS_CNTPCTSS_EL0 sys_reg(3, 3, 14, 0, 5) > +#define SYS_CNTVCTSS_EL0 sys_reg(3, 3, 14, 0, 6) > + > #define SYS_CNTP_TVAL_EL0 sys_reg(3, 3, 14, 2, 0) > #define SYS_CNTP_CTL_EL0 sys_reg(3, 3, 14, 2, 1) > #define SYS_CNTP_CVAL_EL0 sys_reg(3, 3, 14, 2, 2) > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > index b03e383d944a..16710ca55fbb 100644 > --- a/arch/arm64/kernel/traps.c > +++ b/arch/arm64/kernel/traps.c > @@ -653,6 +653,12 @@ static const struct sys64_hook sys64_hooks[] = { > .esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCT, > .handler = cntvct_read_handler, > }, > + { > + /* Trap read access to CNTVCTSS_EL0 */ > + .esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK, > + .esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCTSS, > + .handler = cntvct_read_handler, > + }, > { > /* Trap read access to CNTFRQ_EL0 */ > .esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK, > @@ -729,6 +735,11 @@ static const struct sys64_hook cp15_64_hooks[] = { > .esr_val = ESR_ELx_CP15_64_ISS_SYS_CNTVCT, > .handler = compat_cntvct_read_handler, > }, > + { > + .esr_mask = ESR_ELx_CP15_64_ISS_SYS_MASK, > + .esr_val = ESR_ELx_CP15_64_ISS_SYS_CNTVCTSS, > + .handler = compat_cntvct_read_handler, > + }, > {}, > }; > > -- > 2.30.2 >
On Mon, 09 Aug 2021 17:42:00 +0100, Oliver Upton <oupton@google.com> wrote: > > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote: > > > > CNTPCTSS_EL0 and CNTVCTSS_EL0 are alternatives to the usual > > CNTPCT_EL0 and CNTVCT_EL0 that do not require a previous ISB > > to be synchronised (SS stands for Self-Synchronising). > > > > Use the ARM64_HAS_ECV capability to control alternative sequences > > that switch to these low(er)-cost primitives. Note that the > > counter access in the VDSO is for now left alone until we decide > > whether we want to allow this. > > What remains to be figured out before we add this to the vDSO (and > presumably advertise to userspace through some standard convention)? We need to understand what breaks if we runtime-patch the VDSO just like we do with the rest of the kernel. To start with, the debug version of the shared object is not the same as the object presented to the process. Maybe that's not a problem, but I would tend to err on the side of caution. An alternative suggested by Ard was to have a separate function altogether for the counter access and an ifunc mapping to pick the right one. > It would be nice to skip the trap handler altogether, unless there's a > can of worms lurking that I'm not aware of. The trap handlers are only there to work around errata. If you look at the arch timer code, you will notice that there is a bunch of SoCs and CPUs that do not have a reliable counter, and for which we have to trap the virtual counter accesses from userspace (as well as the VDSO). On sane platforms, userspace is free to use the virtual counter without any trap. Thanks, M.
On Mon, Aug 9, 2021 at 11:11 AM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 09 Aug 2021 17:42:00 +0100, > Oliver Upton <oupton@google.com> wrote: > > > > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > CNTPCTSS_EL0 and CNTVCTSS_EL0 are alternatives to the usual > > > CNTPCT_EL0 and CNTVCT_EL0 that do not require a previous ISB > > > to be synchronised (SS stands for Self-Synchronising). > > > > > > Use the ARM64_HAS_ECV capability to control alternative sequences > > > that switch to these low(er)-cost primitives. Note that the > > > counter access in the VDSO is for now left alone until we decide > > > whether we want to allow this. > > > > What remains to be figured out before we add this to the vDSO (and > > presumably advertise to userspace through some standard convention)? > > We need to understand what breaks if we runtime-patch the VDSO just > like we do with the rest of the kernel. To start with, the debug > version of the shared object is not the same as the object presented > to the process. Maybe that's not a problem, but I would tend to err on > the side of caution. I would too, but there sadly are instances of Linux patching *user* memory already (go look at how KVM/x86 handles the VMCALL/VMMCALL instruction). But yes, I would much prefer the debug vDSO correspond to the actual instructions. > An alternative suggested by Ard was to have a separate function > altogether for the counter access and an ifunc mapping to pick the > right one. > Hmm, this does sound promising. > > It would be nice to skip the trap handler altogether, unless there's a > > can of worms lurking that I'm not aware of. > > The trap handlers are only there to work around errata. If you look at > the arch timer code, you will notice that there is a bunch of SoCs and > CPUs that do not have a reliable counter, and for which we have to > trap the virtual counter accesses from userspace (as well as the > VDSO). > > On sane platforms, userspace is free to use the virtual counter > without any trap. /facepalm I was about 2 cups of coffee short when writing this :) Thanks! -- Oliver
On Mon, 09 Aug 2021 19:17:38 +0100, Oliver Upton <oupton@google.com> wrote: > > On Mon, Aug 9, 2021 at 11:11 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Mon, 09 Aug 2021 17:42:00 +0100, > > Oliver Upton <oupton@google.com> wrote: > > > > > > On Mon, Aug 9, 2021 at 8:48 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > CNTPCTSS_EL0 and CNTVCTSS_EL0 are alternatives to the usual > > > > CNTPCT_EL0 and CNTVCT_EL0 that do not require a previous ISB > > > > to be synchronised (SS stands for Self-Synchronising). > > > > > > > > Use the ARM64_HAS_ECV capability to control alternative sequences > > > > that switch to these low(er)-cost primitives. Note that the > > > > counter access in the VDSO is for now left alone until we decide > > > > whether we want to allow this. > > > > > > What remains to be figured out before we add this to the vDSO (and > > > presumably advertise to userspace through some standard convention)? > > > > We need to understand what breaks if we runtime-patch the VDSO just > > like we do with the rest of the kernel. To start with, the debug > > version of the shared object is not the same as the object presented > > to the process. Maybe that's not a problem, but I would tend to err on > > the side of caution. > > I would too, but there sadly are instances of Linux patching *user* > memory already (go look at how KVM/x86 handles the VMCALL/VMMCALL > instruction). But yes, I would much prefer the debug vDSO correspond > to the actual instructions. Urghhh... This reminds me of https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=arm/netwinder&id=72797818a31d37a7ec28db659afcab0a56d47968 which I never tried to get merged for this exact reason. I'd rather not replicate this sort of braindamage^Wthing if I can avoid it. Thanks, M.
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, + }, {}, };
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(-)