LinuxLists.cc - [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO

[permalink] [raw]

Subject: [PATCH 4/4] MIPS: Remove unused PAGE_* definitions

PAGE_{NONE,READONLY,COPY,SHARED} are no longer needed after refactoring
setup_protection_map() to explicitly list r/w/x permissions.

Signed-off-by: Kevin Cernekee <[email protected]>
---
arch/mips/include/asm/pgtable.h | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
index 0b3e7c6..718991c 100644
--- a/arch/mips/include/asm/pgtable.h
+++ b/arch/mips/include/asm/pgtable.h
@@ -21,13 +21,6 @@
struct mm_struct;
struct vm_area_struct;

-#define PAGE_NONE __pgprot(_PAGE_PRESENT | _CACHE_CACHABLE_NONCOHERENT)
-#define PAGE_SHARED __pgprot(_PAGE_PRESENT | _PAGE_WRITE | \
- _page_cachable_default)
-#define PAGE_COPY __pgprot(_PAGE_PRESENT | _PAGE_NO_EXEC | \
- _page_cachable_default)
-#define PAGE_READONLY __pgprot(_PAGE_PRESENT | \
- _page_cachable_default)
#define PAGE_KERNEL __pgprot(_PAGE_PRESENT | __READABLE | __WRITEABLE | \
_PAGE_GLOBAL | _page_cachable_default)
#define PAGE_USERIO __pgprot(_PAGE_PRESENT | _PAGE_WRITE | \
--
1.7.4.3

2011-04-16 16:52:07

[permalink] [raw]

Subject: [PATCH 3/4] MIPS: Clean up protection_map[] initialization

Fix long lines, factor out cut&paste flags, and add comments to explain
the pgprot flags used.

Signed-off-by: Kevin Cernekee <[email protected]>
---
arch/mips/mm/cache.c | 42 +++++++++++++++++++++++++-----------------
1 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
index 7c251e6..edb003f 100644
--- a/arch/mips/mm/cache.c
+++ b/arch/mips/mm/cache.c
@@ -137,23 +137,31 @@ EXPORT_SYMBOL(_page_cachable_default);

static inline void setup_protection_map(void)
{
- protection_map[0] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
- protection_map[1] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
- protection_map[2] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
- protection_map[3] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
- protection_map[4] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_READ);
- protection_map[5] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
- protection_map[6] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_READ);
- protection_map[7] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
-
- protection_map[8] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
- protection_map[9] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
- protection_map[10] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE | _PAGE_NO_READ);
- protection_map[11] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE);
- protection_map[12] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_READ);
- protection_map[13] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
- protection_map[14] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_WRITE | _PAGE_NO_READ);
- protection_map[15] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_WRITE);
+ int i;
+ const unsigned long prot[] = {
+ /* private mappings (clear the dirty bit until written) */
+ [0] = _PAGE_NO_EXEC | _PAGE_NO_READ, /* --- */
+ [1] = _PAGE_NO_EXEC, /* r-- */
+ [2] = _PAGE_NO_EXEC | _PAGE_NO_READ, /* -w- */
+ [3] = _PAGE_NO_EXEC, /* rw- */
+ [4] = _PAGE_NO_READ, /* --x */
+ [5] = 0, /* r-x */
+ [6] = _PAGE_NO_READ, /* -wx */
+ [7] = 0, /* rwx */
+ /* shared mappings */
+ [8] = _PAGE_NO_EXEC | _PAGE_NO_READ, /* --- */
+ [9] = _PAGE_NO_EXEC, /* r-- */
+ [10] = _PAGE_NO_EXEC | _PAGE_WRITE | _PAGE_NO_READ, /* -w- */
+ [11] = _PAGE_NO_EXEC | _PAGE_WRITE, /* rw- */
+ [12] = _PAGE_NO_READ, /* --x */
+ [13] = 0, /* r-x */
+ [14] = _PAGE_WRITE | _PAGE_NO_READ, /* -wx */
+ [15] = _PAGE_WRITE, /* rwx */
+ };
+
+ for (i = 0; i < ARRAY_SIZE(protection_map); i++)
+ protection_map[i] = __pgprot(_page_cachable_default |
+ _PAGE_PRESENT | prot[i]);
}

void __cpuinit cpu_cache_init(void)
--
1.7.4.3

2011-04-16 16:52:31

[permalink] [raw]

Subject: [PATCH 2/4] MIPS: Add dummy _PAGE_NO_EXEC field for R3000 and 64BIT_PHYS_ADDR cases

Add a non-functional (software-only) _PAGE_NO_EXEC bit for all MIPS
pte layouts. This allows for the elimination of the special case for
non-RIXI when initializing the page protection map.

Signed-off-by: Kevin Cernekee <[email protected]>
---
arch/mips/include/asm/pgtable-bits.h | 21 ++++++-------
arch/mips/mm/cache.c | 54 ++++++++++-----------------------
2 files changed, 27 insertions(+), 48 deletions(-)

