Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1629286 > unrolled thread

Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled

Started byMarc Zyngier <marc.zyngier@arm.com>
First post2017-04-24 10:20 +0200
Last post2017-04-24 10:30 +0200
Articles 8 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if  workaround is enabled Marc Zyngier <marc.zyngier@arm.com> - 2017-04-24 10:20 +0200
    RE: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if  workaround is enabled "Lixiaoping (Timmy)" <lixiaoping3@huawei.com> - 2017-04-24 10:30 +0200
      Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if  workaround is enabled Marc Zyngier <marc.zyngier@arm.com> - 2017-04-24 10:50 +0200
        Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if  workaround is enabled Daniel Lezcano <daniel.lezcano@linaro.org> - 2017-04-24 11:20 +0200
        Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if  workaround is enabled Hanjun Guo <guohanjun@huawei.com> - 2017-04-24 11:20 +0200
          Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if  workaround is enabled Marc Zyngier <marc.zyngier@arm.com> - 2017-04-24 11:40 +0200
            Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if  workaround is enabled Catalin Marinas <catalin.marinas@arm.com> - 2017-04-24 13:30 +0200
    RE: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if  workaround is enabled "Lixiaoping (Timmy)" <lixiaoping3@huawei.com> - 2017-04-24 10:30 +0200

#1629286 — Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled

FromMarc Zyngier <marc.zyngier@arm.com>
Date2017-04-24 10:20 +0200
SubjectRe: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled
Message-ID<tzHnX-4Av-7@gated-at.bofh.it>
On 24/04/17 08:59, Hanjun Guo wrote:
> Hi Marc,
> 
> On 2017/4/5 1:18, Marc Zyngier wrote:
>> Userspace being allowed to use read CNTVCT_EL0 anytime (and not
>> only in the VDSO), we need to enable trapping whenever a cntvct
>> workaround is enabled on a given CPU.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 45 +++++++++++++++++++++++++-----------
>>  1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> [...]
>>  #else
>>  #define arch_timer_check_ool_workaround(t,a)		do { } while(0)
>>  #define erratum_set_next_event_tval_virt(...)		({BUG(); 0;})
>>  #define erratum_set_next_event_tval_phys(...)		({BUG(); 0;})
>>  #define erratum_handler(fn, r, ...)			({false;})
>> +#define arch_timer_this_cpu_has_cntvct_wa()		({false;})
>>  #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
>>  
>>  static __always_inline irqreturn_t timer_handler(const int access,
>> @@ -660,15 +680,23 @@ static void arch_counter_set_user_access(void)
>>  {
>>  	u32 cntkctl = arch_timer_get_cntkctl();
>>  
>> -	/* Disable user access to the timers and the physical counter */
>> +	/* Disable user access to the timers and both counters */
>>  	/* Also disable virtual event stream */
>>  	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
>>  			| ARCH_TIMER_USR_VT_ACCESS_EN
>> +		        | ARCH_TIMER_USR_VCT_ACCESS_EN
>>  			| ARCH_TIMER_VIRT_EVT_EN
>>  			| ARCH_TIMER_USR_PCT_ACCESS_EN);
>>  
>> -	/* Enable user access to the virtual counter */
>> -	cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
>> +	/*
>> +	 * Enable user access to the virtual counter if it doesn't
>> +	 * need to be workaround. The vdso may have been already
>> +	 * disabled though.
>> +	 */
>> +	if (arch_timer_this_cpu_has_cntvct_wa())
>> +		pr_info("CPU%d: Trapping CNTVCT access\n", smp_processor_id());
>> +	else
>> +		cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
> 
> Since CNTVCT_EL0 and CNTFRQ_EL0 share the same control register,then we
> need to trap CNTFRQ_EL0 as well.
> 
> We hit an "undefined instruction" when running Open MPI, which
> read CNTFRQ_EL0 in the user space:
> 
> https://github.com/open-mpi/ompi/blob/5b40fd267f9ddc6463051e402382a988637c3bb3/opal/include/opal/sys/arm64/timer.h
> 
> static inline opal_timer_t
> opal_sys_timer_freq(void) { opal_timer_t freq; __asm__ __volatile__
> ("mrs %0, CNTFRQ_EL0" : "=r" (freq)); return (opal_timer_t)(freq); }
> Please take look :) Thanks Hanjun

