[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[R Sricharan]

With LPAE enabled, alloc_init_section() does not map the
entire address space for unaligned addresses.

The issue also reproduced with CMA + LPAE. CMA tries to map 16MB
with page granularity mappings during boot. alloc_init_pte()
is called and out of 16MB, only 2MB gets mapped and rest remains
unaccessible.

Because of this OMAP5 boot is broken with CMA + LPAE enabled.
Fix the issue by ensuring that the entire addresses are
mapped.

Signed-off-by: R Sricharan <r.sricharan@ti.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Christoffer Dall <chris@cloudcar.com>
Cc: Russell King <linux@arm.linux.org.uk>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Tested-by: Christoffer Dall  <chris@cloudcar.com>
---
 [V2] Moved the loop to alloc_init_pte as per Russell's
     feedback and changed the subject accordingly.
     Using PMD_XXX instead of SECTION_XXX to avoid
     different loop increments with/without LPAE.

 [v3] Removed the dummy variable phys and updated
      the commit log for CMA case.

 [v4] Resending with updated change log and 
      updating the tags.

 arch/arm/mm/mmu.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index f8388ad..b94c313 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -569,11 +569,23 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
 				  unsigned long end, unsigned long pfn,
 				  const struct mem_type *type)
 {
-	pte_t *pte = early_pte_alloc(pmd, addr, type->prot_l1);
+	unsigned long next;
+	pte_t *pte;
+
 	do {
-		set_pte_ext(pte, pfn_pte(pfn, __pgprot(type->prot_pte)), 0);
-		pfn++;
-	} while (pte++, addr += PAGE_SIZE, addr != end);
+		if ((end-addr) & PMD_MASK)
+			next = (addr + PMD_SIZE) & PMD_MASK;
+		else
+			next = end;
+
+		pte = early_pte_alloc(pmd, addr, type->prot_l1);
+		do {
+			set_pte_ext(pte, pfn_pte(pfn,
+					__pgprot(type->prot_pte)), 0);
+			pfn++;
+		} while (pte++, addr += PAGE_SIZE, addr != next);
+
+	} while (pmd++, addr = next, addr != end);
 }
 
 static void __init alloc_init_section(pud_t *pud, unsigned long addr,
-- 
1.7.9.5

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Santosh Shilimkar]

Catalin, Russell,

On Tuesday 29 January 2013 08:37 PM, R Sricharan wrote:
> With LPAE enabled, alloc_init_section() does not map the
> entire address space for unaligned addresses.
>
> The issue also reproduced with CMA + LPAE. CMA tries to map 16MB
> with page granularity mappings during boot. alloc_init_pte()
> is called and out of 16MB, only 2MB gets mapped and rest remains
> unaccessible.
>
> Because of this OMAP5 boot is broken with CMA + LPAE enabled.
> Fix the issue by ensuring that the entire addresses are
> mapped.
>
> Signed-off-by: R Sricharan <r.sricharan@ti.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Christoffer Dall <chris@cloudcar.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Tested-by: Christoffer Dall  <chris@cloudcar.com>
> ---
This patch has been on the list for quite some time. Its
a bug fix and should into mainline. Christoffer already
stumbled on the same issue and has to spend debugging the
known issue.

Can you please give your ack's if it is fine to get into
patch system ?


>   [V2] Moved the loop to alloc_init_pte as per Russell's
>       feedback and changed the subject accordingly.
>       Using PMD_XXX instead of SECTION_XXX to avoid
>       different loop increments with/without LPAE.
>
>   [v3] Removed the dummy variable phys and updated
>        the commit log for CMA case.
>
>   [v4] Resending with updated change log and
>        updating the tags.
>
>   arch/arm/mm/mmu.c |   20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index f8388ad..b94c313 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -569,11 +569,23 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
>   				  unsigned long end, unsigned long pfn,
>   				  const struct mem_type *type)
>   {
> -	pte_t *pte = early_pte_alloc(pmd, addr, type->prot_l1);
> +	unsigned long next;
> +	pte_t *pte;
> +
>   	do {
> -		set_pte_ext(pte, pfn_pte(pfn, __pgprot(type->prot_pte)), 0);
> -		pfn++;
> -	} while (pte++, addr += PAGE_SIZE, addr != end);
> +		if ((end-addr) & PMD_MASK)
> +			next = (addr + PMD_SIZE) & PMD_MASK;
> +		else
> +			next = end;
> +
> +		pte = early_pte_alloc(pmd, addr, type->prot_l1);
> +		do {
> +			set_pte_ext(pte, pfn_pte(pfn,
> +					__pgprot(type->prot_pte)), 0);
> +			pfn++;
> +		} while (pte++, addr += PAGE_SIZE, addr != next);
> +
> +	} while (pmd++, addr = next, addr != end);
>   }
>
>   static void __init alloc_init_section(pud_t *pud, unsigned long addr,
>

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Catalin Marinas]

On Tue, Jan 29, 2013 at 03:07:16PM +0000, R Sricharan wrote:
> With LPAE enabled, alloc_init_section() does not map the
> entire address space for unaligned addresses.
> 
> The issue also reproduced with CMA + LPAE. CMA tries to map 16MB
> with page granularity mappings during boot. alloc_init_pte()
> is called and out of 16MB, only 2MB gets mapped and rest remains
> unaccessible.
> 
> Because of this OMAP5 boot is broken with CMA + LPAE enabled.
> Fix the issue by ensuring that the entire addresses are
> mapped.
> 
> Signed-off-by: R Sricharan <r.sricharan@ti.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Christoffer Dall <chris@cloudcar.com>
> Cc: Russell King <linux@arm.linux.org.uk>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Tested-by: Christoffer Dall  <chris@cloudcar.com>
> ---
>  [V2] Moved the loop to alloc_init_pte as per Russell's
>      feedback and changed the subject accordingly.
>      Using PMD_XXX instead of SECTION_XXX to avoid
>      different loop increments with/without LPAE.
> 
>  [v3] Removed the dummy variable phys and updated
>       the commit log for CMA case.
> 
>  [v4] Resending with updated change log and 
>       updating the tags.
> 
>  arch/arm/mm/mmu.c |   20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index f8388ad..b94c313 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -569,11 +569,23 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
>  				  unsigned long end, unsigned long pfn,
>  				  const struct mem_type *type)
>  {
> -	pte_t *pte = early_pte_alloc(pmd, addr, type->prot_l1);
> +	unsigned long next;
> +	pte_t *pte;
> +
>  	do {
> -		set_pte_ext(pte, pfn_pte(pfn, __pgprot(type->prot_pte)), 0);
> -		pfn++;
> -	} while (pte++, addr += PAGE_SIZE, addr != end);
> +		if ((end-addr) & PMD_MASK)
> +			next = (addr + PMD_SIZE) & PMD_MASK;
> +		else
> +			next = end;

Can use pmd_addr_end(addr, end) here?

> +		pte = early_pte_alloc(pmd, addr, type->prot_l1);
> +		do {
> +			set_pte_ext(pte, pfn_pte(pfn,
> +					__pgprot(type->prot_pte)), 0);
> +			pfn++;
> +		} while (pte++, addr += PAGE_SIZE, addr != next);
> +
> +	} while (pmd++, addr = next, addr != end);

I would actually keep the loop in alloc_init_section(). There is even a
comment in there saying "no need to loop" but you actually moved the
loop in alloc_init_pte().

I'm proposing a simpler patch below (only lightly tested on VE/C-A9).
The only difference is that we do more flush_pmd_entry() calls but I'm
not really bothered, it's during boot and you won't notice.


diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 9c82f98..eaa8ba8 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -205,6 +205,11 @@ static inline pte_t *pmd_page_vaddr(pmd_t pmd)
 
 #define pte_present_user(pte)  (pte_present(pte) && (pte_val(pte) & L_PTE_USER))
 
+#define section_addr_end(addr, end)						\
+({	unsigned long __boundary = ((addr) + SECTION_SIZE) & SECTION_MASK;	\
+	(__boundary - 1 < (end) - 1)? __boundary: (end);			\
+})
+
 #if __LINUX_ARM_ARCH__ < 6
 static inline void __sync_icache_dcache(pte_t pteval)
 {
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 9f06102..0d0faed 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -581,34 +581,19 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
 				      const struct mem_type *type)
 {
 	pmd_t *pmd = pmd_offset(pud, addr);
+	unsigned long next;
 
-	/*
-	 * Try a section mapping - end, addr and phys must all be aligned
-	 * to a section boundary.  Note that PMDs refer to the individual
-	 * L1 entries, whereas PGDs refer to a group of L1 entries making
-	 * up one logical pointer to an L2 table.
-	 */
-	if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
-		pmd_t *p = pmd;
-
-#ifndef CONFIG_ARM_LPAE
-		if (addr & SECTION_SIZE)
-			pmd++;
-#endif
-
-		do {
+	do {
+		next = section_addr_end(addr, end);
+		/* try section mapping first */
+		if (((addr | next | phys) & ~SECTION_MASK) == 0) {
 			*pmd = __pmd(phys | type->prot_sect);
-			phys += SECTION_SIZE;
-		} while (pmd++, addr += SECTION_SIZE, addr != end);
-
-		flush_pmd_entry(p);
-	} else {
-		/*
-		 * No need to loop; pte's aren't interested in the
-		 * individual L1 entries.
-		 */
-		alloc_init_pte(pmd, addr, end, __phys_to_pfn(phys), type);
-	}
+			flush_pmd_entry(pmd);
+		} else {
+			alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), type);
+		}
+		phys += next - addr;
+	} while (pmd++, addr = next, addr != end);
 }
 
 static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Christoffer Dall]

On Fri, Feb 1, 2013 at 11:26 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Tue, Jan 29, 2013 at 03:07:16PM +0000, R Sricharan wrote:
>> With LPAE enabled, alloc_init_section() does not map the
>> entire address space for unaligned addresses.
>>
>> The issue also reproduced with CMA + LPAE. CMA tries to map 16MB
>> with page granularity mappings during boot. alloc_init_pte()
>> is called and out of 16MB, only 2MB gets mapped and rest remains
>> unaccessible.
>>
>> Because of this OMAP5 boot is broken with CMA + LPAE enabled.
>> Fix the issue by ensuring that the entire addresses are
>> mapped.
>>
>> Signed-off-by: R Sricharan <r.sricharan@ti.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Christoffer Dall <chris@cloudcar.com>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Tested-by: Christoffer Dall  <chris@cloudcar.com>
>> ---
>>  [V2] Moved the loop to alloc_init_pte as per Russell's
>>      feedback and changed the subject accordingly.
>>      Using PMD_XXX instead of SECTION_XXX to avoid
>>      different loop increments with/without LPAE.
>>
>>  [v3] Removed the dummy variable phys and updated
>>       the commit log for CMA case.
>>
>>  [v4] Resending with updated change log and
>>       updating the tags.
>>
>>  arch/arm/mm/mmu.c |   20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> index f8388ad..b94c313 100644
>> --- a/arch/arm/mm/mmu.c
>> +++ b/arch/arm/mm/mmu.c
>> @@ -569,11 +569,23 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
>>                                 unsigned long end, unsigned long pfn,
>>                                 const struct mem_type *type)
>>  {
>> -     pte_t *pte = early_pte_alloc(pmd, addr, type->prot_l1);
>> +     unsigned long next;
>> +     pte_t *pte;
>> +
>>       do {
>> -             set_pte_ext(pte, pfn_pte(pfn, __pgprot(type->prot_pte)), 0);
>> -             pfn++;
>> -     } while (pte++, addr += PAGE_SIZE, addr != end);
>> +             if ((end-addr) & PMD_MASK)
>> +                     next = (addr + PMD_SIZE) & PMD_MASK;
>> +             else
>> +                     next = end;
>
> Can use pmd_addr_end(addr, end) here?
>
>> +             pte = early_pte_alloc(pmd, addr, type->prot_l1);
>> +             do {
>> +                     set_pte_ext(pte, pfn_pte(pfn,
>> +                                     __pgprot(type->prot_pte)), 0);
>> +                     pfn++;
>> +             } while (pte++, addr += PAGE_SIZE, addr != next);
>> +
>> +     } while (pmd++, addr = next, addr != end);
>
> I would actually keep the loop in alloc_init_section(). There is even a
> comment in there saying "no need to loop" but you actually moved the
> loop in alloc_init_pte().
>
> I'm proposing a simpler patch below (only lightly tested on VE/C-A9).
> The only difference is that we do more flush_pmd_entry() calls but I'm
> not really bothered, it's during boot and you won't notice.
>
>
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 9c82f98..eaa8ba8 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -205,6 +205,11 @@ static inline pte_t *pmd_page_vaddr(pmd_t pmd)
>
>  #define pte_present_user(pte)  (pte_present(pte) && (pte_val(pte) & L_PTE_USER))
>
> +#define section_addr_end(addr, end)                                            \
> +({     unsigned long __boundary = ((addr) + SECTION_SIZE) & SECTION_MASK;      \
> +       (__boundary - 1 < (end) - 1)? __boundary: (end);                        \
> +})
> +
>  #if __LINUX_ARM_ARCH__ < 6
>  static inline void __sync_icache_dcache(pte_t pteval)
>  {
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 9f06102..0d0faed 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -581,34 +581,19 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
>                                       const struct mem_type *type)
>  {
>         pmd_t *pmd = pmd_offset(pud, addr);
> +       unsigned long next;
>
> -       /*
> -        * Try a section mapping - end, addr and phys must all be aligned
> -        * to a section boundary.  Note that PMDs refer to the individual
> -        * L1 entries, whereas PGDs refer to a group of L1 entries making
> -        * up one logical pointer to an L2 table.
> -        */
> -       if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
> -               pmd_t *p = pmd;
> -
> -#ifndef CONFIG_ARM_LPAE
> -               if (addr & SECTION_SIZE)
> -                       pmd++;
> -#endif
> -
> -               do {
> +       do {
> +               next = section_addr_end(addr, end);
> +               /* try section mapping first */
> +               if (((addr | next | phys) & ~SECTION_MASK) == 0) {
>                         *pmd = __pmd(phys | type->prot_sect);
> -                       phys += SECTION_SIZE;
> -               } while (pmd++, addr += SECTION_SIZE, addr != end);
> -
> -               flush_pmd_entry(p);
> -       } else {
> -               /*
> -                * No need to loop; pte's aren't interested in the
> -                * individual L1 entries.
> -                */
> -               alloc_init_pte(pmd, addr, end, __phys_to_pfn(phys), type);
> -       }
> +                       flush_pmd_entry(pmd);
> +               } else {
> +                       alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), type);

aren't you wasting memory here? The pte doesn't alloc a full page, but
the memblock allocator allocates a full page right?

I thought this was the rationale behind Russell's previous comments on
Santosh's earlier patch version.

> +               }
> +               phys += next - addr;
> +       } while (pmd++, addr = next, addr != end);
>  }
>
>  static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Catalin Marinas]