diff --git a/arch/mips/include/asm/pgtable-bits.h b/arch/mips/include/asm/pgtable-bits.h
index 7afba78..c83eaca 100644
--- a/arch/mips/include/asm/pgtable-bits.h
+++ b/arch/mips/include/asm/pgtable-bits.h
@@ -36,10 +36,11 @@

#define _PAGE_PRESENT (1<<6) /* implemented in software */
#define _PAGE_NO_READ (1<<7) /* implemented in software */
-#define _PAGE_WRITE (1<<8) /* implemented in software */
-#define _PAGE_ACCESSED (1<<9) /* implemented in software */
-#define _PAGE_MODIFIED (1<<10) /* implemented in software */
-#define _PAGE_FILE (1<<10) /* set:pagecache unset:swap */
+#define _PAGE_NO_EXEC (1<<8) /* implemented in software */
+#define _PAGE_WRITE (1<<9) /* implemented in software */
+#define _PAGE_ACCESSED (1<<10) /* implemented in software */
+#define _PAGE_MODIFIED (1<<11) /* implemented in software */
+#define _PAGE_FILE (1<<11) /* set:pagecache unset:swap */

#define _PAGE_R4KBUG (1<<0) /* workaround for r4k bug */
#define _PAGE_GLOBAL (1<<0)
@@ -54,10 +55,11 @@

#define _PAGE_PRESENT (1<<0) /* implemented in software */
#define _PAGE_NO_READ (1<<1) /* implemented in software */
-#define _PAGE_WRITE (1<<2) /* implemented in software */
-#define _PAGE_ACCESSED (1<<3) /* implemented in software */
-#define _PAGE_MODIFIED (1<<4) /* implemented in software */
-#define _PAGE_FILE (1<<4) /* set:pagecache unset:swap */
+#define _PAGE_NO_EXEC (1<<2) /* implemented in software */
+#define _PAGE_WRITE (1<<3) /* implemented in software */
+#define _PAGE_ACCESSED (1<<4) /* implemented in software */
+#define _PAGE_MODIFIED (1<<5) /* implemented in software */
+#define _PAGE_FILE (1<<5) /* set:pagecache unset:swap */

#define _PAGE_GLOBAL (1<<8)
#define _PAGE_VALID (1<<9)
@@ -133,9 +135,6 @@
#endif
#define _PFN_MASK (~((1 << (_PFN_SHIFT)) - 1))

-#ifndef _PAGE_NO_EXEC
-#define _PAGE_NO_EXEC ({BUG(); 0; })
-#endif
#ifndef _PAGE_GLOBAL_SHIFT
#define _PAGE_GLOBAL_SHIFT ilog2(_PAGE_GLOBAL)
#endif
diff --git a/arch/mips/mm/cache.c b/arch/mips/mm/cache.c
index 12af739..7c251e6 100644
--- a/arch/mips/mm/cache.c
+++ b/arch/mips/mm/cache.c
@@ -137,43 +137,23 @@ EXPORT_SYMBOL(_page_cachable_default);

static inline void setup_protection_map(void)
{
- if (kernel_uses_smartmips_rixi) {
- protection_map[0] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
- protection_map[1] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
- protection_map[2] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
- protection_map[3] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
- protection_map[4] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_READ);
- protection_map[5] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
- protection_map[6] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_READ);
- protection_map[7] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
-
- protection_map[8] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
- protection_map[9] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
- protection_map[10] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE | _PAGE_NO_READ);
- protection_map[11] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE);
- protection_map[12] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_READ);
- protection_map[13] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
- protection_map[14] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_WRITE | _PAGE_NO_READ);
- protection_map[15] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_WRITE);
-
- } else {
- protection_map[0] = PAGE_NONE;
- protection_map[1] = PAGE_READONLY;
- protection_map[2] = PAGE_COPY;
- protection_map[3] = PAGE_COPY;
- protection_map[4] = PAGE_READONLY;
- protection_map[5] = PAGE_READONLY;
- protection_map[6] = PAGE_COPY;
- protection_map[7] = PAGE_COPY;
- protection_map[8] = PAGE_NONE;
- protection_map[9] = PAGE_READONLY;
- protection_map[10] = PAGE_SHARED;
- protection_map[11] = PAGE_SHARED;
- protection_map[12] = PAGE_READONLY;
- protection_map[13] = PAGE_READONLY;
- protection_map[14] = PAGE_SHARED;
- protection_map[15] = PAGE_SHARED;
- }
+ protection_map[0] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
+ protection_map[1] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
+ protection_map[2] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
+ protection_map[3] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
+ protection_map[4] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_READ);
+ protection_map[5] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
+ protection_map[6] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_READ);
+ protection_map[7] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
+
+ protection_map[8] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_NO_READ);
+ protection_map[9] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC);
+ protection_map[10] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE | _PAGE_NO_READ);
+ protection_map[11] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_EXEC | _PAGE_WRITE);
+ protection_map[12] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_NO_READ);
+ protection_map[13] = __pgprot(_page_cachable_default | _PAGE_PRESENT);
+ protection_map[14] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_WRITE | _PAGE_NO_READ);
+ protection_map[15] = __pgprot(_page_cachable_default | _PAGE_PRESENT | _PAGE_WRITE);
}