Ah, nice :-/ ... Can you please give the patch below a shot?

Thanks,

	M.

From 3f13e53878356057bde3e023614789d6c6b73628 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 24 Apr 2017 09:04:03 +0100
Subject: [PATCH] arm64: Add CNTFRQ_EL0 trap handler

We now trap accesses to CNTVCT_EL0 when the counter is broken
enough to require the kernel to mediate the access. But it
turns out that some existing userspace (such as OpenMPI) do
probe for the counter frequency, leading to an UNDEF exception
as CNTVCT_EL0 and CNTFRQ_EL0 share the same control bit.

The fix is to handle the exception the same way we do for CNTVCT_EL0.

Fixes: a86bd139f2ae ("arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled")
Reported-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/esr.h |  4 ++++
 arch/arm64/kernel/traps.c    | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index ad42e79a5d4d..8ea134f88fda 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -177,6 +177,10 @@
 
 #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_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
+					 ESR_ELx_SYS64_ISS_DIR_READ)
+
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
 
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 1de444e6c669..d4d6ae02cd55 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -513,6 +513,14 @@ static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
 	regs->pc += 4;
 }
 
+static void cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
+{
+	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
+
+	pt_regs_write_reg(regs, rt, read_sysreg(cntfrq_el0));
+	regs->pc += 4;
+}
+
 struct sys64_hook {
 	unsigned int esr_mask;
 	unsigned int esr_val;
@@ -537,6 +545,12 @@ static struct sys64_hook sys64_hooks[] = {
 		.esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCT,
 		.handler = cntvct_read_handler,
 	},
+	{
+		/* Trap read access to CNTFRQ_EL0 */
+		.esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
+		.esr_val = ESR_ELx_SYS64_ISS_SYS_CNTFRQ,
+		.handler = cntfrq_read_handler,
+	},
 	{},
 };
 
-- 
2.11.0

-- 
Jazz is not dead. It just smells funny...

[toc] | [next] | [standalone]

#1629297

From"Lixiaoping (Timmy)" <lixiaoping3@huawei.com>
Date2017-04-24 10:30 +0200
Message-ID<tzHxE-4Ds-21@gated-at.bofh.it>
In reply to#1629286
Hi Marc,

Sorry about previous email's confidential info. Please forget it.

+#define ESR_ELx_SYS64_ISS_SYS_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
+					 ESR_ELx_SYS64_ISS_DIR_READ)

I think (3, 3, 14, 0, 0) should be (3, 3, 0, 14, 0)?

-----------------------------------------------------------------------------
Best Regards,

Timmy Li (Lixiaoping)

-----Original Message-----
From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
Sent: Monday, April 24, 2017 4:13 PM
To: Guohanjun (Hanjun Guo) <guohanjun@huawei.com>; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>; Catalin Marinas <catalin.marinas@arm.com>; Daniel Lezcano <daniel.lezcano@linaro.org>; Will Deacon <will.deacon@arm.com>; Scott Wood <oss@buserror.net>; Hanjun Guo <hanjun.guo@linaro.org>; Dingtianhong <dingtianhong@huawei.com>; dann frazier <dann.frazier@canonical.com>; Thomas Gleixner <tglx@linutronix.de>; Lixiaoping (Timmy) <lixiaoping3@huawei.com>
Subject: Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled

