All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] MIPS: Fix ejtag handler on SMP
@ 2018-06-08  5:51 r
  2018-06-08 22:37   ` Paul Burton
  0 siblings, 1 reply; 5+ messages in thread
From: r @ 2018-06-08  5:51 UTC (permalink / raw)
  To: linux-mips, paul.burton; +Cc: jhogan, ralf, Heiher

From: Heiher <r@hev.cc>

On SMP systems, the shared ejtag debug buffer may be overwritten by
other cores, because every cores can generate ejtag exception at
same time.

Unfortunately, in that context, it's difficult to relax more registers
to access per cpu buffers. so use ll/sc to serialize the access.

Signed-off-by: Heiher <r@hev.cc>
---
 arch/mips/kernel/genex.S | 46 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index 37b9383eacd3..3804afd878f8 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -354,16 +354,56 @@ NESTED(ejtag_debug_handler, PT_SIZE, sp)
 	sll	k0, k0, 30	# Check for SDBBP.
 	bgez	k0, ejtag_return
 
+#ifdef CONFIG_SMP
+1:	PTR_LA	k0, ejtag_debug_buffer_spinlock
+	ll	k0, 0(k0)
+	bnez	k0, 1b
+	PTR_LA	k0, ejtag_debug_buffer_spinlock
+	sc	k0, 0(k0)
+	beqz	k0, 1b
+# ifdef CONFIG_WEAK_REORDERING_BEYOND_LLSC
+	sync
+# endif
+
+	PTR_LA	k0, ejtag_debug_buffer
+	LONG_S	k1, 0(k0)
+
+	mfc0	k1, CP0_EBASE
+	andi	k1, MIPS_EBASE_CPUNUM
+	PTR_SLL	k1, LONGLOG
+	PTR_LA	k0, ejtag_debug_buffer_per_cpu
+	PTR_ADDU k0, k1
+
+	PTR_LA	k1, ejtag_debug_buffer
+	LONG_L	k1, 0(k1)
+	LONG_S	k1, 0(k0)
+
+	PTR_LA	k0, ejtag_debug_buffer_spinlock
+	sw	zero, 0(k0)
+#else
 	PTR_LA	k0, ejtag_debug_buffer
 	LONG_S	k1, 0(k0)
+#endif
+
 	SAVE_ALL
 	move	a0, sp
 	jal	ejtag_exception_handler
 	RESTORE_ALL
+
+#ifdef CONFIG_SMP
+	mfc0	k1, CP0_EBASE
+	andi	k1, MIPS_EBASE_CPUNUM
+	PTR_SLL	k1, LONGLOG
+	PTR_LA	k0, ejtag_debug_buffer_per_cpu
+	PTR_ADDU k0, k1
+	LONG_L	k1, 0(k0)
+#else
 	PTR_LA	k0, ejtag_debug_buffer
 	LONG_L	k1, 0(k0)
+#endif
 
 ejtag_return:
+	back_to_back_c0_hazard
 	MFC0	k0, CP0_DESAVE
 	.set	mips32
 	deret
@@ -377,6 +417,12 @@ ejtag_return:
 	.data
 EXPORT(ejtag_debug_buffer)
 	.fill	LONGSIZE
+#ifdef CONFIG_SMP
+EXPORT(ejtag_debug_buffer_spinlock)
+	.fill	LONGSIZE
+EXPORT(ejtag_debug_buffer_per_cpu)
+	.fill	LONGSIZE * NR_CPUS
+#endif
 	.previous
 
 	__INIT
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: Fix ejtag handler on SMP
@ 2018-06-08 22:37   ` Paul Burton
  0 siblings, 0 replies; 5+ messages in thread
From: Paul Burton @ 2018-06-08 22:37 UTC (permalink / raw)
  To: r; +Cc: linux-mips, jhogan, ralf

Hi Heiher,