void __cpuinit cpu_cache_init(void)
--
1.7.4.3

2011-04-18 17:33:01

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On 04/16/2011 09:44 AM, Kevin Cernekee wrote:
> Reuse more of the same definitions for the non-RIXI and RIXI cases. This
> avoids having special cases for kernel_uses_smartmips_rixi cluttering up
> the pgtable*.h files.
>
> On hardware that does not support RI/XI, EntryLo bits 31:30 / 63:62 will
> remain unset and RI/XI permissions will not be enforced.
>
> Signed-off-by: Kevin Cernekee<[email protected]>
> ---
> arch/mips/include/asm/pgtable-bits.h | 23 ++++++++---------------
> arch/mips/include/asm/pgtable.h | 21 ++++++++-------------
> arch/mips/mm/tlbex.c | 17 +++++------------
> 3 files changed, 21 insertions(+), 40 deletions(-)
>
[...]

I like this patch.

How much testing have you done on non-RI/XI CPUs?

David Daney

2011-04-18 18:24:23

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Mon, Apr 18, 2011 at 10:32 AM, David Daney <[email protected]> wrote:
> How much testing have you done on non-RI/XI CPUs?

On a non-RIXI CPU I was able to boot the system, run a basic GUI
application, create R/W shared mappings to /dev/mem, insert/remove
kernel modules, run a broken program that dumps core, etc.

I guess it would be a good idea to make sure swap still works. Didn't
try that yet.

Can you think of anything else that might exercise the bits that were
touched by the patch? Were there any tests you ran during the
development of RIXI support which uncovered subtle issues?

Thanks.

2011-04-18 18:52:28

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On 04/18/2011 11:24 AM, Kevin Cernekee wrote:
> On Mon, Apr 18, 2011 at 10:32 AM, David Daney<[email protected]> wrote:
>> How much testing have you done on non-RI/XI CPUs?
>
> On a non-RIXI CPU I was able to boot the system, run a basic GUI
> application, create R/W shared mappings to /dev/mem, insert/remove
> kernel modules, run a broken program that dumps core, etc.
>
> I guess it would be a good idea to make sure swap still works. Didn't
> try that yet.
>
> Can you think of anything else that might exercise the bits that were
> touched by the patch? Were there any tests you ran during the
> development of RIXI support which uncovered subtle issues?
>

We run the LTP, I think it tests these things. We also have a small
test case that tests for both the no-read and no-execute parts, but that
would be expected to fail on platforms that don't have RI/XI bits.

David Daney

2011-05-12 14:10:46

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

Queued for 2.6.40. Thanks Kevin!

Ralf

2011-05-12 14:10:53

[permalink] [raw]

Subject: Re: [PATCH 2/4] MIPS: Add dummy _PAGE_NO_EXEC field for R3000 and 64BIT_PHYS_ADDR cases

Queued for 2.6.40. Thanks Kevin!

Ralf

2011-05-12 14:11:04

[permalink] [raw]

Subject: Re: [PATCH 3/4] MIPS: Clean up protection_map[] initialization

Queued for 2.6.40. Thanks Kevin!

Ralf

2011-05-12 14:11:11

[permalink] [raw]

Subject: Re: [PATCH 4/4] MIPS: Remove unused PAGE_* definitions

Queued for 2.6.40. Thanks Kevin!

Ralf

2011-05-13 15:05:42

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Sat, Apr 16, 2011 at 09:44:29AM -0700, Kevin Cernekee wrote:

> Reuse more of the same definitions for the non-RIXI and RIXI cases. This
> avoids having special cases for kernel_uses_smartmips_rixi cluttering up
> the pgtable*.h files.
>
> On hardware that does not support RI/XI, EntryLo bits 31:30 / 63:62 will
> remain unset and RI/XI permissions will not be enforced.

Nice idea but it breaks on 64-bit hardware running 32-bit kernels. On
those the RI/XI bits written to c0_entrylo0/1 31:30 will be interpreted as
physical address bits 37:36.

I'm removing this patch series from the 2.6.40 patch queue until we can
sort this out.

Ralf

2011-05-13 15:46:20

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Fri, May 13, 2011 at 8:07 AM, Ralf Baechle <[email protected]> wrote:
>> Reuse more of the same definitions for the non-RIXI and RIXI cases. This
>> avoids having special cases for kernel_uses_smartmips_rixi cluttering up
>> the pgtable*.h files.
>>
>> On hardware that does not support RI/XI, EntryLo bits 31:30 / 63:62 will
>> remain unset and RI/XI permissions will not be enforced.
>
> Nice idea but it breaks on 64-bit hardware running 32-bit kernels. On
> those the RI/XI bits written to c0_entrylo0/1 31:30 will be interpreted as
> physical address bits 37:36.