On Fri, Feb 01, 2013 at 04:32:54PM +0000, Christoffer Dall wrote:
> On Fri, Feb 1, 2013 at 11:26 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Tue, Jan 29, 2013 at 03:07:16PM +0000, R Sricharan wrote:
> >> With LPAE enabled, alloc_init_section() does not map the
> >> entire address space for unaligned addresses.
> >>
> >> The issue also reproduced with CMA + LPAE. CMA tries to map 16MB
> >> with page granularity mappings during boot. alloc_init_pte()
> >> is called and out of 16MB, only 2MB gets mapped and rest remains
> >> unaccessible.
> >>
> >> Because of this OMAP5 boot is broken with CMA + LPAE enabled.
> >> Fix the issue by ensuring that the entire addresses are
> >> mapped.
> >>
> >> Signed-off-by: R Sricharan <r.sricharan@ti.com>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Christoffer Dall <chris@cloudcar.com>
> >> Cc: Russell King <linux@arm.linux.org.uk>
> >> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >> Tested-by: Christoffer Dall  <chris@cloudcar.com>
> >> ---
> >>  [V2] Moved the loop to alloc_init_pte as per Russell's
> >>      feedback and changed the subject accordingly.
> >>      Using PMD_XXX instead of SECTION_XXX to avoid
> >>      different loop increments with/without LPAE.
> >>
> >>  [v3] Removed the dummy variable phys and updated
> >>       the commit log for CMA case.
> >>
> >>  [v4] Resending with updated change log and
> >>       updating the tags.
> >>
> >>  arch/arm/mm/mmu.c |   20 ++++++++++++++++----
> >>  1 file changed, 16 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> >> index f8388ad..b94c313 100644
> >> --- a/arch/arm/mm/mmu.c
> >> +++ b/arch/arm/mm/mmu.c
> >> @@ -569,11 +569,23 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
> >>                                 unsigned long end, unsigned long pfn,
> >>                                 const struct mem_type *type)
> >>  {
> >> -     pte_t *pte = early_pte_alloc(pmd, addr, type->prot_l1);
> >> +     unsigned long next;
> >> +     pte_t *pte;
> >> +
> >>       do {
> >> -             set_pte_ext(pte, pfn_pte(pfn, __pgprot(type->prot_pte)), 0);
> >> -             pfn++;
> >> -     } while (pte++, addr += PAGE_SIZE, addr != end);
> >> +             if ((end-addr) & PMD_MASK)
> >> +                     next = (addr + PMD_SIZE) & PMD_MASK;
> >> +             else
> >> +                     next = end;
> >
> > Can use pmd_addr_end(addr, end) here?
> >
> >> +             pte = early_pte_alloc(pmd, addr, type->prot_l1);
> >> +             do {
> >> +                     set_pte_ext(pte, pfn_pte(pfn,
> >> +                                     __pgprot(type->prot_pte)), 0);
> >> +                     pfn++;
> >> +             } while (pte++, addr += PAGE_SIZE, addr != next);
> >> +
> >> +     } while (pmd++, addr = next, addr != end);
> >
> > I would actually keep the loop in alloc_init_section(). There is even a
> > comment in there saying "no need to loop" but you actually moved the
> > loop in alloc_init_pte().
> >
> > I'm proposing a simpler patch below (only lightly tested on VE/C-A9).
> > The only difference is that we do more flush_pmd_entry() calls but I'm
> > not really bothered, it's during boot and you won't notice.
> >
> >
> > diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> > index 9c82f98..eaa8ba8 100644
> > --- a/arch/arm/include/asm/pgtable.h
> > +++ b/arch/arm/include/asm/pgtable.h
> > @@ -205,6 +205,11 @@ static inline pte_t *pmd_page_vaddr(pmd_t pmd)
> >
> >  #define pte_present_user(pte)  (pte_present(pte) && (pte_val(pte) & L_PTE_USER))
> >
> > +#define section_addr_end(addr, end)                                            \
> > +({     unsigned long __boundary = ((addr) + SECTION_SIZE) & SECTION_MASK;      \
> > +       (__boundary - 1 < (end) - 1)? __boundary: (end);                        \
> > +})
> > +
> >  #if __LINUX_ARM_ARCH__ < 6
> >  static inline void __sync_icache_dcache(pte_t pteval)
> >  {
> > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> > index 9f06102..0d0faed 100644
> > --- a/arch/arm/mm/mmu.c
> > +++ b/arch/arm/mm/mmu.c
> > @@ -581,34 +581,19 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
> >                                       const struct mem_type *type)
> >  {
> >         pmd_t *pmd = pmd_offset(pud, addr);
> > +       unsigned long next;
> >
> > -       /*
> > -        * Try a section mapping - end, addr and phys must all be aligned
> > -        * to a section boundary.  Note that PMDs refer to the individual
> > -        * L1 entries, whereas PGDs refer to a group of L1 entries making
> > -        * up one logical pointer to an L2 table.
> > -        */
> > -       if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
> > -               pmd_t *p = pmd;
> > -
> > -#ifndef CONFIG_ARM_LPAE
> > -               if (addr & SECTION_SIZE)
> > -                       pmd++;
> > -#endif
> > -
> > -               do {
> > +       do {
> > +               next = section_addr_end(addr, end);
> > +               /* try section mapping first */
> > +               if (((addr | next | phys) & ~SECTION_MASK) == 0) {
> >                         *pmd = __pmd(phys | type->prot_sect);
> > -                       phys += SECTION_SIZE;
> > -               } while (pmd++, addr += SECTION_SIZE, addr != end);
> > -
> > -               flush_pmd_entry(p);
> > -       } else {
> > -               /*
> > -                * No need to loop; pte's aren't interested in the
> > -                * individual L1 entries.
> > -                */
> > -               alloc_init_pte(pmd, addr, end, __phys_to_pfn(phys), type);
> > -       }
> > +                       flush_pmd_entry(pmd);
> > +               } else {
> > +                       alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), type);
> 
> aren't you wasting memory here? The pte doesn't alloc a full page, but
> the memblock allocator allocates a full page right?
> 
> I thought this was the rationale behind Russell's previous comments on
> Santosh's earlier patch version.

You are right, it's allocating more ptes. Than we can use pmd_addr_end.
I'll go back to the code.

-- 
Catalin

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Christoffer Dall]