On Fri, Jun 08, 2018 at 01:51:09PM +0800, r@hev.cc wrote:
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index 37b9383eacd3..3804afd878f8 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -354,16 +354,56 @@ NESTED(ejtag_debug_handler, PT_SIZE, sp)
>  	sll	k0, k0, 30	# Check for SDBBP.
>  	bgez	k0, ejtag_return
>  
> +#ifdef CONFIG_SMP
> +1:	PTR_LA	k0, ejtag_debug_buffer_spinlock
> +	ll	k0, 0(k0)
> +	bnez	k0, 1b
> +	PTR_LA	k0, ejtag_debug_buffer_spinlock
> +	sc	k0, 0(k0)
> +	beqz	k0, 1b
> +# ifdef CONFIG_WEAK_REORDERING_BEYOND_LLSC
> +	sync
> +# endif
> +
> +	PTR_LA	k0, ejtag_debug_buffer
> +	LONG_S	k1, 0(k0)
> +
> +	mfc0	k1, CP0_EBASE
> +	andi	k1, MIPS_EBASE_CPUNUM
> +	PTR_SLL	k1, LONGLOG
> +	PTR_LA	k0, ejtag_debug_buffer_per_cpu
> +	PTR_ADDU k0, k1

Neat - I like the concept of using the spinlock for just a short window
to free up k1, then using a per-CPU buffer.

Unfortunately it's not going to be as simple as using EBase.CPUNum, for
at least 2 reasons:

  - EBase.CPUNum doesn't always equal the Linux CPU number. For example
    in many systems which implement multi-threading ASE CPUNum is
    actually just a concatenation of some fixed number of bits
    specifying the core number and some fixed number of bits specifying
    the VP(E) number. So for example we might have a system with 2 cores
    each of which have 2 threads, but whose numbering looks like this:

      Linux CPU Number | Core Number | VP(E) Number | EBase.CPUNum
      -----------------|-------------|--------------|-------------
                     0 |  0b00 / 0x0 |   0b00 / 0x0 | 0b0000 / 0x0
		     1 |  0b00 / 0x0 |   0b01 / 0x1 | 0b0001 / 0x1
		     2 |  0b01 / 0x1 |   0b00 / 0x0 | 0b0100 / 0x4
		     3 |  0b01 / 0x1 |   0b01 / 0x1 | 0b0101 / 0x5

    This means that it might be the case that EBase.CPUNum >= NR_CPUS,
    which would result in a buffer overflow here.

    There are other ways this could happen too, for example if the
    kernel is configured with CONFIG_NR_CPUS=2 and run on a system which
    actually has more than 2 CPUs we'd have problems if the kernel was
    actually running on CPUs besides the ones CPUNum refers to as 0 & 1.

  - Some newer MIPS CPUs such as the I6500 introduce the concept of
    clusters, which are a layer of CPU topology beyond cores. In these
    systems EBase.CPUNum may not be aware of clusters at all, so for
    example the first CPU in each cluster may both have EBase.CPUNum
    equal to zero. MIPSr6 introduced the GlobalNumber register to
    resolve this problem.

One option would be to load the CPU number the same way
smp_processor_id() does with:

    lw	k1, TI_CPU(gp)

This has the disadvantage that things may go wrong if we take an EJTAG
exception before the gp register has been setup when a CPU is onlined,
but that should hopefully be a very small window of time. I think that
may be our best option.