On 24/04/17 08:59, Hanjun Guo wrote:
> Hi Marc,
> 
> On 2017/4/5 1:18, Marc Zyngier wrote:
>> Userspace being allowed to use read CNTVCT_EL0 anytime (and not
>> only in the VDSO), we need to enable trapping whenever a cntvct
>> workaround is enabled on a given CPU.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 45 +++++++++++++++++++++++++-----------
>>  1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> [...]
>>  #else
>>  #define arch_timer_check_ool_workaround(t,a)		do { } while(0)
>>  #define erratum_set_next_event_tval_virt(...)		({BUG(); 0;})
>>  #define erratum_set_next_event_tval_phys(...)		({BUG(); 0;})
>>  #define erratum_handler(fn, r, ...)			({false;})
>> +#define arch_timer_this_cpu_has_cntvct_wa()		({false;})
>>  #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
>>  
>>  static __always_inline irqreturn_t timer_handler(const int access,
>> @@ -660,15 +680,23 @@ static void arch_counter_set_user_access(void)
>>  {
>>  	u32 cntkctl = arch_timer_get_cntkctl();
>>  
>> -	/* Disable user access to the timers and the physical counter */
>> +	/* Disable user access to the timers and both counters */
>>  	/* Also disable virtual event stream */
>>  	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
>>  			| ARCH_TIMER_USR_VT_ACCESS_EN
>> +		        | ARCH_TIMER_USR_VCT_ACCESS_EN
>>  			| ARCH_TIMER_VIRT_EVT_EN
>>  			| ARCH_TIMER_USR_PCT_ACCESS_EN);
>>  
>> -	/* Enable user access to the virtual counter */
>> -	cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
>> +	/*
>> +	 * Enable user access to the virtual counter if it doesn't
>> +	 * need to be workaround. The vdso may have been already
>> +	 * disabled though.
>> +	 */
>> +	if (arch_timer_this_cpu_has_cntvct_wa())
>> +		pr_info("CPU%d: Trapping CNTVCT access\n", smp_processor_id());
>> +	else
>> +		cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
> 
> Since CNTVCT_EL0 and CNTFRQ_EL0 share the same control register,then we
> need to trap CNTFRQ_EL0 as well.
> 
> We hit an "undefined instruction" when running Open MPI, which
> read CNTFRQ_EL0 in the user space:
> 
> https://github.com/open-mpi/ompi/blob/5b40fd267f9ddc6463051e402382a988637c3bb3/opal/include/opal/sys/arm64/timer.h
> 
> static inline opal_timer_t
> opal_sys_timer_freq(void) { opal_timer_t freq; __asm__ __volatile__
> ("mrs %0, CNTFRQ_EL0" : "=r" (freq)); return (opal_timer_t)(freq); }
> Please take look :) Thanks Hanjun

Ah, nice :-/ ... Can you please give the patch below a shot?

Thanks,

	M.

From 3f13e53878356057bde3e023614789d6c6b73628 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 24 Apr 2017 09:04:03 +0100
Subject: [PATCH] arm64: Add CNTFRQ_EL0 trap handler

We now trap accesses to CNTVCT_EL0 when the counter is broken
enough to require the kernel to mediate the access. But it
turns out that some existing userspace (such as OpenMPI) do
probe for the counter frequency, leading to an UNDEF exception
as CNTVCT_EL0 and CNTFRQ_EL0 share the same control bit.

The fix is to handle the exception the same way we do for CNTVCT_EL0.

Fixes: a86bd139f2ae ("arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled")
Reported-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/esr.h |  4 ++++
 arch/arm64/kernel/traps.c    | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index ad42e79a5d4d..8ea134f88fda 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -177,6 +177,10 @@
 
 #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_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
+					 ESR_ELx_SYS64_ISS_DIR_READ)
+
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
 
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 1de444e6c669..d4d6ae02cd55 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -513,6 +513,14 @@ static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
 	regs->pc += 4;
 }
 