On Fri, Feb 1, 2013 at 11:37 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Fri, Feb 01, 2013 at 04:32:54PM +0000, Christoffer Dall wrote:
>> On Fri, Feb 1, 2013 at 11:26 AM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > On Tue, Jan 29, 2013 at 03:07:16PM +0000, R Sricharan wrote:
>> >> With LPAE enabled, alloc_init_section() does not map the
>> >> entire address space for unaligned addresses.
>> >>
>> >> The issue also reproduced with CMA + LPAE. CMA tries to map 16MB
>> >> with page granularity mappings during boot. alloc_init_pte()
>> >> is called and out of 16MB, only 2MB gets mapped and rest remains
>> >> unaccessible.
>> >>
>> >> Because of this OMAP5 boot is broken with CMA + LPAE enabled.
>> >> Fix the issue by ensuring that the entire addresses are
>> >> mapped.
>> >>
>> >> Signed-off-by: R Sricharan <r.sricharan@ti.com>
>> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> >> Cc: Christoffer Dall <chris@cloudcar.com>
>> >> Cc: Russell King <linux@arm.linux.org.uk>
>> >> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> >> Tested-by: Christoffer Dall  <chris@cloudcar.com>
>> >> ---
>> >>  [V2] Moved the loop to alloc_init_pte as per Russell's
>> >>      feedback and changed the subject accordingly.
>> >>      Using PMD_XXX instead of SECTION_XXX to avoid
>> >>      different loop increments with/without LPAE.
>> >>
>> >>  [v3] Removed the dummy variable phys and updated
>> >>       the commit log for CMA case.
>> >>
>> >>  [v4] Resending with updated change log and
>> >>       updating the tags.
>> >>
>> >>  arch/arm/mm/mmu.c |   20 ++++++++++++++++----
>> >>  1 file changed, 16 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> >> index f8388ad..b94c313 100644
>> >> --- a/arch/arm/mm/mmu.c
>> >> +++ b/arch/arm/mm/mmu.c
>> >> @@ -569,11 +569,23 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
>> >>                                 unsigned long end, unsigned long pfn,
>> >>                                 const struct mem_type *type)
>> >>  {
>> >> -     pte_t *pte = early_pte_alloc(pmd, addr, type->prot_l1);
>> >> +     unsigned long next;
>> >> +     pte_t *pte;
>> >> +
>> >>       do {
>> >> -             set_pte_ext(pte, pfn_pte(pfn, __pgprot(type->prot_pte)), 0);
>> >> -             pfn++;
>> >> -     } while (pte++, addr += PAGE_SIZE, addr != end);
>> >> +             if ((end-addr) & PMD_MASK)
>> >> +                     next = (addr + PMD_SIZE) & PMD_MASK;
>> >> +             else
>> >> +                     next = end;
>> >
>> > Can use pmd_addr_end(addr, end) here?
>> >
>> >> +             pte = early_pte_alloc(pmd, addr, type->prot_l1);
>> >> +             do {
>> >> +                     set_pte_ext(pte, pfn_pte(pfn,
>> >> +                                     __pgprot(type->prot_pte)), 0);
>> >> +                     pfn++;
>> >> +             } while (pte++, addr += PAGE_SIZE, addr != next);
>> >> +
>> >> +     } while (pmd++, addr = next, addr != end);
>> >
>> > I would actually keep the loop in alloc_init_section(). There is even a
>> > comment in there saying "no need to loop" but you actually moved the
>> > loop in alloc_init_pte().
>> >
>> > I'm proposing a simpler patch below (only lightly tested on VE/C-A9).
>> > The only difference is that we do more flush_pmd_entry() calls but I'm
>> > not really bothered, it's during boot and you won't notice.
>> >
>> >
>> > diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
>> > index 9c82f98..eaa8ba8 100644
>> > --- a/arch/arm/include/asm/pgtable.h
>> > +++ b/arch/arm/include/asm/pgtable.h
>> > @@ -205,6 +205,11 @@ static inline pte_t *pmd_page_vaddr(pmd_t pmd)
>> >
>> >  #define pte_present_user(pte)  (pte_present(pte) && (pte_val(pte) & L_PTE_USER))
>> >
>> > +#define section_addr_end(addr, end)                                            \
>> > +({     unsigned long __boundary = ((addr) + SECTION_SIZE) & SECTION_MASK;      \
>> > +       (__boundary - 1 < (end) - 1)? __boundary: (end);                        \
>> > +})
>> > +
>> >  #if __LINUX_ARM_ARCH__ < 6
>> >  static inline void __sync_icache_dcache(pte_t pteval)
>> >  {
>> > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> > index 9f06102..0d0faed 100644
>> > --- a/arch/arm/mm/mmu.c
>> > +++ b/arch/arm/mm/mmu.c
>> > @@ -581,34 +581,19 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
>> >                                       const struct mem_type *type)
>> >  {
>> >         pmd_t *pmd = pmd_offset(pud, addr);
>> > +       unsigned long next;
>> >
>> > -       /*
>> > -        * Try a section mapping - end, addr and phys must all be aligned
>> > -        * to a section boundary.  Note that PMDs refer to the individual
>> > -        * L1 entries, whereas PGDs refer to a group of L1 entries making
>> > -        * up one logical pointer to an L2 table.
>> > -        */
>> > -       if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
>> > -               pmd_t *p = pmd;
>> > -
>> > -#ifndef CONFIG_ARM_LPAE
>> > -               if (addr & SECTION_SIZE)
>> > -                       pmd++;
>> > -#endif
>> > -
>> > -               do {
>> > +       do {
>> > +               next = section_addr_end(addr, end);
>> > +               /* try section mapping first */
>> > +               if (((addr | next | phys) & ~SECTION_MASK) == 0) {
>> >                         *pmd = __pmd(phys | type->prot_sect);
>> > -                       phys += SECTION_SIZE;
>> > -               } while (pmd++, addr += SECTION_SIZE, addr != end);
>> > -
>> > -               flush_pmd_entry(p);
>> > -       } else {
>> > -               /*
>> > -                * No need to loop; pte's aren't interested in the
>> > -                * individual L1 entries.
>> > -                */
>> > -               alloc_init_pte(pmd, addr, end, __phys_to_pfn(phys), type);
>> > -       }
>> > +                       flush_pmd_entry(pmd);
>> > +               } else {
>> > +                       alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), type);
>>
>> aren't you wasting memory here? The pte doesn't alloc a full page, but
>> the memblock allocator allocates a full page right?
>>
>> I thought this was the rationale behind Russell's previous comments on
>> Santosh's earlier patch version.
>
> You are right, it's allocating more ptes. Than we can use pmd_addr_end.
> I'll go back to the code.
>
don't get me wrong, I strongly prefer keeping that loop in
alloc_init_section, the other way it was weird to read, felt like
shoehorning something, but you may have to keep a pointer around for
unused part of a page for pte or something, not sure what ends up
being nicest.

-Christoffer

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Catalin Marinas]

On Fri, Feb 01, 2013 at 04:40:35PM +0000, Christoffer Dall wrote:
> On Fri, Feb 1, 2013 at 11:37 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Fri, Feb 01, 2013 at 04:32:54PM +0000, Christoffer Dall wrote:
> >> On Fri, Feb 1, 2013 at 11:26 AM, Catalin Marinas
> >> > --- a/arch/arm/mm/mmu.c
> >> > +++ b/arch/arm/mm/mmu.c
> >> > @@ -581,34 +581,19 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
> >> >                                       const struct mem_type *type)
> >> >  {
> >> >         pmd_t *pmd = pmd_offset(pud, addr);
> >> > +       unsigned long next;
> >> >
> >> > -       /*
> >> > -        * Try a section mapping - end, addr and phys must all be aligned
> >> > -        * to a section boundary.  Note that PMDs refer to the individual
> >> > -        * L1 entries, whereas PGDs refer to a group of L1 entries making
> >> > -        * up one logical pointer to an L2 table.
> >> > -        */
> >> > -       if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
> >> > -               pmd_t *p = pmd;
> >> > -
> >> > -#ifndef CONFIG_ARM_LPAE
> >> > -               if (addr & SECTION_SIZE)
> >> > -                       pmd++;
> >> > -#endif
> >> > -
> >> > -               do {
> >> > +       do {
> >> > +               next = section_addr_end(addr, end);
> >> > +               /* try section mapping first */
> >> > +               if (((addr | next | phys) & ~SECTION_MASK) == 0) {
> >> >                         *pmd = __pmd(phys | type->prot_sect);
> >> > -                       phys += SECTION_SIZE;
> >> > -               } while (pmd++, addr += SECTION_SIZE, addr != end);
> >> > -
> >> > -               flush_pmd_entry(p);
> >> > -       } else {
> >> > -               /*
> >> > -                * No need to loop; pte's aren't interested in the
> >> > -                * individual L1 entries.
> >> > -                */
> >> > -               alloc_init_pte(pmd, addr, end, __phys_to_pfn(phys), type);
> >> > -       }
> >> > +                       flush_pmd_entry(pmd);
> >> > +               } else {
> >> > +                       alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), type);
> >>
> >> aren't you wasting memory here? The pte doesn't alloc a full page, but
> >> the memblock allocator allocates a full page right?
> >>
> >> I thought this was the rationale behind Russell's previous comments on
> >> Santosh's earlier patch version.
> >
> > You are right, it's allocating more ptes. Than we can use pmd_addr_end.
> > I'll go back to the code.
> >
> don't get me wrong, I strongly prefer keeping that loop in
> alloc_init_section, the other way it was weird to read, felt like
> shoehorning something, but you may have to keep a pointer around for
> unused part of a page for pte or something, not sure what ends up
> being nicest.