Thanks,
    Paul

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: Fix ejtag handler on SMP
@ 2018-06-08 22:37   ` Paul Burton
  0 siblings, 0 replies; 5+ messages in thread
From: Paul Burton @ 2018-06-08 22:37 UTC (permalink / raw)
  To: r; +Cc: linux-mips, jhogan, ralf

Hi Heiher,

On Fri, Jun 08, 2018 at 01:51:09PM +0800, r@hev.cc wrote:
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index 37b9383eacd3..3804afd878f8 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -354,16 +354,56 @@ NESTED(ejtag_debug_handler, PT_SIZE, sp)
>  	sll	k0, k0, 30	# Check for SDBBP.
>  	bgez	k0, ejtag_return
>  
> +#ifdef CONFIG_SMP
> +1:	PTR_LA	k0, ejtag_debug_buffer_spinlock
> +	ll	k0, 0(k0)
> +	bnez	k0, 1b
> +	PTR_LA	k0, ejtag_debug_buffer_spinlock
> +	sc	k0, 0(k0)
> +	beqz	k0, 1b
> +# ifdef CONFIG_WEAK_REORDERING_BEYOND_LLSC
> +	sync
> +# endif
> +
> +	PTR_LA	k0, ejtag_debug_buffer
> +	LONG_S	k1, 0(k0)
> +
> +	mfc0	k1, CP0_EBASE
> +	andi	k1, MIPS_EBASE_CPUNUM
> +	PTR_SLL	k1, LONGLOG
> +	PTR_LA	k0, ejtag_debug_buffer_per_cpu
> +	PTR_ADDU k0, k1

Neat - I like the concept of using the spinlock for just a short window
to free up k1, then using a per-CPU buffer.

Unfortunately it's not going to be as simple as using EBase.CPUNum, for
at least 2 reasons:

  - EBase.CPUNum doesn't always equal the Linux CPU number. For example
    in many systems which implement multi-threading ASE CPUNum is
    actually just a concatenation of some fixed number of bits
    specifying the core number and some fixed number of bits specifying
    the VP(E) number. So for example we might have a system with 2 cores
    each of which have 2 threads, but whose numbering looks like this:

      Linux CPU Number | Core Number | VP(E) Number | EBase.CPUNum
      -----------------|-------------|--------------|-------------
                     0 |  0b00 / 0x0 |   0b00 / 0x0 | 0b0000 / 0x0
		     1 |  0b00 / 0x0 |   0b01 / 0x1 | 0b0001 / 0x1
		     2 |  0b01 / 0x1 |   0b00 / 0x0 | 0b0100 / 0x4
		     3 |  0b01 / 0x1 |   0b01 / 0x1 | 0b0101 / 0x5

    This means that it might be the case that EBase.CPUNum >= NR_CPUS,
    which would result in a buffer overflow here.

    There are other ways this could happen too, for example if the
    kernel is configured with CONFIG_NR_CPUS=2 and run on a system which
    actually has more than 2 CPUs we'd have problems if the kernel was
    actually running on CPUs besides the ones CPUNum refers to as 0 & 1.

  - Some newer MIPS CPUs such as the I6500 introduce the concept of
    clusters, which are a layer of CPU topology beyond cores. In these
    systems EBase.CPUNum may not be aware of clusters at all, so for
    example the first CPU in each cluster may both have EBase.CPUNum
    equal to zero. MIPSr6 introduced the GlobalNumber register to
    resolve this problem.

One option would be to load the CPU number the same way
smp_processor_id() does with:

    lw	k1, TI_CPU(gp)

This has the disadvantage that things may go wrong if we take an EJTAG
exception before the gp register has been setup when a CPU is onlined,
but that should hopefully be a very small window of time. I think that
may be our best option.

Thanks,
    Paul

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] MIPS: Fix ejtag handler on SMP
  2018-03-30  9:05 r
@ 2018-04-03  8:42 ` James Hogan
  0 siblings, 0 replies; 5+ messages in thread
From: James Hogan @ 2018-04-03  8:42 UTC (permalink / raw)
  To: r; +Cc: ralf, linux-mips

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

On Fri, Mar 30, 2018 at 05:05:15PM +0800, r@hev.cc wrote:
> From: Heiher <r@hev.cc>

Please can you add a proper commit description, explaining the problem
and what your patch does to fix it.

