[PATCH v8 07/16] ARM: LPAE: Introduce the 3-level page table format definitions

[catalin.marinas@arm.com (Catalin Marinas)]

On Thu, Nov 10, 2011 at 09:53:53PM +0000, Nicolas Pitre wrote:
> On Mon, 7 Nov 2011, Catalin Marinas wrote:
> 
> > This patch introduces the pgtable-3level*.h files with definitions
> > specific to the LPAE page table format (3 levels of page tables).
> [...]
> > diff --git a/arch/arm/include/asm/pgtable-3level-hwdef.h b/arch/arm/include/asm/pgtable-3level-hwdef.h
> > new file mode 100644
> > index 0000000..7c238a3
> > --- /dev/null
> > +++ b/arch/arm/include/asm/pgtable-3level-hwdef.h
> [...]
> > +#ifdef __ASSEMBLY__
> > +/* avoid 'shift count out of range' warning */
> > +#define PMD_SECT_XN		(0)
> > +#else
> > +#define PMD_SECT_XN		((pmdval_t)1 << 54)
> > +#endif
> 
> Please don't do that.  Having a symbol that changes value depending on 
> the type of code is evil.  If PMD_SECT_XN is actually used in assembly 
> code then you have to deal with it, not blindly redefine it as zero.
> In my compilation test I didn't see any such warning anyway.

When compiling for LPAE, I get:

arch/arm/mm/proc-v7.S: Assembler messages:
arch/arm/mm/proc-v7.S|349| Warning: shift count out of range (54 is not between 0 and 31)
arch/arm/mm/proc-v7.S|359| Warning: shift count out of range (54 is not between 0 and 31)

because we have a .long definition that cannot accommodate bigger
values. I also wouldn't change the __cpu_io_mmu_flags type to long long.

But I can deal with this directly in the head.S file. The fix-up would
look like this:

diff --git a/arch/arm/include/asm/pgtable-3level-hwdef.h b/arch/arm/include/asm/pgtable-3level-hwdef.h
index 7c238a3..d795282 100644
--- a/arch/arm/include/asm/pgtable-3level-hwdef.h
+++ b/arch/arm/include/asm/pgtable-3level-hwdef.h
@@ -41,12 +41,7 @@
 #define PMD_SECT_S		(_AT(pmdval_t, 3) << 8)
 #define PMD_SECT_AF		(_AT(pmdval_t, 1) << 10)
 #define PMD_SECT_nG		(_AT(pmdval_t, 1) << 11)
-#ifdef __ASSEMBLY__
-/* avoid 'shift count out of range' warning */
-#define PMD_SECT_XN		(0)
-#else
-#define PMD_SECT_XN		((pmdval_t)1 << 54)
-#endif
+#define PMD_SECT_XN		(_AT(pmdval_t, 1) << 54)
 #define PMD_SECT_AP_WRITE	(_AT(pmdval_t, 0))
 #define PMD_SECT_AP_READ	(_AT(pmdval_t, 0))
 #define PMD_SECT_TEX(x)		(_AT(pmdval_t, 0))
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index a400a4d..a1df2ac 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -278,6 +278,8 @@ __create_page_tables:
 	orr	r3, r7, r3, lsl #SECTION_SHIFT
 #ifdef CONFIG_ARM_LPAE
 	mov	r7, #1 << (54 - 32)		@ XN
+#else
+	orr	r3, r3, #PMD_SECT_XN
 #endif
 1:	str	r3, [r0], #4
 #ifdef CONFIG_ARM_LPAE
diff --git a/arch/arm/mm/proc-v7.S b/arch/arm/mm/proc-v7.S
index fc224bc..0b68a93 100644
--- a/arch/arm/mm/proc-v7.S
+++ b/arch/arm/mm/proc-v7.S
@@ -303,7 +303,7 @@ __v7_setup_stack:
 			PMD_SECT_AF | PMD_FLAGS_SMP | \mm_mmuflags)
 	ALT_UP(.long	PMD_TYPE_SECT | PMD_SECT_AP_WRITE | PMD_SECT_AP_READ | \
 			PMD_SECT_AF | PMD_FLAGS_UP | \mm_mmuflags)
-	.long	PMD_TYPE_SECT | PMD_SECT_XN | PMD_SECT_AP_WRITE | \
+	.long	PMD_TYPE_SECT | PMD_SECT_AP_WRITE | \
 		PMD_SECT_AP_READ | PMD_SECT_AF | \io_mmuflags
 	W(b)	\initfunc
 	.long	cpu_arch_name

> > diff --git a/arch/arm/include/asm/pgtable-3level-types.h b/arch/arm/include/asm/pgtable-3level-types.h
> > new file mode 100644
> > index 0000000..921aa30
> > --- /dev/null
> > +++ b/arch/arm/include/asm/pgtable-3level-types.h
> > @@ -0,0 +1,70 @@
> [...]
> > +typedef u64 pteval_t;
> > +typedef u64 pmdval_t;
> > +typedef u64 pgdval_t;
> > +
> > +#undef STRICT_MM_TYPECHECKS
> > +
> > +#ifdef STRICT_MM_TYPECHECKS
> > +
> > +/*
> > + * These are used to make use of C type-checking..
> > + */
> > +typedef struct { pteval_t pte; } pte_t;
> > +typedef struct { pmdval_t pmd; } pmd_t;
> > +typedef struct { pgdval_t pgd; } pgd_t;
> > +typedef struct { pteval_t pgprot; } pgprot_t;
> > +
> > +#define pte_val(x)      ((x).pte)
> > +#define pmd_val(x)      ((x).pmd)
> > +#define pgd_val(x)	((x).pgd)
> > +#define pgprot_val(x)   ((x).pgprot)
> > +
> > +#define __pte(x)        ((pte_t) { (x) } )
> > +#define __pmd(x)        ((pmd_t) { (x) } )
> > +#define __pgd(x)	((pgd_t) { (x) } )
> > +#define __pgprot(x)     ((pgprot_t) { (x) } )
> > +
> > +#else	/* !STRICT_MM_TYPECHECKS */
> > +
> > +typedef pteval_t pte_t;
> > +typedef pmdval_t pmd_t;
> > +typedef pgdval_t pgd_t;
> > +typedef pteval_t pgprot_t;
> > +
> > +#define pte_val(x)	(x)
> > +#define pmd_val(x)	(x)
> > +#define pgd_val(x)	(x)
> > +#define pgprot_val(x)	(x)
> > +
> > +#define __pte(x)	(x)
> > +#define __pmd(x)	(x)
> > +#define __pgd(x)	(x)
> > +#define __pgprot(x)	(x)
> > +
> > +#endif	/* STRICT_MM_TYPECHECKS */
> 
> Isn't the vast majority of this file actually common with the content of 
> pgtable-2level-types.h?  Except for the basic types and pgd handling, 
> this looks pretty similar.

The pgd_t and pgd_val definitions are different. But pgd_t depends on
the pmd_t definition so we would end up with #ifdef's in a common file
(or two includes for the type headers). IMHO, it's cleaner this way.

-- 
Catalin