Another try. This time I kept the same logic as before but added a loop
on the outside (and indented the code). With classic MMU
pmd_addr_end(addr, end) always return end, so the logic doesn't change.
With LPAE we should get the standard looping over pmd entries. Again,
only tested on C-A9.


diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 9f06102..47154f3 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -581,34 +581,36 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
 				      const struct mem_type *type)
 {
 	pmd_t *pmd = pmd_offset(pud, addr);
+	unsigned long next;
 
-	/*
-	 * Try a section mapping - end, addr and phys must all be aligned
-	 * to a section boundary.  Note that PMDs refer to the individual
-	 * L1 entries, whereas PGDs refer to a group of L1 entries making
-	 * up one logical pointer to an L2 table.
-	 */
-	if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
-		pmd_t *p = pmd;
+	do {
+		next = pmd_addr_end(addr, end);
+
+		/*
+		 * Try a section mapping - next, addr and phys must all be
+		 * aligned to a section boundary.  Note that PMDs refer to the
+		 * individual L1 entries, whereas PGDs refer to a group of L1
+		 * entries making up one logical pointer to an L2 table.
+		 */
+		if (((addr | next | phys) & ~SECTION_MASK) == 0) {
+			pmd_t *p = pmd;
 
 #ifndef CONFIG_ARM_LPAE
-		if (addr & SECTION_SIZE)
-			pmd++;
+			if (addr & SECTION_SIZE)
+				pmd++;
 #endif
+			do {
+				*pmd = __pmd(phys | type->prot_sect);
+				phys += SECTION_SIZE;
+			} while (pmd++, addr += SECTION_SIZE, addr != next);
 
-		do {
-			*pmd = __pmd(phys | type->prot_sect);
-			phys += SECTION_SIZE;
-		} while (pmd++, addr += SECTION_SIZE, addr != end);
-
-		flush_pmd_entry(p);
-	} else {
-		/*
-		 * No need to loop; pte's aren't interested in the
-		 * individual L1 entries.
-		 */
-		alloc_init_pte(pmd, addr, end, __phys_to_pfn(phys), type);
-	}
+			flush_pmd_entry(p);
+		} else {
+			alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), type);
+			phys += next - addr;
+			pmd++;
+		}
+	} while (addr = next, addr != end);
 }
 
 static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Christoffer Dall]

[...]

>
> Another try. This time I kept the same logic as before but added a loop
> on the outside (and indented the code). With classic MMU
> pmd_addr_end(addr, end) always return end, so the logic doesn't change.
> With LPAE we should get the standard looping over pmd entries. Again,
> only tested on C-A9.
>

I think the logic should be explained in a comment in the code if we
take this approach - it's not logical to understand that you switch
between using the outer vs. inner loop depending on your config.

>
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 9f06102..47154f3 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -581,34 +581,36 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
>                                       const struct mem_type *type)
>  {
>         pmd_t *pmd = pmd_offset(pud, addr);
> +       unsigned long next;
>
> -       /*
> -        * Try a section mapping - end, addr and phys must all be aligned
> -        * to a section boundary.  Note that PMDs refer to the individual
> -        * L1 entries, whereas PGDs refer to a group of L1 entries making
> -        * up one logical pointer to an L2 table.
> -        */
> -       if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
> -               pmd_t *p = pmd;
> +       do {
> +               next = pmd_addr_end(addr, end);
> +
> +               /*
> +                * Try a section mapping - next, addr and phys must all be
> +                * aligned to a section boundary.  Note that PMDs refer to the
> +                * individual L1 entries, whereas PGDs refer to a group of L1
> +                * entries making up one logical pointer to an L2 table.
> +                */
> +               if (((addr | next | phys) & ~SECTION_MASK) == 0) {
> +                       pmd_t *p = pmd;
>
>  #ifndef CONFIG_ARM_LPAE
> -               if (addr & SECTION_SIZE)
> -                       pmd++;
> +                       if (addr & SECTION_SIZE)
> +                               pmd++;
>  #endif

perhaps we could comment this ifdef now when we're at it, I spent a
considerable amount of time understanding it, and now I forgot again.
IIRC the logic relies on state set up during early assembly boot,
which is a little much to ask of the reader of this code...

> +                       do {
> +                               *pmd = __pmd(phys | type->prot_sect);
> +                               phys += SECTION_SIZE;
> +                       } while (pmd++, addr += SECTION_SIZE, addr != next);
>
> -               do {
> -                       *pmd = __pmd(phys | type->prot_sect);
> -                       phys += SECTION_SIZE;
> -               } while (pmd++, addr += SECTION_SIZE, addr != end);
> -
> -               flush_pmd_entry(p);
> -       } else {
> -               /*
> -                * No need to loop; pte's aren't interested in the
> -                * individual L1 entries.
> -                */
> -               alloc_init_pte(pmd, addr, end, __phys_to_pfn(phys), type);
> -       }
> +                       flush_pmd_entry(p);
> +               } else {
> +                       alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), type);
> +                       phys += next - addr;
> +                       pmd++;
> +               }
> +       } while (addr = next, addr != end);

aren't you still wasting memory for the individual PTEs inside alloc_init_pte?

also, I would prefer having two distinct loops than this outer/inner
scheme, but with the right comment I'm fine with it.

>  }
>
>  static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,

Let me know if you'd like me to test it on omap5.

-Christoffer

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[R Sricharan]

Hi Catalin,