Hmm, are you sure? (Unfortunately I do not have a 64-bit machine to
test it on.)

I did not touch David's existing build_update_entries(), which makes a
point not to set the RI/XI bits when the RIXI feature is disabled:

if (kernel_uses_smartmips_rixi) {
UASM_i_SRL(p, tmp, tmp, ilog2(_PAGE_NO_EXEC));
UASM_i_SRL(p, ptep, ptep, ilog2(_PAGE_NO_EXEC));
UASM_i_ROTR(p, tmp, tmp, ilog2(_PAGE_GLOBAL) -
ilog2(_PAGE_NO_EXEC));
if (r4k_250MHZhwbug())
UASM_i_MTC0(p, 0, C0_ENTRYLO0);
UASM_i_MTC0(p, tmp, C0_ENTRYLO0); /* load it */
UASM_i_ROTR(p, ptep, ptep, ilog2(_PAGE_GLOBAL) -
ilog2(_PAGE_NO_EXEC));
} else {
UASM_i_SRL(p, tmp, tmp, ilog2(_PAGE_GLOBAL)); /*
convert to entrylo0 */
if (r4k_250MHZhwbug())
UASM_i_MTC0(p, 0, C0_ENTRYLO0);
UASM_i_MTC0(p, tmp, C0_ENTRYLO0); /* load it */
UASM_i_SRL(p, ptep, ptep, ilog2(_PAGE_GLOBAL)); /*
convert to entrylo1 */
if (r45k_bvahwbug())
uasm_i_mfc0(p, tmp, C0_INDEX);
}

If RIXI is enabled, it shifts the SW bits off the end of the register,
then rotates the RI/XI bits into place.

If RIXI is disabled, it shifts the SW bits + RI/XI bits off the end of
the register. It should not be setting bits 31:30 or 63:62, ever.

(A side issue here is that ROTR is a MIPS R2 instruction, so we could
never remove the old handler and use the RIXI version of the TLB
handler on an R1 machine.)

If setting EntryLo bits 31:30 for RI/XI is illegal on a 64-bit system
running a 32-bit kernel, I suspect we will have a problem with the
existing RIXI TLB update code, regardless of whether my changes are
applied.

2011-05-13 15:54:40

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Fri, May 13, 2011 at 08:46:18AM -0700, Kevin Cernekee wrote:

> >> On hardware that does not support RI/XI, EntryLo bits 31:30 / 63:62 will
> >> remain unset and RI/XI permissions will not be enforced.
> >
> > Nice idea but it breaks on 64-bit hardware running 32-bit kernels. ?On
> > those the RI/XI bits written to c0_entrylo0/1 31:30 will be interpreted as
> > physical address bits 37:36.
>
> Hmm, are you sure? (Unfortunately I do not have a 64-bit machine to
> test it on.)
>
> I did not touch David's existing build_update_entries(), which makes a
> point not to set the RI/XI bits when the RIXI feature is disabled:
>
> if (kernel_uses_smartmips_rixi) {
> UASM_i_SRL(p, tmp, tmp, ilog2(_PAGE_NO_EXEC));
> UASM_i_SRL(p, ptep, ptep, ilog2(_PAGE_NO_EXEC));
> UASM_i_ROTR(p, tmp, tmp, ilog2(_PAGE_GLOBAL) -
> ilog2(_PAGE_NO_EXEC));
> if (r4k_250MHZhwbug())
> UASM_i_MTC0(p, 0, C0_ENTRYLO0);
> UASM_i_MTC0(p, tmp, C0_ENTRYLO0); /* load it */
> UASM_i_ROTR(p, ptep, ptep, ilog2(_PAGE_GLOBAL) -
> ilog2(_PAGE_NO_EXEC));
> } else {
> UASM_i_SRL(p, tmp, tmp, ilog2(_PAGE_GLOBAL)); /*
> convert to entrylo0 */
> if (r4k_250MHZhwbug())
> UASM_i_MTC0(p, 0, C0_ENTRYLO0);
> UASM_i_MTC0(p, tmp, C0_ENTRYLO0); /* load it */
> UASM_i_SRL(p, ptep, ptep, ilog2(_PAGE_GLOBAL)); /*
> convert to entrylo1 */
> if (r45k_bvahwbug())
> uasm_i_mfc0(p, tmp, C0_INDEX);
> }
>
> If RIXI is enabled, it shifts the SW bits off the end of the register,
> then rotates the RI/XI bits into place.
>
> If RIXI is disabled, it shifts the SW bits + RI/XI bits off the end of
> the register. It should not be setting bits 31:30 or 63:62, ever.
>
> (A side issue here is that ROTR is a MIPS R2 instruction, so we could
> never remove the old handler and use the RIXI version of the TLB
> handler on an R1 machine.)
>
> If setting EntryLo bits 31:30 for RI/XI is illegal on a 64-bit system
> running a 32-bit kernel, I suspect we will have a problem with the
> existing RIXI TLB update code, regardless of whether my changes are
> applied.