+static void cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
+{
+	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
+
+	pt_regs_write_reg(regs, rt, read_sysreg(cntfrq_el0));
+	regs->pc += 4;
+}
+
 struct sys64_hook {
 	unsigned int esr_mask;
 	unsigned int esr_val;
@@ -537,6 +545,12 @@ static struct sys64_hook sys64_hooks[] = {
 		.esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCT,
 		.handler = cntvct_read_handler,
 	},
+	{
+		/* Trap read access to CNTFRQ_EL0 */
+		.esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
+		.esr_val = ESR_ELx_SYS64_ISS_SYS_CNTFRQ,
+		.handler = cntfrq_read_handler,
+	},
 	{},
 };
 
-- 
2.11.0

-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1629307

FromMarc Zyngier <marc.zyngier@arm.com>
Date2017-04-24 10:50 +0200
Message-ID<tzHR0-4JU-9@gated-at.bofh.it>
In reply to#1629297
On 24/04/17 09:25, Lixiaoping (Timmy) wrote:
> Hi Marc,
> 
> Sorry about previous email's confidential info. Please forget it.
> 
> +#define ESR_ELx_SYS64_ISS_SYS_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
> +					 ESR_ELx_SYS64_ISS_DIR_READ)
> 
> I think (3, 3, 14, 0, 0) should be (3, 3, 0, 14, 0)?

Thanks for spotting this. I assumed that the sys_reg() and
SR_ELx_SYS64_ISS_SYS_VAL() macros took their arguments in the same 
order. That would have been too easy... ;-)

Amended patch below, please let me know if it works for you.

Thanks,

	M.

From 4444c86a97c1a487e12c319fdc197c88631d72b5 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 24 Apr 2017 09:04:03 +0100
Subject: [PATCH] arm64: Add CNTFRQ_EL0 trap handler

We now trap accesses to CNTVCT_EL0 when the counter is broken
enough to require the kernel to mediate the access. But it
turns out that some existing userspace (such as OpenMPI) do
probe for the counter frequency, leading to an UNDEF exception
as CNTVCT_EL0 and CNTFRQ_EL0 share the same control bit.

The fix is to handle the exception the same way we do for CNTVCT_EL0.

Fixes: a86bd139f2ae ("arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled")
Reported-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/esr.h |  4 ++++
 arch/arm64/kernel/traps.c    | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index ad42e79a5d4d..85997c0e5443 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -177,6 +177,10 @@
 
 #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_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 0, 14, 0) | \
+					 ESR_ELx_SYS64_ISS_DIR_READ)
+
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
 
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 1de444e6c669..d4d6ae02cd55 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -513,6 +513,14 @@ static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
 	regs->pc += 4;
 }
 
+static void cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
+{
+	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
+
+	pt_regs_write_reg(regs, rt, read_sysreg(cntfrq_el0));
+	regs->pc += 4;
+}
+
 struct sys64_hook {
 	unsigned int esr_mask;
 	unsigned int esr_val;
@@ -537,6 +545,12 @@ static struct sys64_hook sys64_hooks[] = {
 		.esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCT,
 		.handler = cntvct_read_handler,
 	},
+	{
+		/* Trap read access to CNTFRQ_EL0 */
+		.esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
+		.esr_val = ESR_ELx_SYS64_ISS_SYS_CNTFRQ,
+		.handler = cntfrq_read_handler,
+	},
 	{},
 };
 
-- 
2.11.0