[ snip..]
>
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 9f06102..47154f3 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -581,34 +581,36 @@ static void __init alloc_init_section(pud_t *pud, unsigned long addr,
>   				      const struct mem_type *type)
>   {
>   	pmd_t *pmd = pmd_offset(pud, addr);
> +	unsigned long next;
>
> -	/*
> -	 * Try a section mapping - end, addr and phys must all be aligned
> -	 * to a section boundary.  Note that PMDs refer to the individual
> -	 * L1 entries, whereas PGDs refer to a group of L1 entries making
> -	 * up one logical pointer to an L2 table.
> -	 */
> -	if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
> -		pmd_t *p = pmd;
> +	do {
> +		next = pmd_addr_end(addr, end);
> +
> +		/*
> +		 * Try a section mapping - next, addr and phys must all be
> +		 * aligned to a section boundary.  Note that PMDs refer to the
> +		 * individual L1 entries, whereas PGDs refer to a group of L1
> +		 * entries making up one logical pointer to an L2 table.
> +		 */
> +		if (((addr | next | phys) & ~SECTION_MASK) == 0) {
> +			pmd_t *p = pmd;
>
    There is a  need to do page mappings even when all the addresses
    are aligned. This was added with CMA, which required the initial
    mappings to be set with 2 level tables.

>   #ifndef CONFIG_ARM_LPAE
> -		if (addr & SECTION_SIZE)
> -			pmd++;
> +			if (addr & SECTION_SIZE)
> +				pmd++;
>   #endif
> +			do {
> +				*pmd = __pmd(phys | type->prot_sect);
> +				phys += SECTION_SIZE;
> +			} while (pmd++, addr += SECTION_SIZE, addr != next);
>
> -		do {
> -			*pmd = __pmd(phys | type->prot_sect);
> -			phys += SECTION_SIZE;
> -		} while (pmd++, addr += SECTION_SIZE, addr != end);
> -
> -		flush_pmd_entry(p);
> -	} else {
> -		/*
> -		 * No need to loop; pte's aren't interested in the
> -		 * individual L1 entries.
> -		 */
> -		alloc_init_pte(pmd, addr, end, __phys_to_pfn(phys), type);
> -	}
> +			flush_pmd_entry(p);
> +		} else {
> +			alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), type);
> +			phys += next - addr;
> +			pmd++;
> +		}
> +	} while (addr = next, addr != end);
>   }
>
>   static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
>
  I did a similar kind of patch in my V1 [1].
  I should be using PMD_MASK instead of SECTION_MASK there, and
  updated it in the next version.

  [1] https://patchwork.kernel.org/patch/1272991/


Regards,
  Sricharan

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[R Sricharan]