I'm not totally certain with my explanation but it seemed like a good
working hypothesis. Jayachandran C. bisected this morning's linux-queue
on his Netlogic XLR which is MIPS64 R1 and found this comment causing
the problem.

Ralf

2011-05-13 16:55:24

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Fri, May 13, 2011 at 8:56 AM, Ralf Baechle <[email protected]> wrote:
> I'm not totally certain with my explanation but it seemed like a good
> working hypothesis. Jayachandran C. bisected this morning's linux-queue
> on his Netlogic XLR which is MIPS64 R1 and found this comment causing
> the problem.

Jayachandran, could you please confirm/deny the following:

Netlogic XLR is a MIPS64 R1 system.

You are running a 32-bit kernel.

You are using 64-bit physical addresses.

You are not enabling RI/XI.

The commit that caused the regression was "[PATCH 1/4] MIPS: Replace
_PAGE_READ with _PAGE_NO_READ" (not 2/4, 3/4, or 4/4).

Do you have a log showing the failure, or any other details of what happened?

Thanks.

2011-05-13 17:17:48

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On 05/13/2011 08:56 AM, Ralf Baechle wrote:
> On Fri, May 13, 2011 at 08:46:18AM -0700, Kevin Cernekee wrote:
>
>>>> On hardware that does not support RI/XI, EntryLo bits 31:30 / 63:62 will
>>>> remain unset and RI/XI permissions will not be enforced.
>>>
>>> Nice idea but it breaks on 64-bit hardware running 32-bit kernels. On
>>> those the RI/XI bits written to c0_entrylo0/1 31:30 will be interpreted as
>>> physical address bits 37:36.
>>
>> Hmm, are you sure? (Unfortunately I do not have a 64-bit machine to
>> test it on.)
>>
>> I did not touch David's existing build_update_entries(), which makes a
>> point not to set the RI/XI bits when the RIXI feature is disabled:
>>
>> if (kernel_uses_smartmips_rixi) {
>> UASM_i_SRL(p, tmp, tmp, ilog2(_PAGE_NO_EXEC));
>> UASM_i_SRL(p, ptep, ptep, ilog2(_PAGE_NO_EXEC));
>> UASM_i_ROTR(p, tmp, tmp, ilog2(_PAGE_GLOBAL) -
>> ilog2(_PAGE_NO_EXEC));
>> if (r4k_250MHZhwbug())
>> UASM_i_MTC0(p, 0, C0_ENTRYLO0);
>> UASM_i_MTC0(p, tmp, C0_ENTRYLO0); /* load it */
>> UASM_i_ROTR(p, ptep, ptep, ilog2(_PAGE_GLOBAL) -
>> ilog2(_PAGE_NO_EXEC));
>> } else {
>> UASM_i_SRL(p, tmp, tmp, ilog2(_PAGE_GLOBAL)); /*
>> convert to entrylo0 */
>> if (r4k_250MHZhwbug())
>> UASM_i_MTC0(p, 0, C0_ENTRYLO0);
>> UASM_i_MTC0(p, tmp, C0_ENTRYLO0); /* load it */
>> UASM_i_SRL(p, ptep, ptep, ilog2(_PAGE_GLOBAL)); /*
>> convert to entrylo1 */
>> if (r45k_bvahwbug())
>> uasm_i_mfc0(p, tmp, C0_INDEX);
>> }
>>
>> If RIXI is enabled, it shifts the SW bits off the end of the register,
>> then rotates the RI/XI bits into place.
>>
>> If RIXI is disabled, it shifts the SW bits + RI/XI bits off the end of
>> the register. It should not be setting bits 31:30 or 63:62, ever.
>>
>> (A side issue here is that ROTR is a MIPS R2 instruction, so we could
>> never remove the old handler and use the RIXI version of the TLB
>> handler on an R1 machine.)
>>
>> If setting EntryLo bits 31:30 for RI/XI is illegal on a 64-bit system
>> running a 32-bit kernel, I suspect we will have a problem with the
>> existing RIXI TLB update code, regardless of whether my changes are
>> applied.
>
> I'm not totally certain with my explanation but it seemed like a good
> working hypothesis. Jayachandran C. bisected this morning's linux-queue
> on his Netlogic XLR which is MIPS64 R1 and found this comment causing
> the problem.

He should dump out the TLB Refill handler with the patch set applied.
Perhaps it is erroneously getting in RI/XI mode.

Put #define DEBUG 1 as the first line of tlbex.c, then boot the kernel
with 'debug' on the command line. Also it is best to have early printk
enabled so you can see the dump.

David Daney.

2011-05-13 17:53:38