> 
> Signed-off-by: Heiher <r@hev.cc>
> ---
>  arch/mips/kernel/genex.S | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
> index 37b9383eacd3..9e0857fbe281 100644
> --- a/arch/mips/kernel/genex.S
> +++ b/arch/mips/kernel/genex.S
> @@ -354,6 +354,17 @@ NESTED(ejtag_debug_handler, PT_SIZE, sp)
>  	sll	k0, k0, 30	# Check for SDBBP.
>  	bgez	k0, ejtag_return
>  
> +#ifdef CONFIG_SMP
> +	PTR_LA	k0, ejtag_debug_buffer
> +1:	sync

Is the sync necessary? Or is that one of those platform specific
workarounds?

> +	ll	k0, LONGSIZE(k0)
> +	bnez	k0, 1b
> +	PTR_LA	k0, ejtag_debug_buffer
> +	sc	k0, LONGSIZE(k0)
> +	beqz	k0, 1b
> +	sync
> +#endif
> +
>  	PTR_LA	k0, ejtag_debug_buffer
>  	LONG_S	k1, 0(k0)
>  	SAVE_ALL
> @@ -363,6 +374,11 @@ NESTED(ejtag_debug_handler, PT_SIZE, sp)
>  	PTR_LA	k0, ejtag_debug_buffer
>  	LONG_L	k1, 0(k0)
>  
> +#ifdef CONFIG_SMP
> +	sw	zero, LONGSIZE(k0)
> +	sync

Same question. Its about to deret anyway which should cover that I
think?

> +#endif
> +
>  ejtag_return:
>  	MFC0	k0, CP0_DESAVE

Not specific to your patch, but I wonder whether there should be a
back_to_back_c0_hazard (ehb on r2+) somewhere before this MFC0.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH] MIPS: Fix ejtag handler on SMP
@ 2018-03-30  9:05 r
  2018-04-03  8:42 ` James Hogan
  0 siblings, 1 reply; 5+ messages in thread
From: r @ 2018-03-30  9:05 UTC (permalink / raw)
  To: ralf, linux-mips; +Cc: Heiher

From: Heiher <r@hev.cc>

Signed-off-by: Heiher <r@hev.cc>
---
 arch/mips/kernel/genex.S | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S
index 37b9383eacd3..9e0857fbe281 100644
--- a/arch/mips/kernel/genex.S
+++ b/arch/mips/kernel/genex.S
@@ -354,6 +354,17 @@ NESTED(ejtag_debug_handler, PT_SIZE, sp)
 	sll	k0, k0, 30	# Check for SDBBP.
 	bgez	k0, ejtag_return
 
+#ifdef CONFIG_SMP
+	PTR_LA	k0, ejtag_debug_buffer
+1:	sync
+	ll	k0, LONGSIZE(k0)
+	bnez	k0, 1b
+	PTR_LA	k0, ejtag_debug_buffer
+	sc	k0, LONGSIZE(k0)
+	beqz	k0, 1b
+	sync
+#endif
+
 	PTR_LA	k0, ejtag_debug_buffer
 	LONG_S	k1, 0(k0)
 	SAVE_ALL
@@ -363,6 +374,11 @@ NESTED(ejtag_debug_handler, PT_SIZE, sp)
 	PTR_LA	k0, ejtag_debug_buffer
 	LONG_L	k1, 0(k0)
 
+#ifdef CONFIG_SMP
+	sw	zero, LONGSIZE(k0)
+	sync
+#endif
+
 ejtag_return:
 	MFC0	k0, CP0_DESAVE
 	.set	mips32
@@ -377,6 +393,9 @@ ejtag_return:
 	.data
 EXPORT(ejtag_debug_buffer)
 	.fill	LONGSIZE
+#ifdef CONFIG_SMP
+	.fill	LONGSIZE
+#endif
 	.previous
 
 	__INIT
-- 
2.16.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-06-08 22:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-08  5:51 [PATCH] MIPS: Fix ejtag handler on SMP r
2018-06-08 22:37 ` Paul Burton
2018-06-08 22:37   ` Paul Burton
  -- strict thread matches above, loose matches on Subject: below --
2018-03-30  9:05 r
2018-04-03  8:42 ` James Hogan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.