[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 at 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 at ti.com>
>> Cc: Catalin Marinas <catalin.marinas at arm.com>
>> Cc: Christoffer Dall <chris at cloudcar.com>
>> Cc: Russell King <linux at arm.linux.org.uk>
>> Acked-by: Santosh Shilimkar <santosh.shilimkar at ti.com>
>> Tested-by: Christoffer Dall  <chris at 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,