by Jayachandran C.

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Fri, May 13, 2011 at 09:55:21AM -0700, Kevin Cernekee wrote:
> On Fri, May 13, 2011 at 8:56 AM, Ralf Baechle <[email protected]> wrote:
> > I'm not totally certain with my explanation but it seemed like a good
> > working hypothesis. ?Jayachandran C. bisected this morning's linux-queue
> > on his Netlogic XLR which is MIPS64 R1 and found this comment causing
> > the problem.
>
> Jayachandran, could you please confirm/deny the following:
>
> Netlogic XLR is a MIPS64 R1 system.
>
> You are running a 32-bit kernel.
>
> You are using 64-bit physical addresses.
>
> You are not enabling RI/XI.
>
> The commit that caused the regression was "[PATCH 1/4] MIPS: Replace
> _PAGE_READ with _PAGE_NO_READ" (not 2/4, 3/4, or 4/4).
>
> Do you have a log showing the failure, or any other details of what happened?

Yes, it is a MIPS64R1 system with 64-bit (well 40bit) physical address, we
don't have rixi either on hardware on in kernel overrides. git bisect pointed
the specific patch.

And with your patch it works only on 64 bit compile, 32 bit kernel fails on
init with:
|malloc: subst.c:521: assertion botched
|free: called with already freed block argument

For 32-bit the config is nlm_xlr_defconfig in the source tree. Let me know if
you need any further info.

Regards,
JC.

2011-05-13 17:51:46

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Fri, May 13, 2011 at 10:36 AM, Jayachandran C.
<[email protected]> wrote:
> For 32-bit the config is nlm_xlr_defconfig in the source tree. Let me know if
> you need any further info.

Would you be able to dump out the TLB handlers in this configuration,
per David's suggestion?

Thanks.

2011-05-13 18:44:06

by Jayachandran C.

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Fri, May 13, 2011 at 10:51:44AM -0700, Kevin Cernekee wrote:
> On Fri, May 13, 2011 at 10:36 AM, Jayachandran C.
> <[email protected]> wrote:
> > For 32-bit the config is nlm_xlr_defconfig in the source tree. Let me know if
> > you need any further info.
>
> Would you be able to dump out the TLB handlers in this configuration,
> per David's suggestion?

The current linux-mips queue works for me, and I don't have the old source
or binaries with me anymore. You surely should be able build with
nlm_xlr_defconfig and see if the rixi is enabled in the build, if you want
any config register dump on the CPU, please let me know.

JC.

2011-05-13 18:57:32

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On 05/13/2011 11:45 AM, Jayachandran C. wrote:
> On Fri, May 13, 2011 at 10:51:44AM -0700, Kevin Cernekee wrote:
>> On Fri, May 13, 2011 at 10:36 AM, Jayachandran C.
>> <[email protected]> wrote:
>>> For 32-bit the config is nlm_xlr_defconfig in the source tree. Let me know if
>>> you need any further info.
>>
>> Would you be able to dump out the TLB handlers in this configuration,
>> per David's suggestion?
>
> The current linux-mips queue works for me, and I don't have the old source
> or binaries with me anymore. You surely should be able build with
> nlm_xlr_defconfig and see if the rixi is enabled in the build, if you want
> any config register dump on the CPU, please let me know.

The problem is that we don't have your hardware to test anything on.

Kevin's patches are a good cleanup, but we cannot use them if they break
things. So the ideal situation would be for people maintaining the
failing ports to help figure out where they fail.

David Daney

2011-05-13 22:09:16

by Arnaud Patard

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

2011-05-13 22:06:37

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Fri, May 13, 2011 at 11:45 AM, Jayachandran C.
<[email protected]> wrote:
> The current linux-mips queue works for me, and I don't have the old source
> or binaries with me anymore. You surely should be able build with
> nlm_xlr_defconfig and see if the rixi is enabled in the build, if you want
> any config register dump on the CPU, please let me know.

I was able to locate an old MIPS R5000 based system and get linux-mips
queue running on it. Here are the settings:

CONFIG_ARCH_DMA_ADDR_T_64BIT=y
CONFIG_64BIT_PHYS_ADDR=y
CONFIG_ARCH_PHYS_ADDR_T_64BIT=y
CONFIG_PHYS_ADDR_T_64BIT=y
CONFIG_SYS_SUPPORTS_32BIT_KERNEL=y
CONFIG_CPU_SUPPORTS_32BIT_KERNEL=y
CONFIG_SYS_SUPPORTS_64BIT_KERNEL=y
CONFIG_CPU_SUPPORTS_64BIT_KERNEL=y
CONFIG_32BIT=y
# CONFIG_64BIT is not set
CONFIG_SMP=y

This was intended to mimic the XLR configuration: 32-bit kernel on a
64-bit CPU with 64-bit physical addressing, SMP (albeit with 1 CPU),
no RI/XI.

After applying all 4 of my page cleanup patches, the system still
booted and ran normally.

Userspace is glibc with a bash shell (also tried uClibc w/bash, same
result). Since the reported assertion appeared to be in bash.

