v3.8-rc issues with big endian

v3.8-rc issues with big endian

[Ben Dooks]

Whilst debugging with big endian ARM on v3.8-rc I came across a problem
with the new ASID code (commit b5466f8728527a05a493cc4abe9e6f034a1bbaab)
which is causing issues with user-land failing to work.

Since the new ASID code is using 64-bit accesses, the ASID ID is now
being placed in context->id + 4, which means that the assembly mm code
no-longer works.

I also noticed and fixed a couple of places where the new mmid macro was
not being used, so fixed those too.

Tested on a highbank system.

 [PATCH 1/2] ARM: mm: replace direct access to mm->context.id with
 [PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[PATCH 1/2] ARM: mm: replace direct access to mm->context.id with new macro

[Ben Dooks]

The mmid macro is meant to be used to get the mm->context.id data
from the mm structure, but it seems to have been missed in a cuple
of files.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 arch/arm/mm/proc-v6.S        |    2 +-
 arch/arm/mm/proc-v7-2level.S |    2 +-
 arch/arm/mm/proc-v7-3level.S |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/proc-v6.S b/arch/arm/mm/proc-v6.S
index 09c5233..bcaaa8d 100644
--- a/arch/arm/mm/proc-v6.S
+++ b/arch/arm/mm/proc-v6.S
@@ -101,7 +101,7 @@ ENTRY(cpu_v6_dcache_clean_area)
 ENTRY(cpu_v6_switch_mm)
 #ifdef CONFIG_MMU
 	mov	r2, #0
-	ldr	r1, [r1, #MM_CONTEXT_ID]	@ get mm->context.id
+	mmid	r1, r1				@ get mm->context.id
 	ALT_SMP(orr	r0, r0, #TTB_FLAGS_SMP)
 	ALT_UP(orr	r0, r0, #TTB_FLAGS_UP)
 	mcr	p15, 0, r2, c7, c5, 6		@ flush BTAC/BTB
diff --git a/arch/arm/mm/proc-v7-2level.S b/arch/arm/mm/proc-v7-2level.S
index 6d98c13..78f520b 100644
--- a/arch/arm/mm/proc-v7-2level.S
+++ b/arch/arm/mm/proc-v7-2level.S
@@ -40,7 +40,7 @@
 ENTRY(cpu_v7_switch_mm)
 #ifdef CONFIG_MMU
 	mov	r2, #0
-	ldr	r1, [r1, #MM_CONTEXT_ID]	@ get mm->context.id
+	mmid	r1, r1				@ get mm->context.id
 	ALT_SMP(orr	r0, r0, #TTB_FLAGS_SMP)
 	ALT_UP(orr	r0, r0, #TTB_FLAGS_UP)
 #ifdef CONFIG_ARM_ERRATA_430973
diff --git a/arch/arm/mm/proc-v7-3level.S b/arch/arm/mm/proc-v7-3level.S
index 7b56386..50bf1da 100644
--- a/arch/arm/mm/proc-v7-3level.S
+++ b/arch/arm/mm/proc-v7-3level.S
@@ -47,7 +47,7 @@
  */
 ENTRY(cpu_v7_switch_mm)
 #ifdef CONFIG_MMU
-	ldr	r1, [r1, #MM_CONTEXT_ID]	@ get mm->context.id
+	mmid	r1, r1				@ get mm->context.id
 	and	r3, r1, #0xff
 	mov	r3, r3, lsl #(48 - 32)		@ ASID
 	mcrr	p15, 0, r0, r3, c2		@ set TTB 0
-- 
1.7.10.4

[PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[Ben Dooks]

Since the new ASID code introduced in b5466f8728527a05a493cc4abe9e6f034a1bbaab
uses 64bit operations, when running in BE mode we see the values in
mm->context.id swapped around as the lowest word is being stored in
mm->context.id + 4, instead of + 0.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 arch/arm/mm/proc-macros.S |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
index eb6aa73..5a6a347 100644
--- a/arch/arm/mm/proc-macros.S
+++ b/arch/arm/mm/proc-macros.S
@@ -40,7 +40,11 @@
  * mmid - get context id from mm pointer (mm->context.id)
  */
 	.macro	mmid, rd, rn
+#ifdef __ARMEB__
+	ldr	\rd, [\rn, #MM_CONTEXT_ID + 4 ]
+#else
 	ldr	\rd, [\rn, #MM_CONTEXT_ID]
+#endif
 	.endm
 
 /*
-- 
1.7.10.4

[PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[Ben Dooks]

On 09/02/2013 23:15, Ben Dooks wrote:
> Since the new ASID code introduced in
> b5466f8728527a05a493cc4abe9e6f034a1bbaab
> uses 64bit operations, when running in BE mode we see the values in
> mm->context.id swapped around as the lowest word is being stored in
> mm->context.id + 4, instead of + 0.

The one thing I have just realised with this is that we are no-longer
going to be storing the mm->context.id data in the sat was a pre the
ASID changes, which could confuse debuggers which assume they can read
the data.

Should we fix this the easy way below, or go and change the code
in arch/arm/mm/context.c to deal with re-arranging the atomic64
accesses when in big-endian mode?

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  arch/arm/mm/proc-macros.S |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
> index eb6aa73..5a6a347 100644
> --- a/arch/arm/mm/proc-macros.S
> +++ b/arch/arm/mm/proc-macros.S
> @@ -40,7 +40,11 @@
>   * mmid - get context id from mm pointer (mm->context.id)
>   */
>  	.macro	mmid, rd, rn
> +#ifdef __ARMEB__
> +	ldr	\rd, [\rn, #MM_CONTEXT_ID + 4 ]
> +#else
>  	ldr	\rd, [\rn, #MM_CONTEXT_ID]
> +#endif
>  	.endm
>
>  /*

[PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[Sergei Shtylyov]

Hello.

On 10-02-2013 3:15, Ben Dooks wrote:

> Since the new ASID code introduced in b5466f8728527a05a493cc4abe9e6f034a1bbaab

    Plase also specify that commit's summary in parens (or however you like).

> uses 64bit operations, when running in BE mode we see the values in
> mm->context.id swapped around as the lowest word is being stored in
> mm->context.id + 4, instead of + 0.

> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

WBR, Sergei

[PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[Ben Dooks]

On 10/02/2013 14:14, Sergei Shtylyov wrote:
> Hello.
>
> On 10-02-2013 3:15, Ben Dooks wrote:
>
>> Since the new ASID code introduced in 
>> b5466f8728527a05a493cc4abe9e6f034a1bbaab
>
>    Plase also specify that commit's summary in parens (or however you 
> like).
>
>> uses 64bit operations, when running in BE mode we see the values in
>> mm->context.id swapped around as the lowest word is being stored in
>> mm->context.id + 4, instead of + 0.
>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>
> WBR, Sergei

Thanks, will sort that out and also take into account other feedback 
and
put new patches out today.

-- 
Ben

[PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[Ben Dooks]

Since the new ASID code in [b5466f8728527a05a493cc4abe9e6f034a1bbaab]
was changed to use 64bit operations it has broken the BE operation due
to an issue with the MM code accessing sub-fields of mm->context.id.

When running in BE mode we see the values in mm->context.id are stored
with the highest value first, so the LDR in the arch/arm/mm/proc-macros.S
reads the wrong part of this field. To resolve this, change the LDR in
the mmid macro to load from +4.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 arch/arm/mm/context.c     |    3 +++
 arch/arm/mm/proc-macros.S |    5 +++++
 2 files changed, 8 insertions(+)

diff --git a/arch/arm/mm/context.c b/arch/arm/mm/context.c
index bc4a5e9..7a05111 100644
--- a/arch/arm/mm/context.c
+++ b/arch/arm/mm/context.c
@@ -34,6 +34,9 @@
  * The ASID is used to tag entries in the CPU caches and TLBs.
  * The context ID is used by debuggers and trace logic, and
  * should be unique within all running processes.
+ *
+ * In big endian operation, the two 32 bit words are swapped if accesed by
+ * non 64-bit operations.
  */
 #define ASID_FIRST_VERSION	(1ULL << ASID_BITS)
 #define NUM_USER_ASIDS		(ASID_FIRST_VERSION - 1)
diff --git a/arch/arm/mm/proc-macros.S b/arch/arm/mm/proc-macros.S
index eb6aa73..f9a0aa7 100644
--- a/arch/arm/mm/proc-macros.S
+++ b/arch/arm/mm/proc-macros.S
@@ -38,9 +38,14 @@
 
 /*
  * mmid - get context id from mm pointer (mm->context.id)
+ * note, this field is 64bit, so in big-endian the two words are swapped too.
  */
 	.macro	mmid, rd, rn
+#ifdef __ARMEB__
+	ldr	\rd, [\rn, #MM_CONTEXT_ID + 4 ]
+#else
 	ldr	\rd, [\rn, #MM_CONTEXT_ID]
+#endif
 	.endm
 
 /*
-- 
1.7.10.4

[PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[Ben Dooks]

On 10/02/2013 14:57, Ben Dooks wrote:
> Since the new ASID code in [b5466f8728527a05a493cc4abe9e6f034a1bbaab]
> was changed to use 64bit operations it has broken the BE operation 
> due
> to an issue with the MM code accessing sub-fields of mm->context.id.

Fixed the ref, new patch pushed to git.

[PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[Russell King - ARM Linux]

On Sun, Feb 10, 2013 at 03:05:11PM +0000, Ben Dooks wrote:
> On 10/02/2013 14:57, Ben Dooks wrote:
>> Since the new ASID code in [b5466f8728527a05a493cc4abe9e6f034a1bbaab]
>> was changed to use 64bit operations it has broken the BE operation due
>> to an issue with the MM code accessing sub-fields of mm->context.id.
>
> Fixed the ref, new patch pushed to git.

No you haven't.

"Since the new ASID code in b5466f872 (ARM: mm: remove IPI broadcasting
on ASID rollover)..."

is the correct way to refer to another patch by commit ID - both the ID
_and_ its summary line.  You can use a short ID but it's a good idea to
add a few additional characters to avoid future hash clashes.

v3.8-rc issues with big endian

[Nicolas Pitre]

On Sat, 9 Feb 2013, Ben Dooks wrote:

> Whilst debugging with big endian ARM on v3.8-rc I came across a problem
> with the new ASID code (commit b5466f8728527a05a493cc4abe9e6f034a1bbaab)
> which is causing issues with user-land failing to work.
> 
> Since the new ASID code is using 64-bit accesses, the ASID ID is now
> being placed in context->id + 4, which means that the assembly mm code
> no-longer works.
> 
> I also noticed and fixed a couple of places where the new mmid macro was
> not being used, so fixed those too.
> 
> Tested on a highbank system.
> 
>  [PATCH 1/2] ARM: mm: replace direct access to mm->context.id with
>  [PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

For both patches:

Acked-by: Nicolas Pitre <nico@linaro.org>



> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[Ben Dooks]

On 10/02/13 23:29, Russell King - ARM Linux wrote:
> On Sun, Feb 10, 2013 at 03:05:11PM +0000, Ben Dooks wrote:
>> On 10/02/2013 14:57, Ben Dooks wrote:
>>> Since the new ASID code in [b5466f8728527a05a493cc4abe9e6f034a1bbaab]
>>> was changed to use 64bit operations it has broken the BE operation due
>>> to an issue with the MM code accessing sub-fields of mm->context.id.
>>
>> Fixed the ref, new patch pushed to git.
>
> No you haven't.
>
> "Since the new ASID code in b5466f872 (ARM: mm: remove IPI broadcasting
> on ASID rollover)..."
>
> is the correct way to refer to another patch by commit ID - both the ID
> _and_ its summary line.  You can use a short ID but it's a good idea to
> add a few additional characters to avoid future hash clashes.

My repo is showing:

  Since the new ASID code in b5466f8728527a05a493cc4abe9e6f034a1bbaab
  ("ARM: mm: remove IPI broadcasting on ASID rollover") was changed to
  use 64bit operations it has broken the BE operation due to an issue
  with the MM code accessing sub-fields of mm->context.id.

Would you like the commit-id shortened?
Is there anything else before this pair can be pulled?

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

[PATCH 2/2] ARM: mm: mm->context.id fix for big-endian

[Will Deacon]

On Mon, Feb 11, 2013 at 10:38:29AM +0000, Ben Dooks wrote:
> On 10/02/13 23:29, Russell King - ARM Linux wrote:
> > On Sun, Feb 10, 2013 at 03:05:11PM +0000, Ben Dooks wrote:
> >> On 10/02/2013 14:57, Ben Dooks wrote:
> >>> Since the new ASID code in [b5466f8728527a05a493cc4abe9e6f034a1bbaab]
> >>> was changed to use 64bit operations it has broken the BE operation due
> >>> to an issue with the MM code accessing sub-fields of mm->context.id.
> >>
> >> Fixed the ref, new patch pushed to git.
> >
> > No you haven't.
> >
> > "Since the new ASID code in b5466f872 (ARM: mm: remove IPI broadcasting
> > on ASID rollover)..."
> >
> > is the correct way to refer to another patch by commit ID - both the ID
> > _and_ its summary line.  You can use a short ID but it's a good idea to
> > add a few additional characters to avoid future hash clashes.
> 
> My repo is showing:
> 
>   Since the new ASID code in b5466f8728527a05a493cc4abe9e6f034a1bbaab
>   ("ARM: mm: remove IPI broadcasting on ASID rollover") was changed to
>   use 64bit operations it has broken the BE operation due to an issue
>   with the MM code accessing sub-fields of mm->context.id.
> 
> Would you like the commit-id shortened?
> Is there anything else before this pair can be pulled?

You can add my acked-by if you like:

  Acked-by: Will Deacon <will.deacon@arm.com>

Given that these are two small patches without any dependencies, it's
probably easier to stick them in the patch system (which will also resolve
any confusion about the form of the final commits).

Cheers,

Will