-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1629342

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2017-04-24 11:20 +0200
Message-ID<tzIk2-59r-17@gated-at.bofh.it>
In reply to#1629307
On Mon, Apr 24, 2017 at 05:14:29PM +0800, Hanjun Guo wrote:
> On 2017/4/24 16:40, Marc Zyngier wrote:
> > On 24/04/17 09:25, Lixiaoping (Timmy) wrote:
> >> Hi Marc,
> >>
> >> Sorry about previous email's confidential info. Please forget it.
> >>
> >> +#define ESR_ELx_SYS64_ISS_SYS_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
> >> +					 ESR_ELx_SYS64_ISS_DIR_READ)
> >>
> >> I think (3, 3, 14, 0, 0) should be (3, 3, 0, 14, 0)?
> > Thanks for spotting this. I assumed that the sys_reg() and
> > SR_ELx_SYS64_ISS_SYS_VAL() macros took their arguments in the same 
> > order. That would have been too easy... ;-)
> >
> > Amended patch below, please let me know if it works for you.
> >
> > Thanks,
> >
> > 	M.
> >
> > >From 4444c86a97c1a487e12c319fdc197c88631d72b5 Mon Sep 17 00:00:00 2001
> > From: Marc Zyngier <marc.zyngier@arm.com>
> > Date: Mon, 24 Apr 2017 09:04:03 +0100
> > Subject: [PATCH] arm64: Add CNTFRQ_EL0 trap handler
> >
> > We now trap accesses to CNTVCT_EL0 when the counter is broken
> > enough to require the kernel to mediate the access. But it
> > turns out that some existing userspace (such as OpenMPI) do
> > probe for the counter frequency, leading to an UNDEF exception
> > as CNTVCT_EL0 and CNTFRQ_EL0 share the same control bit.
> >
> > The fix is to handle the exception the same way we do for CNTVCT_EL0.
> >
> > Fixes: a86bd139f2ae ("arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled")
> > Reported-by: Hanjun Guo <guohanjun@huawei.com>
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > ---
> 
> I tested this patch and the undefined instruction error is gone, I can
> get the FREQ in the user space now, thank you very much for the quick
> response.
> 
> Tested-by: Hanjun Guo <guohanjun@huawei.com>
> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>

Thanks for providing so quickly a fix and test it.

  -- Daniel

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1629346

FromHanjun Guo <guohanjun@huawei.com>
Date2017-04-24 11:20 +0200
Message-ID<tzIk2-59r-19@gated-at.bofh.it>
In reply to#1629307
On 2017/4/24 16:40, Marc Zyngier wrote:
> On 24/04/17 09:25, Lixiaoping (Timmy) wrote:
>> Hi Marc,
>>
>> Sorry about previous email's confidential info. Please forget it.
>>
>> +#define ESR_ELx_SYS64_ISS_SYS_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
>> +					 ESR_ELx_SYS64_ISS_DIR_READ)
>>
>> I think (3, 3, 14, 0, 0) should be (3, 3, 0, 14, 0)?
> Thanks for spotting this. I assumed that the sys_reg() and
> SR_ELx_SYS64_ISS_SYS_VAL() macros took their arguments in the same 
> order. That would have been too easy... ;-)
>
> Amended patch below, please let me know if it works for you.
>
> Thanks,
>
> 	M.
>
> >From 4444c86a97c1a487e12c319fdc197c88631d72b5 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@arm.com>
> Date: Mon, 24 Apr 2017 09:04:03 +0100
> Subject: [PATCH] arm64: Add CNTFRQ_EL0 trap handler
>
> We now trap accesses to CNTVCT_EL0 when the counter is broken
> enough to require the kernel to mediate the access. But it
> turns out that some existing userspace (such as OpenMPI) do
> probe for the counter frequency, leading to an UNDEF exception
> as CNTVCT_EL0 and CNTFRQ_EL0 share the same control bit.
>
> The fix is to handle the exception the same way we do for CNTVCT_EL0.
>
> Fixes: a86bd139f2ae ("arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled")
> Reported-by: Hanjun Guo <guohanjun@huawei.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---

I tested this patch and the undefined instruction error is gone, I can
get the FREQ in the user space now, thank you very much for the quick
response.

Tested-by: Hanjun Guo <guohanjun@huawei.com>
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>

Thanks
Hanjun

[toc] | [prev] | [next] | [standalone]

#1629363