Jayachandran - how do you think we should debug this? It seems like
the problem only shows up on XLR. Since this is a relatively new
platform, is it possible that something else might be broken still?

FWIW, here are the TLB handlers:

Refill:

80000000: 401b2000 mfc0 k1,c0_context
80000004: 3c1a80a5 lui k0,0x80a5
80000008: 001bddc2 srl k1,k1,0x17
8000000c: 035bd821 addu k1,k0,k1
80000010: 401a4000 mfc0 k0,c0_badvaddr
80000014: 8f7b4000 lw k1,16384(k1)
80000018: 001ad542 srl k0,k0,0x15
8000001c: 001ad080 sll k0,k0,0x2
80000020: 037ad821 addu k1,k1,k0
80000024: 401a2000 mfc0 k0,c0_context
80000028: 8f7b0000 lw k1,0(k1)
8000002c: 335a0ff0 andi k0,k0,0xff0
80000030: 037ad821 addu k1,k1,k0
80000034: df7a0000 ld k0,0(k1)
80000038: df7b0008 ld k1,8(k1)
8000003c: 001ad1ba dsrl k0,k0,0x6
80000040: 409a1000 mtc0 k0,c0_entrylo0
80000044: 001bd9ba dsrl k1,k1,0x6
80000048: 409b1800 mtc0 k1,c0_entrylo1
8000004c: 00000000 nop
80000050: 42000006 tlbwr
80000054: 42000018 eret

Load:

801fe000: 401b2000 mfc0 k1,c0_context
801fe004: 3c1a80a5 lui k0,0x80a5
801fe008: 001bddc2 srl k1,k1,0x17
801fe00c: 035bd821 addu k1,k0,k1
801fe010: 401a4000 mfc0 k0,c0_badvaddr
801fe014: 8f7b4000 lw k1,16384(k1)
801fe018: 001ad542 srl k0,k0,0x15
801fe01c: 001ad080 sll k0,k0,0x2
801fe020: 037ad821 addu k1,k1,k0
801fe024: 401a4000 mfc0 k0,c0_badvaddr
801fe028: 8f7b0000 lw k1,0(k1)
801fe02c: 001ad242 srl k0,k0,0x9
801fe030: 335a0ff8 andi k0,k0,0xff8
801fe034: 037ad821 addu k1,k1,k0
801fe038: d37a0000 lld k0,0(k1)
801fe03c: 42000008 tlbp
801fe040: 335a0001 andi k0,k0,0x1
801fe044: 13400010 beqz k0,0x801fe088
801fe048: d37a0000 lld k0,0(k1)
801fe04c: 375a0084 ori k0,k0,0x84
801fe050: f37a0000 scd k0,0(k1)
801fe054: 1340fff8 beqz k0,0x801fe038
801fe058: 00000000 nop
801fe05c: 377b0008 ori k1,k1,0x8
801fe060: 3b7b0008 xori k1,k1,0x8
801fe064: df7a0000 ld k0,0(k1)
801fe068: df7b0008 ld k1,8(k1)
801fe06c: 001ad1ba dsrl k0,k0,0x6
801fe070: 409a1000 mtc0 k0,c0_entrylo0
801fe074: 001bd9ba dsrl k1,k1,0x6
801fe078: 409b1800 mtc0 k1,c0_entrylo1
801fe07c: 00000000 nop
801fe080: 42000002 tlbwi
801fe084: 42000018 eret
801fe088: 080036d8 j 0x8000db60
801fe08c: 00000000 nop

Store:

801fe200: 401b2000 mfc0 k1,c0_context
801fe204: 3c1a80a5 lui k0,0x80a5
801fe208: 001bddc2 srl k1,k1,0x17
801fe20c: 035bd821 addu k1,k0,k1
801fe210: 401a4000 mfc0 k0,c0_badvaddr
801fe214: 8f7b4000 lw k1,16384(k1)
801fe218: 001ad542 srl k0,k0,0x15
801fe21c: 001ad080 sll k0,k0,0x2
801fe220: 037ad821 addu k1,k1,k0
801fe224: 401a4000 mfc0 k0,c0_badvaddr
801fe228: 8f7b0000 lw k1,0(k1)
801fe22c: 001ad242 srl k0,k0,0x9
801fe230: 335a0ff8 andi k0,k0,0xff8
801fe234: 037ad821 addu k1,k1,k0
801fe238: d37a0000 lld k0,0(k1)
801fe23c: 42000008 tlbp
801fe240: 335a0003 andi k0,k0,0x3
801fe244: 3b5a0003 xori k0,k0,0x3
801fe248: 17400010 bnez k0,0x801fe28c
801fe24c: d37a0000 lld k0,0(k1)
801fe250: 375a018c ori k0,k0,0x18c
801fe254: f37a0000 scd k0,0(k1)
801fe258: 1340fff7 beqz k0,0x801fe238
801fe25c: 00000000 nop
801fe260: 377b0008 ori k1,k1,0x8
801fe264: 3b7b0008 xori k1,k1,0x8
801fe268: df7a0000 ld k0,0(k1)
801fe26c: df7b0008 ld k1,8(k1)
801fe270: 001ad1ba dsrl k0,k0,0x6
801fe274: 409a1000 mtc0 k0,c0_entrylo0
801fe278: 001bd9ba dsrl k1,k1,0x6
801fe27c: 409b1800 mtc0 k1,c0_entrylo1
801fe280: 00000000 nop
801fe284: 42000002 tlbwi
801fe288: 42000018 eret
801fe28c: 0800371d j 0x8000dc74
801fe290: 00000000 nop

