[PATCH] ARM: map_init_section flushes incorrect pmd

[PATCH] ARM: map_init_section flushes incorrect pmd

[Po-Yu Chuang]

This bug was introduced in commit e651eab0.
Some v4/v5 platforms failed to boot due to this.

Signed-off-by: Po-Yu Chuang <ratbert.chuang@gmail.com>
---
 arch/arm/mm/mmu.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index e0d8565..19a43f8 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -620,6 +620,8 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 			unsigned long end, phys_addr_t phys,
 			const struct mem_type *type)
 {
+	pmd_t *p = pmd;
+
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * In classic MMU format, puds and pmds are folded in to
@@ -638,7 +640,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 		phys += SECTION_SIZE;
 	} while (pmd++, addr += SECTION_SIZE, addr != end);
 
-	flush_pmd_entry(pmd);
+	flush_pmd_entry(p);
 }
 
 static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
-- 
1.7.9.5

[PATCH] ARM: map_init_section flushes incorrect pmd

[Will Deacon]

On Tue, May 28, 2013 at 11:48:20AM +0100, Po-Yu Chuang wrote:
> This bug was introduced in commit e651eab0.
> Some v4/v5 platforms failed to boot due to this.
> 
> Signed-off-by: Po-Yu Chuang <ratbert.chuang@gmail.com>
> ---
>  arch/arm/mm/mmu.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index e0d8565..19a43f8 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -620,6 +620,8 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
>  			unsigned long end, phys_addr_t phys,
>  			const struct mem_type *type)
>  {
> +	pmd_t *p = pmd;
> +
>  #ifndef CONFIG_ARM_LPAE
>  	/*
>  	 * In classic MMU format, puds and pmds are folded in to
> @@ -638,7 +640,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
>  		phys += SECTION_SIZE;
>  	} while (pmd++, addr += SECTION_SIZE, addr != end);
>  
> -	flush_pmd_entry(pmd);
> +	flush_pmd_entry(p);

Wait, shouldn't this flush be *inside* the loop anyway? Otherwise we just
flush the cacheline containing the first pmd. The flushing code could also
flush to PoU instead of PoC for UP ARMv7, but that's an unrelated optimisation.

Will

[PATCH] ARM: map_init_section flushes incorrect pmd

[Sricharan R]

On Tuesday 28 May 2013 06:35 PM, Will Deacon wrote:
> On Tue, May 28, 2013 at 11:48:20AM +0100, Po-Yu Chuang wrote:
>> This bug was introduced in commit e651eab0.
>> Some v4/v5 platforms failed to boot due to this.
>>
>> Signed-off-by: Po-Yu Chuang <ratbert.chuang@gmail.com>
>> ---
>>  arch/arm/mm/mmu.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> index e0d8565..19a43f8 100644
>> --- a/arch/arm/mm/mmu.c
>> +++ b/arch/arm/mm/mmu.c
>> @@ -620,6 +620,8 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
>>  			unsigned long end, phys_addr_t phys,
>>  			const struct mem_type *type)
>>  {
>> +	pmd_t *p = pmd;
>> +
>>  #ifndef CONFIG_ARM_LPAE
>>  	/*
>>  	 * In classic MMU format, puds and pmds are folded in to
>> @@ -638,7 +640,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
>>  		phys += SECTION_SIZE;
>>  	} while (pmd++, addr += SECTION_SIZE, addr != end);
>>  
>> -	flush_pmd_entry(pmd);
>> +	flush_pmd_entry(p);
> Wait, shouldn't this flush be *inside* the loop anyway? Otherwise we just
> flush the cacheline containing the first pmd. The flushing code could also
> flush to PoU instead of PoC for UP ARMv7, but that's an unrelated optimisation.
   I think in LPAE this loop iterates once and non LPAE twice.
   So both the entries should be contained in same cache line right ?

Regards,
 Sricharan

[PATCH] ARM: map_init_section flushes incorrect pmd

[Will Deacon]

On Tue, May 28, 2013 at 03:03:36PM +0100, Sricharan R wrote:
> On Tuesday 28 May 2013 06:35 PM, Will Deacon wrote:
> > On Tue, May 28, 2013 at 11:48:20AM +0100, Po-Yu Chuang wrote:
> >> This bug was introduced in commit e651eab0.
> >> Some v4/v5 platforms failed to boot due to this.
> >>
> >> Signed-off-by: Po-Yu Chuang <ratbert.chuang@gmail.com>
> >> ---
> >>  arch/arm/mm/mmu.c |    4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> >> index e0d8565..19a43f8 100644
> >> --- a/arch/arm/mm/mmu.c
> >> +++ b/arch/arm/mm/mmu.c
> >> @@ -620,6 +620,8 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
> >>  			unsigned long end, phys_addr_t phys,
> >>  			const struct mem_type *type)
> >>  {
> >> +	pmd_t *p = pmd;
> >> +
> >>  #ifndef CONFIG_ARM_LPAE
> >>  	/*
> >>  	 * In classic MMU format, puds and pmds are folded in to
> >> @@ -638,7 +640,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
> >>  		phys += SECTION_SIZE;
> >>  	} while (pmd++, addr += SECTION_SIZE, addr != end);
> >>  
> >> -	flush_pmd_entry(pmd);
> >> +	flush_pmd_entry(p);
> > Wait, shouldn't this flush be *inside* the loop anyway? Otherwise we just
> > flush the cacheline containing the first pmd. The flushing code could also
> > flush to PoU instead of PoC for UP ARMv7, but that's an unrelated optimisation.
>    I think in LPAE this loop iterates once and non LPAE twice.
>    So both the entries should be contained in same cache line right ?

Dunno, are there any guarantees about alignment of the starting pmd? Even
so, the function takes the range as parameters, so I don't think we
should tailor it to the caller. It may explain why this hasn't come up
sooner though.

Will

[PATCH] ARM: map_init_section flushes incorrect pmd

[Sricharan R]

On Tuesday 28 May 2013 07:37 PM, Will Deacon wrote:
> On Tue, May 28, 2013 at 03:03:36PM +0100, Sricharan R wrote:
>> On Tuesday 28 May 2013 06:35 PM, Will Deacon wrote:
>>> On Tue, May 28, 2013 at 11:48:20AM +0100, Po-Yu Chuang wrote:
>>>> This bug was introduced in commit e651eab0.
>>>> Some v4/v5 platforms failed to boot due to this.
>>>>
>>>> Signed-off-by: Po-Yu Chuang <ratbert.chuang@gmail.com>
>>>> ---
>>>>  arch/arm/mm/mmu.c |    4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>>>> index e0d8565..19a43f8 100644
>>>> --- a/arch/arm/mm/mmu.c
>>>> +++ b/arch/arm/mm/mmu.c
>>>> @@ -620,6 +620,8 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
>>>>  			unsigned long end, phys_addr_t phys,
>>>>  			const struct mem_type *type)
>>>>  {
>>>> +	pmd_t *p = pmd;
>>>> +
>>>>  #ifndef CONFIG_ARM_LPAE
>>>>  	/*
>>>>  	 * In classic MMU format, puds and pmds are folded in to
>>>> @@ -638,7 +640,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
>>>>  		phys += SECTION_SIZE;
>>>>  	} while (pmd++, addr += SECTION_SIZE, addr != end);
>>>>  
>>>> -	flush_pmd_entry(pmd);
>>>> +	flush_pmd_entry(p);
>>> Wait, shouldn't this flush be *inside* the loop anyway? Otherwise we just
>>> flush the cacheline containing the first pmd. The flushing code could also
>>> flush to PoU instead of PoC for UP ARMv7, but that's an unrelated optimisation.
>>    I think in LPAE this loop iterates once and non LPAE twice.
>>    So both the entries should be contained in same cache line right ?
> Dunno, are there any guarantees about alignment of the starting pmd? Even
> so, the function takes the range as parameters, so I don't think we
> should tailor it to the caller. It may explain why this hasn't come up
> sooner though.
>
> Will
 
 This function is not exposed outside. And the ranges passed to this is going
to not more than 2 entries in any case. If we put the flush inside the loop,
then we will end up doing an extra flush for the same line. Regarding the
alignment, I think if the pgd base is aligned, then rest should be fine.
Will have to check this.

Regards,
 Sricharan

[PATCH] ARM: map_init_section flushes incorrect pmd

[Po-Yu Chuang]

On Wed, May 29, 2013 at 2:48 AM, Sricharan R <r.sricharan@ti.com> wrote:
>
> On Tuesday 28 May 2013 07:37 PM, Will Deacon wrote:
> > On Tue, May 28, 2013 at 03:03:36PM +0100, Sricharan R wrote:
> >> On Tuesday 28 May 2013 06:35 PM, Will Deacon wrote:
> >>> On Tue, May 28, 2013 at 11:48:20AM +0100, Po-Yu Chuang wrote:
> >>>> This bug was introduced in commit e651eab0.
> >>>> Some v4/v5 platforms failed to boot due to this.
> >>>>
> >>>> Signed-off-by: Po-Yu Chuang <ratbert.chuang@gmail.com>
> >>>> ---
> >>>>  arch/arm/mm/mmu.c |    4 +++-
> >>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> >>>> index e0d8565..19a43f8 100644
> >>>> --- a/arch/arm/mm/mmu.c
> >>>> +++ b/arch/arm/mm/mmu.c
> >>>> @@ -620,6 +620,8 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
> >>>>                    unsigned long end, phys_addr_t phys,
> >>>>                    const struct mem_type *type)
> >>>>  {
> >>>> +  pmd_t *p = pmd;
> >>>> +
> >>>>  #ifndef CONFIG_ARM_LPAE
> >>>>    /*
> >>>>     * In classic MMU format, puds and pmds are folded in to
> >>>> @@ -638,7 +640,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
> >>>>            phys += SECTION_SIZE;
> >>>>    } while (pmd++, addr += SECTION_SIZE, addr != end);
> >>>>
> >>>> -  flush_pmd_entry(pmd);
> >>>> +  flush_pmd_entry(p);
> >>> Wait, shouldn't this flush be *inside* the loop anyway? Otherwise we just
> >>> flush the cacheline containing the first pmd. The flushing code could also
> >>> flush to PoU instead of PoC for UP ARMv7, but that's an unrelated optimisation.
> >>    I think in LPAE this loop iterates once and non LPAE twice.
> >>    So both the entries should be contained in same cache line right ?
> > Dunno, are there any guarantees about alignment of the starting pmd? Even
> > so, the function takes the range as parameters, so I don't think we
> > should tailor it to the caller. It may explain why this hasn't come up
> > sooner though.
> >
> > Will
>
>  This function is not exposed outside. And the ranges passed to this is going
> to not more than 2 entries in any case. If we put the flush inside the loop,
> then we will end up doing an extra flush for the same line. Regarding the
> alignment, I think if the pgd base is aligned, then rest should be fine.
> Will have to check this.
>
> Regards,
>  Sricharan
>

Hi ,

Catalin and Sricharan,
Thanks for your explanation.

Will,
I guess nobody noticed this because the MMU of later v7 processors
fetches page table
from D-cache. It even doesn't need to clean pmd to PoU.

Regards,
Po-Yu

[PATCH] ARM: map_init_section flushes incorrect pmd

[Will Deacon]

On Wed, May 29, 2013 at 03:14:58AM +0100, Po-Yu Chuang wrote:
> Will,
> I guess nobody noticed this because the MMU of later v7 processors
> fetches page table
> from D-cache. It even doesn't need to clean pmd to PoU.

It does if it's UP. The walker is only guaranteed to read from L1 if you
have the multiprocessing extensions.

As for this function, looks like it's ok because it has precisely one
caller, so it might be worth prefixing it with some underscores to make it
clear that nobody else should be calling it!

Will

[PATCH] ARM: map_init_section flushes incorrect pmd

[Po-Yu Chuang]

Hi Will,

On Wed, May 29, 2013 at 4:54 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, May 29, 2013 at 03:14:58AM +0100, Po-Yu Chuang wrote:
>> Will,
>> I guess nobody noticed this because the MMU of later v7 processors
>> fetches page table
>> from D-cache. It even doesn't need to clean pmd to PoU.
>
> It does if it's UP. The walker is only guaranteed to read from L1 if you
> have the multiprocessing extensions.

Ya, I see.

>
> As for this function, looks like it's ok because it has precisely one
> caller, so it might be worth prefixing it with some underscores to make it
> clear that nobody else should be calling it!

I am fine with that.
Should I create a new patch?

Regards,
Po-Yu

[PATCH] ARM: map_init_section flushes incorrect pmd

[Po-Yu Chuang]

On Wed, May 29, 2013 at 5:34 PM, Po-Yu Chuang <ratbert.chuang@gmail.com> wrote:
> Hi Will,
>
> On Wed, May 29, 2013 at 4:54 PM, Will Deacon <will.deacon@arm.com> wrote:
>> On Wed, May 29, 2013 at 03:14:58AM +0100, Po-Yu Chuang wrote:
>>> Will,
>>> I guess nobody noticed this because the MMU of later v7 processors
>>> fetches page table
>>> from D-cache. It even doesn't need to clean pmd to PoU.
>>
>> It does if it's UP. The walker is only guaranteed to read from L1 if you
>> have the multiprocessing extensions.
>
> Ya, I see.
>>
>> As for this function, looks like it's ok because it has precisely one
>> caller, so it might be worth prefixing it with some underscores to make it
>> clear that nobody else should be calling it!
>
> I am fine with that.
> Should I create a new patch?

Hi,

Does anyone have more comment about this?

Regards,
Po-Yu

[PATCH] ARM: map_init_section flushes incorrect pmd

[Will Deacon]

On Thu, May 30, 2013 at 09:15:26AM +0100, Po-Yu Chuang wrote:
> On Wed, May 29, 2013 at 5:34 PM, Po-Yu Chuang <ratbert.chuang@gmail.com> wrote:
> > Hi Will,
> >
> > On Wed, May 29, 2013 at 4:54 PM, Will Deacon <will.deacon@arm.com> wrote:
> >> On Wed, May 29, 2013 at 03:14:58AM +0100, Po-Yu Chuang wrote:
> >>> Will,
> >>> I guess nobody noticed this because the MMU of later v7 processors
> >>> fetches page table
> >>> from D-cache. It even doesn't need to clean pmd to PoU.
> >>
> >> It does if it's UP. The walker is only guaranteed to read from L1 if you
> >> have the multiprocessing extensions.
> >
> > Ya, I see.
> >>
> >> As for this function, looks like it's ok because it has precisely one
> >> caller, so it might be worth prefixing it with some underscores to make it
> >> clear that nobody else should be calling it!
> >
> > I am fine with that.
> > Should I create a new patch?
> 
> Hi,
> 
> Does anyone have more comment about this?

Sorry, I was snowed under yesterday. If you fancy adding the underscores to
__map_init_section, that would help preserve what little remains of my
sanity.

Your existing patch looks technically correct.

Will

[PATCH] ARM: map_init_section flushes incorrect pmd

[Po-Yu Chuang]

On Thu, May 30, 2013 at 5:12 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Thu, May 30, 2013 at 09:15:26AM +0100, Po-Yu Chuang wrote:
>> On Wed, May 29, 2013 at 5:34 PM, Po-Yu Chuang <ratbert.chuang@gmail.com> wrote:
>> > Hi Will,
>> >
>> > On Wed, May 29, 2013 at 4:54 PM, Will Deacon <will.deacon@arm.com> wrote:
>> >> On Wed, May 29, 2013 at 03:14:58AM +0100, Po-Yu Chuang wrote:
>> >>> Will,
>> >>> I guess nobody noticed this because the MMU of later v7 processors
>> >>> fetches page table
>> >>> from D-cache. It even doesn't need to clean pmd to PoU.
>> >>
>> >> It does if it's UP. The walker is only guaranteed to read from L1 if you
>> >> have the multiprocessing extensions.
>> >
>> > Ya, I see.
>> >>
>> >> As for this function, looks like it's ok because it has precisely one
>> >> caller, so it might be worth prefixing it with some underscores to make it
>> >> clear that nobody else should be calling it!
>> >
>> > I am fine with that.
>> > Should I create a new patch?
>>
>> Hi,
>>
>> Does anyone have more comment about this?
Hi Will,

> Sorry, I was snowed under yesterday. If you fancy adding the underscores to
> __map_init_section, that would help preserve what little remains of my
> sanity.

Although I recognize each word above, I am not quite sure what do you mean. :-p
I assume that the sentences mean you prefer adding underscores, so I
will submit a new patch.

>
> Your existing patch looks technically correct.

Regards,
Po-Yu

[PATCH v2] ARM: map_init_section flushes incorrect pmd

[Po-Yu Chuang]

This bug was introduced in commit e651eab0.
Some v4/v5 platforms failed to boot due to this.

Signed-off-by: Po-Yu Chuang <ratbert.chuang@gmail.com>
---
v2:
prefix underscores to map_init_section to emphasize that it should never be
called by some random function.

 arch/arm/mm/mmu.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index e0d8565..4d409e6 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -616,10 +616,12 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 }
 
-static void __init map_init_section(pmd_t *pmd, unsigned long addr,
+static void __init __map_init_section(pmd_t *pmd, unsigned long addr,
 			unsigned long end, phys_addr_t phys,
 			const struct mem_type *type)
 {
+	pmd_t *p = pmd;
+
 #ifndef CONFIG_ARM_LPAE
 	/*
 	 * In classic MMU format, puds and pmds are folded in to
@@ -638,7 +640,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
 		phys += SECTION_SIZE;
 	} while (pmd++, addr += SECTION_SIZE, addr != end);
 
-	flush_pmd_entry(pmd);
+	flush_pmd_entry(p);
 }
 
 static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
@@ -661,7 +663,7 @@ static void __init alloc_init_pmd(pud_t *pud, unsigned long addr,
 		 */
 		if (type->prot_sect &&
 				((addr | next | phys) & ~SECTION_MASK) == 0) {
-			map_init_section(pmd, addr, next, phys, type);
+			__map_init_section(pmd, addr, next, phys, type);
 		} else {
 			alloc_init_pte(pmd, addr, next,
 						__phys_to_pfn(phys), type);
-- 
1.7.9.5

[PATCH v2] ARM: map_init_section flushes incorrect pmd

[Jonas Jensen]

On 30 May 2013 13:46, Po-Yu Chuang <ratbert.chuang@gmail.com> wrote:
> This bug was introduced in commit e651eab0.
> Some v4/v5 platforms failed to boot due to this.

Thanks!

Confirming that this fixes boot on MOXA ART (FA526).

Best regards,
Jonas

[PATCH] ARM: map_init_section flushes incorrect pmd

[Catalin Marinas]

On Tue, May 28, 2013 at 02:05:02PM +0100, Will Deacon wrote:
> On Tue, May 28, 2013 at 11:48:20AM +0100, Po-Yu Chuang wrote:
> > This bug was introduced in commit e651eab0.
> > Some v4/v5 platforms failed to boot due to this.
> > 
> > Signed-off-by: Po-Yu Chuang <ratbert.chuang@gmail.com>
> > ---
> >  arch/arm/mm/mmu.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> > index e0d8565..19a43f8 100644
> > --- a/arch/arm/mm/mmu.c
> > +++ b/arch/arm/mm/mmu.c
> > @@ -620,6 +620,8 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
> >  			unsigned long end, phys_addr_t phys,
> >  			const struct mem_type *type)
> >  {
> > +	pmd_t *p = pmd;
> > +
> >  #ifndef CONFIG_ARM_LPAE
> >  	/*
> >  	 * In classic MMU format, puds and pmds are folded in to
> > @@ -638,7 +640,7 @@ static void __init map_init_section(pmd_t *pmd, unsigned long addr,
> >  		phys += SECTION_SIZE;
> >  	} while (pmd++, addr += SECTION_SIZE, addr != end);
> >  
> > -	flush_pmd_entry(pmd);
> > +	flush_pmd_entry(p);
> 
> Wait, shouldn't this flush be *inside* the loop anyway? Otherwise we just
> flush the cacheline containing the first pmd. The flushing code could also
> flush to PoU instead of PoC for UP ARMv7, but that's an unrelated optimisation.

With LPAE, map_init_section() is called once per section from
alloc_init_pmd(). The loop is there for classic MMU to allow setting two
pmd entries (maximum) and flush_pmd_entry() takes care of both (it
flushes a full cache line anyway).

-- 
Catalin