[RESENDING PATCH] powerpc/wii: properly disable use of BATs when requested.

[RESENDING PATCH] powerpc/wii: properly disable use of BATs when requested.

[Christophe Leroy]

'nobats' kernel parameter or some options like CONFIG_DEBUG_PAGEALLOC
deny the use of BATS for mapping memory.

This patch makes sure that the specific wii RAM mapping function
takes it into account as well.

Fixes: de32400dd26e ("wii: use both mem1 and mem2 as ram")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 Resending due to some servers not accepting 'umlaut' on Jonathan's family name. Sorry Jonathan.

<linux-kernel@vger.kernel.org>: host vger.kernel.org[209.132.180.67] said: 550
    5.7.1 Content-Policy reject msg: Message headers can not have 8-bit
    non-ASCII characters in it; Use MIME encodings if such are needed!  BF:<H
    1.16368e-10>; S1729557AbfAOQgb (in reply to end of DATA command)

<stable@vger.kernel.org>: host vger.kernel.org[209.132.180.67] said: 550 5.7.1
    Content-Policy reject msg: Message headers can not have 8-bit non-ASCII
    characters in it; Use MIME encodings if such are needed!  BF:<H
    1.16368e-10>; S1729557AbfAOQgb (in reply to end of DATA command)

 arch/powerpc/platforms/embedded6xx/wii.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/platforms/embedded6xx/wii.c b/arch/powerpc/platforms/embedded6xx/wii.c
index ecf703ee3a76..bae843e32ae7 100644
--- a/arch/powerpc/platforms/embedded6xx/wii.c
+++ b/arch/powerpc/platforms/embedded6xx/wii.c
@@ -82,6 +82,9 @@ unsigned long __init wii_mmu_mapin_mem2(unsigned long top)
 
 	/* MEM2 64MB@0x10000000 */
 	delta = wii_hole_start + wii_hole_size;
+	if (__map_without_bats)
+		return delta;
+
 	size = top - delta;
 	for (bl = 128<<10; bl < max_size; bl <<= 1) {
 		if (bl * 2 > size)
-- 
2.13.3

Re: [RESENDING PATCH] powerpc/wii: properly disable use of BATs when requested.

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

Hi again,

On Tue, Jan 15, 2019 at 04:43:20PM +0000, Christophe Leroy wrote:
> 'nobats' kernel parameter or some options like CONFIG_DEBUG_PAGEALLOC
> deny the use of BATS for mapping memory.
> 
> This patch makes sure that the specific wii RAM mapping function
> takes it into account as well.
> 
> Fixes: de32400dd26e ("wii: use both mem1 and mem2 as ram")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
[...]
>  	/* MEM2 64MB@0x10000000 */
>  	delta = wii_hole_start + wii_hole_size;
> +	if (__map_without_bats)
> +		return delta;
> +

Nothing is visibly broken without this patch, even with
CONFIG_DEBUG_PAGEALLOC (tested on top of v5.0-rc2), but the patch still
looks correct.

I'd prefer the 'if' block to be before the whole delta/size calculation,
to make the code slightly more readable because the delta and size
calculations stay in one visual block. It doesn't need to happen after
delta is calculated.

tentatively,
Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>


Thanks,
Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RESENDING PATCH] powerpc/wii: properly disable use of BATs when requested.

[Christophe Leroy]

Le 17/01/2019 à 02:05, Jonathan Neuschäfer a écrit :
> Hi again,
> 
> On Tue, Jan 15, 2019 at 04:43:20PM +0000, Christophe Leroy wrote:
>> 'nobats' kernel parameter or some options like CONFIG_DEBUG_PAGEALLOC
>> deny the use of BATS for mapping memory.
>>
>> This patch makes sure that the specific wii RAM mapping function
>> takes it into account as well.
>>
>> Fixes: de32400dd26e ("wii: use both mem1 and mem2 as ram")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
> [...]
>>   	/* MEM2 64MB@0x10000000 */
>>   	delta = wii_hole_start + wii_hole_size;
>> +	if (__map_without_bats)
>> +		return delta;
>> +
> 
> Nothing is visibly broken without this patch, even with
> CONFIG_DEBUG_PAGEALLOC (tested on top of v5.0-rc2), but the patch still
> looks correct.

Obviously, CONFIG_DEBUG_PAGEALLOC cannot work without this patch.
The purpose of CONFIG_DEBUG_PAGEALLOC is to unmap unused parts of memory 
so that any access to them will pagefault.

As this function inconditionnaly sets a BAT for the second block of RAM, 
any access to free area in the upper block will be granted without a fault.

I think you can test it by doing a kmalloc() then a kfree(), then try to 
read in that area (hopefully kmalloc() allocates memory from the top so 
it should go in the upper block). Maybe there is an LKDTM test for that.

> 
> I'd prefer the 'if' block to be before the whole delta/size calculation,
> to make the code slightly more readable because the delta and size
> calculations stay in one visual block. It doesn't need to happen after
> delta is calculated.

Euh ... the function has to return 'delta', so if I put the if block 
before the calculation of delta, it means we have to calculate delta twice:

	if (__map_without_bats)
		return wii_hole_start + wii_hole_size;

	delta = wii_hole_start + wii_hole_size;

My eyes don't really like it, so if we want to keep delta and size 
calculation together, the 'if' will go after calculation of size.

In anycase, this change is only really for fixing stable releases 
because this function will go away with my other serie.

Christophe

> 
> tentatively,
> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> 
> 
> Thanks,
> Jonathan
> 

Re: [RESENDING PATCH] powerpc/wii: properly disable use of BATs when requested.

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1987 bytes --]

On Thu, Jan 17, 2019 at 11:29:06AM +0100, Christophe Leroy wrote:
[...]
> > >   	/* MEM2 64MB@0x10000000 */
> > >   	delta = wii_hole_start + wii_hole_size;
> > > +	if (__map_without_bats)
> > > +		return delta;
> > > +
> > 
> > Nothing is visibly broken without this patch, even with
> > CONFIG_DEBUG_PAGEALLOC (tested on top of v5.0-rc2), but the patch still
> > looks correct.
> 
> Obviously, CONFIG_DEBUG_PAGEALLOC cannot work without this patch.
> The purpose of CONFIG_DEBUG_PAGEALLOC is to unmap unused parts of memory so
> that any access to them will pagefault.
> 
> As this function inconditionnaly sets a BAT for the second block of RAM, any
> access to free area in the upper block will be granted without a fault.
> 
> I think you can test it by doing a kmalloc() then a kfree(), then try to
> read in that area (hopefully kmalloc() allocates memory from the top so it
> should go in the upper block). Maybe there is an LKDTM test for that.

Ah, makes sense, thanks for the explanation.

> 
> > 
> > I'd prefer the 'if' block to be before the whole delta/size calculation,
> > to make the code slightly more readable because the delta and size
> > calculations stay in one visual block. It doesn't need to happen after
> > delta is calculated.
> 
> Euh ... the function has to return 'delta', so if I put the if block before
> the calculation of delta, it means we have to calculate delta twice:

Oh, sorry, I misread the code, but you're completely right (I shouldn't
answer mails while tired).

> 
> 	if (__map_without_bats)
> 		return wii_hole_start + wii_hole_size;
> 
> 	delta = wii_hole_start + wii_hole_size;
> 
> My eyes don't really like it, so if we want to keep delta and size
> calculation together, the 'if' will go after calculation of size.

I agree.

> In anycase, this change is only really for fixing stable releases because
> this function will go away with my other serie.

ACK


Thanks,
Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]