Modify:

801fe400: 401b2000 mfc0 k1,c0_context
801fe404: 3c1a80a5 lui k0,0x80a5
801fe408: 001bddc2 srl k1,k1,0x17
801fe40c: 035bd821 addu k1,k0,k1
801fe410: 401a4000 mfc0 k0,c0_badvaddr
801fe414: 8f7b4000 lw k1,16384(k1)
801fe418: 001ad542 srl k0,k0,0x15
801fe41c: 001ad080 sll k0,k0,0x2
801fe420: 037ad821 addu k1,k1,k0
801fe424: 401a4000 mfc0 k0,c0_badvaddr
801fe428: 8f7b0000 lw k1,0(k1)
801fe42c: 001ad242 srl k0,k0,0x9
801fe430: 335a0ff8 andi k0,k0,0xff8
801fe434: 037ad821 addu k1,k1,k0
801fe438: d37a0000 lld k0,0(k1)
801fe43c: 42000008 tlbp
801fe440: 335a0002 andi k0,k0,0x2
801fe444: 13400010 beqz k0,0x801fe488
801fe448: d37a0000 lld k0,0(k1)
801fe44c: 375a018c ori k0,k0,0x18c
801fe450: f37a0000 scd k0,0(k1)
801fe454: 1340fff8 beqz k0,0x801fe438
801fe458: 00000000 nop
801fe45c: 377b0008 ori k1,k1,0x8
801fe460: 3b7b0008 xori k1,k1,0x8
801fe464: df7a0000 ld k0,0(k1)
801fe468: df7b0008 ld k1,8(k1)
801fe46c: 001ad1ba dsrl k0,k0,0x6
801fe470: 409a1000 mtc0 k0,c0_entrylo0
801fe474: 001bd9ba dsrl k1,k1,0x6
801fe478: 409b1800 mtc0 k1,c0_entrylo1
801fe47c: 00000000 nop
801fe480: 42000002 tlbwi
801fe484: 42000018 eret
801fe488: 0800371d j 0x8000dc74
801fe48c: 00000000 nop

2011-05-14 05:15:09

by Jayachandran C.

[permalink] [raw]

Subject: Re: [PATCH 1/4] MIPS: Replace _PAGE_READ with _PAGE_NO_READ

On Fri, May 13, 2011 at 03:06:35PM -0700, Kevin Cernekee wrote:
> On Fri, May 13, 2011 at 11:45 AM, Jayachandran C.
> <[email protected]> wrote:
> > The current linux-mips queue works for me, and I don't have the old source
> > or binaries with me anymore. You surely should be able build with
> > nlm_xlr_defconfig and see if the rixi is enabled in the build, if you want
> > any config register dump on the CPU, please let me know.
>
> I was able to locate an old MIPS R5000 based system and get linux-mips
> queue running on it. Here are the settings:
>
> CONFIG_ARCH_DMA_ADDR_T_64BIT=y
> CONFIG_64BIT_PHYS_ADDR=y
> CONFIG_ARCH_PHYS_ADDR_T_64BIT=y
> CONFIG_PHYS_ADDR_T_64BIT=y
> CONFIG_SYS_SUPPORTS_32BIT_KERNEL=y
> CONFIG_CPU_SUPPORTS_32BIT_KERNEL=y
> CONFIG_SYS_SUPPORTS_64BIT_KERNEL=y
> CONFIG_CPU_SUPPORTS_64BIT_KERNEL=y
> CONFIG_32BIT=y
> # CONFIG_64BIT is not set
> CONFIG_SMP=y
>
> This was intended to mimic the XLR configuration: 32-bit kernel on a
> 64-bit CPU with 64-bit physical addressing, SMP (albeit with 1 CPU),
> no RI/XI.
>
> After applying all 4 of my page cleanup patches, the system still
> booted and ran normally.
>
> Userspace is glibc with a bash shell (also tried uClibc w/bash, same
> result). Since the reported assertion appeared to be in bash.
>
> Jayachandran - how do you think we should debug this? It seems like
> the problem only shows up on XLR. Since this is a relatively new
> platform, is it possible that something else might be broken still?

Can you send me the patchset which works on top of queue with any
debugging you want enabled? I can try that and send you the results.

It is also possible that something is broken with the XLR platform code,
it is currently almost straight r4k...

Thanks,
JC.

2011-05-14 06:02:32