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