FromMarc Zyngier <marc.zyngier@arm.com>
Date2017-04-24 11:40 +0200
Message-ID<tzIDn-5fx-25@gated-at.bofh.it>
In reply to#1629346
On 24/04/17 10:14, Hanjun Guo wrote:
> On 2017/4/24 16:40, Marc Zyngier wrote:
>> On 24/04/17 09:25, Lixiaoping (Timmy) wrote:
>>> Hi Marc,
>>>
>>> Sorry about previous email's confidential info. Please forget it.
>>>
>>> +#define ESR_ELx_SYS64_ISS_SYS_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
>>> +					 ESR_ELx_SYS64_ISS_DIR_READ)
>>>
>>> I think (3, 3, 14, 0, 0) should be (3, 3, 0, 14, 0)?
>> Thanks for spotting this. I assumed that the sys_reg() and
>> SR_ELx_SYS64_ISS_SYS_VAL() macros took their arguments in the same 
>> order. That would have been too easy... ;-)
>>
>> Amended patch below, please let me know if it works for you.
>>
>> Thanks,
>>
>> 	M.
>>
>> >From 4444c86a97c1a487e12c319fdc197c88631d72b5 Mon Sep 17 00:00:00 2001
>> From: Marc Zyngier <marc.zyngier@arm.com>
>> Date: Mon, 24 Apr 2017 09:04:03 +0100
>> Subject: [PATCH] arm64: Add CNTFRQ_EL0 trap handler
>>
>> We now trap accesses to CNTVCT_EL0 when the counter is broken
>> enough to require the kernel to mediate the access. But it
>> turns out that some existing userspace (such as OpenMPI) do
>> probe for the counter frequency, leading to an UNDEF exception
>> as CNTVCT_EL0 and CNTFRQ_EL0 share the same control bit.
>>
>> The fix is to handle the exception the same way we do for CNTVCT_EL0.
>>
>> Fixes: a86bd139f2ae ("arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled")
>> Reported-by: Hanjun Guo <guohanjun@huawei.com>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
> 
> I tested this patch and the undefined instruction error is gone, I can
> get the FREQ in the user space now, thank you very much for the quick
> response.
> 
> Tested-by: Hanjun Guo <guohanjun@huawei.com>
> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>

Thanks for giving it a go. Catalin, can you queue this via the arm64 tree?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1629441

FromCatalin Marinas <catalin.marinas@arm.com>
Date2017-04-24 13:30 +0200
Message-ID<tzKlQ-6oI-23@gated-at.bofh.it>
In reply to#1629363
On Mon, Apr 24, 2017 at 10:33:29AM +0100, Marc Zyngier wrote:
> On 24/04/17 10:14, Hanjun Guo wrote:
> > On 2017/4/24 16:40, Marc Zyngier wrote:
> >> On 24/04/17 09:25, Lixiaoping (Timmy) wrote:
> >>> Sorry about previous email's confidential info. Please forget it.
> >>>
> >>> +#define ESR_ELx_SYS64_ISS_SYS_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
> >>> +					 ESR_ELx_SYS64_ISS_DIR_READ)
> >>>
> >>> I think (3, 3, 14, 0, 0) should be (3, 3, 0, 14, 0)?
> >> Thanks for spotting this. I assumed that the sys_reg() and
> >> SR_ELx_SYS64_ISS_SYS_VAL() macros took their arguments in the same 
> >> order. That would have been too easy... ;-)
> >>
> >> Amended patch below, please let me know if it works for you.
> >>
> >> Thanks,
> >>
> >> 	M.
> >>
> >> >From 4444c86a97c1a487e12c319fdc197c88631d72b5 Mon Sep 17 00:00:00 2001
> >> From: Marc Zyngier <marc.zyngier@arm.com>
> >> Date: Mon, 24 Apr 2017 09:04:03 +0100
> >> Subject: [PATCH] arm64: Add CNTFRQ_EL0 trap handler
> >>
> >> We now trap accesses to CNTVCT_EL0 when the counter is broken
> >> enough to require the kernel to mediate the access. But it
> >> turns out that some existing userspace (such as OpenMPI) do
> >> probe for the counter frequency, leading to an UNDEF exception
> >> as CNTVCT_EL0 and CNTFRQ_EL0 share the same control bit.
> >>
> >> The fix is to handle the exception the same way we do for CNTVCT_EL0.
> >>
> >> Fixes: a86bd139f2ae ("arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled")
> >> Reported-by: Hanjun Guo <guohanjun@huawei.com>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> > 
> > I tested this patch and the undefined instruction error is gone, I can
> > get the FREQ in the user space now, thank you very much for the quick
> > response.
> > 
> > Tested-by: Hanjun Guo <guohanjun@huawei.com>
> > Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
> 
> Thanks for giving it a go. Catalin, can you queue this via the arm64 tree?

