Re: [PATCH 04/12] mips: Reserve memory for the kernel image resources

[Serge Semin]

On Tue, Apr 30, 2019 at 10:58:33PM +0000, Paul Burton wrote:

Hello Paul

> Hi Serge,
> 
> On Fri, Apr 26, 2019 at 03:00:36AM +0300, Serge Semin wrote:
> > >  1) Older systems generally had something like an ISA bus which used
> > >     addresses below the kernel, and bootloaders like YAMON left behind
> > >     functions that could be called right at the start of RAM. This sort
> > >     of thing should be accounted for by /memreserve/ in DT or similar
> > >     platform-specific reservations though rather than generically, and
> > >     at least Malta & SEAD-3 DTs already have /memreserve/ entries for
> > >     it. So this part I think is OK. Some other older platforms might
> > >     need updating, but that's fine.
> > > 
> > 
> > Regarding ISA. As far as I remember devices on that bus can DMA only to the
> > lowest 16MB. So in case if kernel is too big or placed pretty much high,
> > they may be left even without reachable memory at all in current
> > implementation.
> 
> Sure - I'm not too worried about these old buses, platforms can continue
> to reserve the memory through DT or otherwise if they need to.
> 
> > >  2) trap_init() only allocates memory for the exception vector if using
> > >     a vectored interrupt mode. In other cases it just uses CAC_BASE
> > >     which currently gets reserved as part of this region between
> > >     PHYS_OFFSET & _text.
> > > 
> > >     I think this behavior is bogus, and we should instead:
> > > 
> > >     - Allocate the exception vector memory using memblock_alloc() for
> > >       CPUs implementing MIPSr2 or higher (ie. CPUs with a programmable
> > >       EBase register). If we're not using vectored interrupts then
> > >       allocating one page will do, and we already have the size
> > >       calculation for if we are.
> > > 
> > >     - Otherwise use CAC_BASE but call memblock_reserve() on the first
> > >       page.
> > > 
> > >     I think we should make that change before this one goes in. I can
> > >     try to get to it tomorrow, but feel free to beat me to it.
> > > 
> > 
> > As far as I understood you and the code this should be enough to fix
> > the problem:
> > diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
> > index 98ca55d62201..f680253e2617 100644
> > --- a/arch/mips/kernel/traps.c
> > +++ b/arch/mips/kernel/traps.c
> > @@ -2326,6 +2326,8 @@ void __init trap_init(void)
> >  				ebase += (read_c0_ebase() & 0x3ffff000);
> >  			}
> >  		}
> > +
> > +		memblock_reserve(ebase, PAGE_SIZE);
> >  	}
> >  
> >  	if (cpu_has_mmips) {
> > ---
> > 
> > Allocation has already been implemented in the if-branch under the
> > (cpu_has_veic || cpu_has_vint) condition. So we don't need to change
> > there anything.
> > In case if vectored interrupts aren't supported the else-clause is
> > taken and we need to reserve whatever is set in the exception base
> > address variable.
> > 
> > I'll add this patch between 3d and 4th ones if you are ok with it.
> 
> I think that would work, but I have other motivations to allocate the
> memory in non-vectored cases anyway. I just sent a series that does that
> & cleans up a little [1]. If you could take a look that would be great.
> With that change made I think this patch will be good to apply.
> 

Just reviewed and tested your series on my machine. I tagged the whole series
in a response to the cover-letter of [1].

Could you please proceed with this patchset review procedure? There are
also eight more patches left without your tag or comment.  This patch
is also left with no explicit tag.

BTW I see you already applied patches 1-3 to the mips-next, so what shall I
do when sending a v2 patchset with fixes asked to be provided for patch 12
and possibly for others in future? Shall I just resend the series without that
applied patches or send them over with your acked-by tagges?

-Sergey

> Thanks,
>     Paul
> 
> [1] https://lore.kernel.org/linux-mips/20190430225216.7164-1-paul.burton@xxxxxxxx/T/#t