On Monday 04 February 2013 10:10 AM, R Sricharan wrote:
> Hi Catalin,
>
> [ snip..]
>>
>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> index 9f06102..47154f3 100644
>> --- a/arch/arm/mm/mmu.c
>> +++ b/arch/arm/mm/mmu.c
>> @@ -581,34 +581,36 @@ static void __init alloc_init_section(pud_t
>> *pud, unsigned long addr,
>>                         const struct mem_type *type)
>>   {
>>       pmd_t *pmd = pmd_offset(pud, addr);
>> +    unsigned long next;
>>
>> -    /*
>> -     * Try a section mapping - end, addr and phys must all be aligned
>> -     * to a section boundary.  Note that PMDs refer to the individual
>> -     * L1 entries, whereas PGDs refer to a group of L1 entries making
>> -     * up one logical pointer to an L2 table.
>> -     */
>> -    if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
>> -        pmd_t *p = pmd;
>> +    do {
>> +        next = pmd_addr_end(addr, end);
>> +
>> +        /*
>> +         * Try a section mapping - next, addr and phys must all be
>> +         * aligned to a section boundary.  Note that PMDs refer to the
>> +         * individual L1 entries, whereas PGDs refer to a group of L1
>> +         * entries making up one logical pointer to an L2 table.
>> +         */
>> +        if (((addr | next | phys) & ~SECTION_MASK) == 0) {
>> +            pmd_t *p = pmd;
>>
>     There is a  need to do page mappings even when all the addresses
>     are aligned. This was added with CMA, which required the initial
>     mappings to be set with 2 level tables.
>
    Sorry, i wanted to ask type->prot_sect is removed here and is that
   intentional?
>>   #ifndef CONFIG_ARM_LPAE
>> -        if (addr & SECTION_SIZE)
>> -            pmd++;
>> +            if (addr & SECTION_SIZE)
>> +                pmd++;
>>   #endif
>> +            do {
>> +                *pmd = __pmd(phys | type->prot_sect);
>> +                phys += SECTION_SIZE;
>> +            } while (pmd++, addr += SECTION_SIZE, addr != next);
>>
>> -        do {
>> -            *pmd = __pmd(phys | type->prot_sect);
>> -            phys += SECTION_SIZE;
>> -        } while (pmd++, addr += SECTION_SIZE, addr != end);
>> -
>> -        flush_pmd_entry(p);
>> -    } else {
>> -        /*
>> -         * No need to loop; pte's aren't interested in the
>> -         * individual L1 entries.
>> -         */
>> -        alloc_init_pte(pmd, addr, end, __phys_to_pfn(phys), type);
>> -    }
>> +            flush_pmd_entry(p);
>> +        } else {
>> +            alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys), type);
>> +            phys += next - addr;
>> +            pmd++;
>> +        }
>> +    } while (addr = next, addr != end);
>>   }
>>
>>   static void __init alloc_init_pud(pgd_t *pgd, unsigned long addr,
>>
   I did a similar kind of patch in my V1 [1].
    I should be using PMD_MASK instead of SECTION_MASK there, and
   updated it in the next version.

    [1] https://patchwork.kernel.org/patch/1272991/


Regards,
   Sricharan

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[R Sricharan]

Hi,

On Monday 04 February 2013 10:14 AM, R Sricharan wrote:
> On Monday 04 February 2013 10:10 AM, R Sricharan wrote:
>> Hi Catalin,
>>
>> [ snip..]
>>>
>>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>>> index 9f06102..47154f3 100644
>>> --- a/arch/arm/mm/mmu.c
>>> +++ b/arch/arm/mm/mmu.c
>>> @@ -581,34 +581,36 @@ static void __init alloc_init_section(pud_t
>>> *pud, unsigned long addr,
>>>                         const struct mem_type *type)
>>>   {
>>>       pmd_t *pmd = pmd_offset(pud, addr);
>>> +    unsigned long next;
>>>
>>> -    /*
>>> -     * Try a section mapping - end, addr and phys must all be aligned
>>> -     * to a section boundary.  Note that PMDs refer to the individual
>>> -     * L1 entries, whereas PGDs refer to a group of L1 entries making
>>> -     * up one logical pointer to an L2 table.
>>> -     */
>>> -    if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) ==
>>> 0) {
>>> -        pmd_t *p = pmd;
>>> +    do {
>>> +        next = pmd_addr_end(addr, end);
>>> +
>>> +        /*
>>> +         * Try a section mapping - next, addr and phys must all be
>>> +         * aligned to a section boundary.  Note that PMDs refer to the
>>> +         * individual L1 entries, whereas PGDs refer to a group of L1
>>> +         * entries making up one logical pointer to an L2 table.
>>> +         */
>>> +        if (((addr | next | phys) & ~SECTION_MASK) == 0) {
>>> +            pmd_t *p = pmd;
>>>
>>     There is a  need to do page mappings even when all the addresses
>>     are aligned. This was added with CMA, which required the initial
>>     mappings to be set with 2 level tables.
>>
>     Sorry, i wanted to ask type->prot_sect is removed here and is that
>    intentional?
       Tested this patch on OMAP5. The type->prot_sect check is
       required, without which mappings are not even created for CMA.
       (ie) type->prot_sect is not set when this function is called
       in CMA context, thus no valid mapping is created.

       After fixing this, the LPAE + CMA enabled combination booted
       fine on OMAP5.

Regards,
  Sricharan

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Catalin Marinas]

On Mon, Feb 04, 2013 at 04:44:19AM +0000, R Sricharan wrote:
> On Monday 04 February 2013 10:10 AM, R Sricharan wrote:
> >> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> >> index 9f06102..47154f3 100644
> >> --- a/arch/arm/mm/mmu.c
> >> +++ b/arch/arm/mm/mmu.c
> >> @@ -581,34 +581,36 @@ static void __init alloc_init_section(pud_t
> >> *pud, unsigned long addr,
> >>                         const struct mem_type *type)
> >>   {
> >>       pmd_t *pmd = pmd_offset(pud, addr);
> >> +    unsigned long next;
> >>
> >> -    /*
> >> -     * Try a section mapping - end, addr and phys must all be aligned
> >> -     * to a section boundary.  Note that PMDs refer to the individual
> >> -     * L1 entries, whereas PGDs refer to a group of L1 entries making
> >> -     * up one logical pointer to an L2 table.
> >> -     */
> >> -    if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
> >> -        pmd_t *p = pmd;
> >> +    do {
> >> +        next = pmd_addr_end(addr, end);
> >> +
> >> +        /*
> >> +         * Try a section mapping - next, addr and phys must all be
> >> +         * aligned to a section boundary.  Note that PMDs refer to the
> >> +         * individual L1 entries, whereas PGDs refer to a group of L1
> >> +         * entries making up one logical pointer to an L2 table.
> >> +         */
> >> +        if (((addr | next | phys) & ~SECTION_MASK) == 0) {
> >> +            pmd_t *p = pmd;
> >>
> >     There is a  need to do page mappings even when all the addresses
> >     are aligned. This was added with CMA, which required the initial
> >     mappings to be set with 2 level tables.
> >
>     Sorry, i wanted to ask type->prot_sect is removed here and is that
>    intentional?

No, I just forgot about it.

>    I did a similar kind of patch in my V1 [1].
>     I should be using PMD_MASK instead of SECTION_MASK there, and
>    updated it in the next version.
> 
>     [1] https://patchwork.kernel.org/patch/1272991/

With regards to your current patch, I really don't think looping over
pmd in alloc_init_pte() is the right fix. The alloc_init_pte() function
gets a pmd argument and it is supposed to make it point to a pte and
populate that pte rather than populate a number of pmds.

create_mapping() loops over pgds. alloc_init_pud() loops over puds
(well, we don't have any but we have the function for consistency).
alloc_init_section() should loop over pmds (we can even change the name
to alloc_init_pmd()).

Your original patch from August was better as it kept the looping
consistent but as you said, it should be using pmd_addr_end(). We can
use something simpler like alloc_init_pmd() on arm64 and instead of
set_pmd() there just call a separate map_init_section() which for
2-levels it sets both entries. This may address Russell's comment that
the resulting code was ugly.

The problem with the classic MMU is that if we have a 1MB range we can
end up with just page mappings. The code is currently buggy since if we
have create_mapping() for a 1MB range and later create_mapping() for a
4KB in the next MB, the first 1MB is removed.

-- 
Catalin

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Russell King - ARM Linux]

On Wed, Feb 06, 2013 at 12:15:23PM +0000, Catalin Marinas wrote:
> The problem with the classic MMU is that if we have a 1MB range we can
> end up with just page mappings. The code is currently buggy since if we
> have create_mapping() for a 1MB range and later create_mapping() for a
> 4KB in the next MB, the first 1MB is removed.

And that is where we rely on people calling create_mapping() - who
will be the SoC porters - not to be idiotic and do that kind of thing.
That's worked fine for the last 10+ years so there's no need to make
the code any more complex by trying to fix something that really isn't
a problem.

I wouldn't be surprised if we have some mappings which are created by
4K page tables, and then one half of the page table is overwritten by
a section mapping - so trying to fix this _may_ be risky.

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Catalin Marinas]

On Wed, Feb 06, 2013 at 12:22:13PM +0000, Russell King - ARM Linux wrote:
> On Wed, Feb 06, 2013 at 12:15:23PM +0000, Catalin Marinas wrote:
> > The problem with the classic MMU is that if we have a 1MB range we can
> > end up with just page mappings. The code is currently buggy since if we
> > have create_mapping() for a 1MB range and later create_mapping() for a
> > 4KB in the next MB, the first 1MB is removed.
> 
> And that is where we rely on people calling create_mapping() - who
> will be the SoC porters - not to be idiotic and do that kind of thing.
> That's worked fine for the last 10+ years so there's no need to make
> the code any more complex by trying to fix something that really isn't
> a problem.
> 
> I wouldn't be surprised if we have some mappings which are created by
> 4K page tables, and then one half of the page table is overwritten by
> a section mapping - so trying to fix this _may_ be risky.

If the 4K is called first, it should be fine. But some WARN_ON() calls
wouldn't hurt, just to be sure. It's not a fast path anyway.

-- 
Catalin

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[R Sricharan]

On Wednesday 06 February 2013 06:03 PM, Catalin Marinas wrote:
> On Wed, Feb 06, 2013 at 12:22:13PM +0000, Russell King - ARM Linux wrote:
>> On Wed, Feb 06, 2013 at 12:15:23PM +0000, Catalin Marinas wrote:
>>> The problem with the classic MMU is that if we have a 1MB range we can
>>> end up with just page mappings. The code is currently buggy since if we
>>> have create_mapping() for a 1MB range and later create_mapping() for a
>>> 4KB in the next MB, the first 1MB is removed.
>>
>> And that is where we rely on people calling create_mapping() - who
>> will be the SoC porters - not to be idiotic and do that kind of thing.
>> That's worked fine for the last 10+ years so there's no need to make
>> the code any more complex by trying to fix something that really isn't
>> a problem.
>>
>> I wouldn't be surprised if we have some mappings which are created by
>> 4K page tables, and then one half of the page table is overwritten by
>> a section mapping - so trying to fix this _may_ be risky.
>
> If the 4K is called first, it should be fine. But some WARN_ON() calls
> wouldn't hurt, just to be sure. It's not a fast path anyway.
>
Just to understand, wont we hit pmd_bad already in this case ?

Regards,
  Sricharan

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[R Sricharan]

On Wednesday 06 February 2013 05:45 PM, Catalin Marinas wrote:
> On Mon, Feb 04, 2013 at 04:44:19AM +0000, R Sricharan wrote:
>> On Monday 04 February 2013 10:10 AM, R Sricharan wrote:
>>>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>>>> index 9f06102..47154f3 100644
>>>> --- a/arch/arm/mm/mmu.c
>>>> +++ b/arch/arm/mm/mmu.c
>>>> @@ -581,34 +581,36 @@ static void __init alloc_init_section(pud_t
>>>> *pud, unsigned long addr,
>>>>                          const struct mem_type *type)
>>>>    {
>>>>        pmd_t *pmd = pmd_offset(pud, addr);
>>>> +    unsigned long next;
>>>>
>>>> -    /*
>>>> -     * Try a section mapping - end, addr and phys must all be aligned
>>>> -     * to a section boundary.  Note that PMDs refer to the individual
>>>> -     * L1 entries, whereas PGDs refer to a group of L1 entries making
>>>> -     * up one logical pointer to an L2 table.
>>>> -     */
>>>> -    if (type->prot_sect && ((addr | end | phys) & ~SECTION_MASK) == 0) {
>>>> -        pmd_t *p = pmd;
>>>> +    do {
>>>> +        next = pmd_addr_end(addr, end);
>>>> +
>>>> +        /*
>>>> +         * Try a section mapping - next, addr and phys must all be
>>>> +         * aligned to a section boundary.  Note that PMDs refer to the
>>>> +         * individual L1 entries, whereas PGDs refer to a group of L1
>>>> +         * entries making up one logical pointer to an L2 table.
>>>> +         */
>>>> +        if (((addr | next | phys) & ~SECTION_MASK) == 0) {
>>>> +            pmd_t *p = pmd;
>>>>
>>>      There is a  need to do page mappings even when all the addresses
>>>      are aligned. This was added with CMA, which required the initial
>>>      mappings to be set with 2 level tables.
>>>
>>      Sorry, i wanted to ask type->prot_sect is removed here and is that
>>     intentional?
>
> No, I just forgot about it.
>
>>     I did a similar kind of patch in my V1 [1].
>>      I should be using PMD_MASK instead of SECTION_MASK there, and
>>     updated it in the next version.
>>
>>      [1] https://patchwork.kernel.org/patch/1272991/
>
> With regards to your current patch, I really don't think looping over
> pmd in alloc_init_pte() is the right fix. The alloc_init_pte() function
> gets a pmd argument and it is supposed to make it point to a pte and
> populate that pte rather than populate a number of pmds.
>
> create_mapping() loops over pgds. alloc_init_pud() loops over puds
> (well, we don't have any but we have the function for consistency).
> alloc_init_section() should loop over pmds (we can even change the name
> to alloc_init_pmd()).
>
> Your original patch from August was better as it kept the looping
> consistent but as you said, it should be using pmd_addr_end(). We can
> use something simpler like alloc_init_pmd() on arm64 and instead of
> set_pmd() there just call a separate map_init_section() which for
> 2-levels it sets both entries. This may address Russell's comment that
> the resulting code was ugly.
  Thanks. So just to understand, you mean alloc_init_pmd loops over
  map_init_section. map_init_section populates either one pmd
  or calls alloc_init_pte. correct ? . I can send a v5 for this.
>
> The problem with the classic MMU is that if we have a 1MB range we can
> end up with just page mappings. The code is currently buggy since if we
> have create_mapping() for a 1MB range and later create_mapping() for a
> 4KB in the next MB, the first 1MB is removed.
>
  Ok, so in that case we will have a BUG_ON(pmd_bad) right?
  Earlier we had this hit when both static and page mappings
  were attempted in the same 2MB range and understood it was wrong.

Regards,
  Sricharan

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[Catalin Marinas]

On Wed, Feb 06, 2013 at 01:56:12PM +0000, R Sricharan wrote:
> On Wednesday 06 February 2013 05:45 PM, Catalin Marinas wrote:
> > On Mon, Feb 04, 2013 at 04:44:19AM +0000, R Sricharan wrote:
> >>     I did a similar kind of patch in my V1 [1].
> >>      I should be using PMD_MASK instead of SECTION_MASK there, and
> >>     updated it in the next version.
> >>
> >>      [1] https://patchwork.kernel.org/patch/1272991/
> >
> > With regards to your current patch, I really don't think looping over
> > pmd in alloc_init_pte() is the right fix. The alloc_init_pte() function
> > gets a pmd argument and it is supposed to make it point to a pte and
> > populate that pte rather than populate a number of pmds.
> >
> > create_mapping() loops over pgds. alloc_init_pud() loops over puds
> > (well, we don't have any but we have the function for consistency).
> > alloc_init_section() should loop over pmds (we can even change the name
> > to alloc_init_pmd()).
> >
> > Your original patch from August was better as it kept the looping
> > consistent but as you said, it should be using pmd_addr_end(). We can
> > use something simpler like alloc_init_pmd() on arm64 and instead of
> > set_pmd() there just call a separate map_init_section() which for
> > 2-levels it sets both entries. This may address Russell's comment that
> > the resulting code was ugly.
> 
>   Thanks. So just to understand, you mean alloc_init_pmd loops over
>   map_init_section. map_init_section populates either one pmd
>   or calls alloc_init_pte. correct ? . I can send a v5 for this.

alloc_init_pmd() loops over pmd (similar to alloc_init_pud). If
(type->prot_sect && ((addr | next | phys) & ~SECTION_MASK) == 0) you
call a map_init_section (whatever name you think is better) function
which contains the current section code from alloc_init_section().
Something like below (easier than explaining):

static void __init map_init_section(pmd_t *pmd, unsigned long addr,
				    unsigned long end, phys_addr_t phys,
				    const struct mem_type *type)
{
#ifndef CONFIG_ARM_LPAE
	if (addr & SECTION_SIZE)
		pmd++;
#endif

	do {
		*pmd = __pmd(phys | type->prot_sect);
		phys += SECTION_SIZE;
	} while (pmd++, addr += SECTION_SIZE, addr != end);

	flush_pmd_entry(p);
}

Pretty much avoiding the indentation level in alloc_init_section() with
multiple loops.

> > The problem with the classic MMU is that if we have a 1MB range we can
> > end up with just page mappings. The code is currently buggy since if we
> > have create_mapping() for a 1MB range and later create_mapping() for a
> > 4KB in the next MB, the first 1MB is removed.
>
>   Ok, so in that case we will have a BUG_ON(pmd_bad) right?

Yes, you are right, the BUG_ON(pmd_bad) should catch this. The other
scenario (4K mapping followed by 1M) wouldn't be but it's relatively
safe as we don't override any entry.

-- 
Catalin

[PATCH v4] ARM: LPAE: Fix mapping in alloc_init_pte for unaligned addresses

[R Sricharan]

On Wednesday 06 February 2013 08:46 PM, Catalin Marinas wrote:
> On Wed, Feb 06, 2013 at 01:56:12PM +0000, R Sricharan wrote:
>> On Wednesday 06 February 2013 05:45 PM, Catalin Marinas wrote:
>>> On Mon, Feb 04, 2013 at 04:44:19AM +0000, R Sricharan wrote:
>>>>      I did a similar kind of patch in my V1 [1].
>>>>       I should be using PMD_MASK instead of SECTION_MASK there, and
>>>>      updated it in the next version.
>>>>
>>>>       [1] https://patchwork.kernel.org/patch/1272991/
>>>
>>> With regards to your current patch, I really don't think looping over
>>> pmd in alloc_init_pte() is the right fix. The alloc_init_pte() function
>>> gets a pmd argument and it is supposed to make it point to a pte and
>>> populate that pte rather than populate a number of pmds.
>>>
>>> create_mapping() loops over pgds. alloc_init_pud() loops over puds
>>> (well, we don't have any but we have the function for consistency).
>>> alloc_init_section() should loop over pmds (we can even change the name
>>> to alloc_init_pmd()).
>>>
>>> Your original patch from August was better as it kept the looping
>>> consistent but as you said, it should be using pmd_addr_end(). We can
>>> use something simpler like alloc_init_pmd() on arm64 and instead of
>>> set_pmd() there just call a separate map_init_section() which for
>>> 2-levels it sets both entries. This may address Russell's comment that
>>> the resulting code was ugly.
>>
>>    Thanks. So just to understand, you mean alloc_init_pmd loops over
>>    map_init_section. map_init_section populates either one pmd
>>    or calls alloc_init_pte. correct ? . I can send a v5 for this.
>
> alloc_init_pmd() loops over pmd (similar to alloc_init_pud). If
> (type->prot_sect && ((addr | next | phys) & ~SECTION_MASK) == 0) you
> call a map_init_section (whatever name you think is better) function
> which contains the current section code from alloc_init_section().
> Something like below (easier than explaining):
>
> static void __init map_init_section(pmd_t *pmd, unsigned long addr,
> 				    unsigned long end, phys_addr_t phys,
> 				    const struct mem_type *type)
> {
> #ifndef CONFIG_ARM_LPAE
> 	if (addr & SECTION_SIZE)
> 		pmd++;
> #endif
>
> 	do {
> 		*pmd = __pmd(phys | type->prot_sect);
> 		phys += SECTION_SIZE;
> 	} while (pmd++, addr += SECTION_SIZE, addr != end);
>
> 	flush_pmd_entry(p);
> }
>
> Pretty much avoiding the indentation level in alloc_init_section() with
> multiple loops.
Thanks for explaining. will post V5 for this.

Regards,
  Sricharan