Done. Thanks for the quick fix.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1629298

From"Lixiaoping (Timmy)" <lixiaoping3@huawei.com>
Date2017-04-24 10:30 +0200
Message-ID<tzHxD-4Ds-15@gated-at.bofh.it>
In reply to#1629286
Hi Marc,

+#define ESR_ELx_SYS64_ISS_SYS_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
+					 ESR_ELx_SYS64_ISS_DIR_READ)

I think (3, 3, 14, 0, 0) should be (3, 3, 0, 14, 0)?

-----------------------------------------------------------------------------
Best Regards,

Timmy Li (Lixiaoping)
Turing Architecture and Design Dept, Hisilicon

-----------------------------------------------------------------------------
This e-mail and its attachments contain confidential information from HUAWEI, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!

-----Original Message-----
From: Marc Zyngier [mailto:marc.zyngier@arm.com] 
Sent: Monday, April 24, 2017 4:13 PM
To: Guohanjun (Hanjun Guo) <guohanjun@huawei.com>; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>; Catalin Marinas <catalin.marinas@arm.com>; Daniel Lezcano <daniel.lezcano@linaro.org>; Will Deacon <will.deacon@arm.com>; Scott Wood <oss@buserror.net>; Hanjun Guo <hanjun.guo@linaro.org>; Dingtianhong <dingtianhong@huawei.com>; dann frazier <dann.frazier@canonical.com>; Thomas Gleixner <tglx@linutronix.de>; Lixiaoping (Timmy) <lixiaoping3@huawei.com>
Subject: Re: [PATCH v3 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled

On 24/04/17 08:59, Hanjun Guo wrote:
> Hi Marc,
> 
> On 2017/4/5 1:18, Marc Zyngier wrote:
>> Userspace being allowed to use read CNTVCT_EL0 anytime (and not
>> only in the VDSO), we need to enable trapping whenever a cntvct
>> workaround is enabled on a given CPU.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/clocksource/arm_arch_timer.c | 45 +++++++++++++++++++++++++-----------
>>  1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> [...]
>>  #else
>>  #define arch_timer_check_ool_workaround(t,a)		do { } while(0)
>>  #define erratum_set_next_event_tval_virt(...)		({BUG(); 0;})
>>  #define erratum_set_next_event_tval_phys(...)		({BUG(); 0;})
>>  #define erratum_handler(fn, r, ...)			({false;})
>> +#define arch_timer_this_cpu_has_cntvct_wa()		({false;})
>>  #endif /* CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND */
>>  
>>  static __always_inline irqreturn_t timer_handler(const int access,
>> @@ -660,15 +680,23 @@ static void arch_counter_set_user_access(void)
>>  {
>>  	u32 cntkctl = arch_timer_get_cntkctl();
>>  
>> -	/* Disable user access to the timers and the physical counter */
>> +	/* Disable user access to the timers and both counters */
>>  	/* Also disable virtual event stream */
>>  	cntkctl &= ~(ARCH_TIMER_USR_PT_ACCESS_EN
>>  			| ARCH_TIMER_USR_VT_ACCESS_EN
>> +		        | ARCH_TIMER_USR_VCT_ACCESS_EN
>>  			| ARCH_TIMER_VIRT_EVT_EN
>>  			| ARCH_TIMER_USR_PCT_ACCESS_EN);
>>  
>> -	/* Enable user access to the virtual counter */
>> -	cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
>> +	/*
>> +	 * Enable user access to the virtual counter if it doesn't
>> +	 * need to be workaround. The vdso may have been already
>> +	 * disabled though.
>> +	 */
>> +	if (arch_timer_this_cpu_has_cntvct_wa())
>> +		pr_info("CPU%d: Trapping CNTVCT access\n", smp_processor_id());
>> +	else
>> +		cntkctl |= ARCH_TIMER_USR_VCT_ACCESS_EN;
> 
> Since CNTVCT_EL0 and CNTFRQ_EL0 share the same control register,then we
> need to trap CNTFRQ_EL0 as well.
> 
> We hit an "undefined instruction" when running Open MPI, which
> read CNTFRQ_EL0 in the user space:
> 
> https://github.com/open-mpi/ompi/blob/5b40fd267f9ddc6463051e402382a988637c3bb3/opal/include/opal/sys/arm64/timer.h
> 
> static inline opal_timer_t
> opal_sys_timer_freq(void) { opal_timer_t freq; __asm__ __volatile__
> ("mrs %0, CNTFRQ_EL0" : "=r" (freq)); return (opal_timer_t)(freq); }
> Please take look :) Thanks Hanjun

Ah, nice :-/ ... Can you please give the patch below a shot?

Thanks,

	M.

From 3f13e53878356057bde3e023614789d6c6b73628 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 24 Apr 2017 09:04:03 +0100
Subject: [PATCH] arm64: Add CNTFRQ_EL0 trap handler

We now trap accesses to CNTVCT_EL0 when the counter is broken
enough to require the kernel to mediate the access. But it
turns out that some existing userspace (such as OpenMPI) do
probe for the counter frequency, leading to an UNDEF exception
as CNTVCT_EL0 and CNTFRQ_EL0 share the same control bit.

The fix is to handle the exception the same way we do for CNTVCT_EL0.

Fixes: a86bd139f2ae ("arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled")
Reported-by: Hanjun Guo <guohanjun@huawei.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/esr.h |  4 ++++
 arch/arm64/kernel/traps.c    | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index ad42e79a5d4d..8ea134f88fda 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -177,6 +177,10 @@
 
 #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_CNTFRQ	(ESR_ELx_SYS64_ISS_SYS_VAL(3, 3, 14, 0, 0) | \
+					 ESR_ELx_SYS64_ISS_DIR_READ)
+
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
 
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 1de444e6c669..d4d6ae02cd55 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -513,6 +513,14 @@ static void cntvct_read_handler(unsigned int esr, struct pt_regs *regs)
 	regs->pc += 4;
 }
 
+static void cntfrq_read_handler(unsigned int esr, struct pt_regs *regs)
+{
+	int rt = (esr & ESR_ELx_SYS64_ISS_RT_MASK) >> ESR_ELx_SYS64_ISS_RT_SHIFT;
+
+	pt_regs_write_reg(regs, rt, read_sysreg(cntfrq_el0));
+	regs->pc += 4;
+}
+
 struct sys64_hook {
 	unsigned int esr_mask;
 	unsigned int esr_val;
@@ -537,6 +545,12 @@ static struct sys64_hook sys64_hooks[] = {
 		.esr_val = ESR_ELx_SYS64_ISS_SYS_CNTVCT,
 		.handler = cntvct_read_handler,
 	},
+	{
+		/* Trap read access to CNTFRQ_EL0 */
+		.esr_mask = ESR_ELx_SYS64_ISS_SYS_OP_MASK,
+		.esr_val = ESR_ELx_SYS64_ISS_SYS_CNTFRQ,
+		.handler = cntfrq_read_handler,
+	},
 	{},
 };
 
-- 
2.11.0

-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web