Re: [PATCH] efi/arm: fix allocation failure when reserving the kernel base

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 20 Aug 2019 at 10:49, Mike Rapoport <rppt@linux.ibm.com> wrote:
>
> On Mon, Aug 19, 2019 at 05:56:51PM +0300, Ard Biesheuvel wrote:
> > On Mon, 19 Aug 2019 at 11:01, Chester Lin <clin@suse.com> wrote:
> > >
> > > Hi Mike and Ard,
> > >
> > > On Thu, Aug 15, 2019 at 04:37:39PM +0300, Mike Rapoport wrote:
> > > > On Thu, Aug 15, 2019 at 02:32:50PM +0300, Ard Biesheuvel wrote:
> > > > > (adding Mike)
> > > > >
>
> ...
>
> > > > > > In this case the kernel failed to reserve cma, which should hit the issue of
> > > > > > memblock_limit=0x1000 as I had mentioned in my patch description. The first
> > > > > > block [0-0xfff] was scanned in adjust_lowmem_bounds(), but it did not align
> > > > > > with PMD_SIZE so the cma reservation failed because the memblock.current_limit
> > > > > > was extremely low. That's why I expand the first reservation from 1 PAGESIZE to
> > > > > > 1 PMD_SIZE in my patch in order to avoid this issue. Please kindly let me know
> > > > > > if any suggestion, thank you.
> > > >
> > > >
> > > > > This looks like it is a separate issue. The memblock/cma code should
> > > > > not choke on a reserved page of memory at 0x0.
> > > > >
> > > > > Perhaps Russell or Mike (cc'ed) have an idea how to address this?
> > > >
> > > > Presuming that the last memblock dump comes from the end of
> > > > arm_memblock_init() with the this memory map
> > > >
> > > > memory[0x0] [0x0000000000000000-0x0000000000000fff], 0x0000000000001000 bytes flags: 0x4
> > > > memory[0x1] [0x0000000000001000-0x0000000007ef5fff], 0x0000000007ef5000 bytes flags: 0x0
> > > > memory[0x2] [0x0000000007ef6000-0x0000000007f09fff], 0x0000000000014000 bytes flags: 0x4
> > > > memory[0x3] [0x0000000007f0a000-0x000000003cb3efff], 0x0000000034c35000 bytes flags: 0x0
> > > >
> > > > adjust_lowmem_bounds() will set the memblock_limit (and respectively global
> > > > memblock.current_limit) to 0x1000 and any further memblock_alloc*() will
> > > > happily fail.
> > > >
> > > > I believe that the assumption for memblock_limit calculations was that the
> > > > first bank has several megs at least.
> > > >
> > > > I wonder if this hack would help:
> > > >
> > > > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> > > > index d9a0038..948e5b9 100644
> > > > --- a/arch/arm/mm/mmu.c
> > > > +++ b/arch/arm/mm/mmu.c
> > > > @@ -1206,7 +1206,7 @@ void __init adjust_lowmem_bounds(void)
> > > >                        * allocated when mapping the start of bank 0, which
> > > >                        * occurs before any free memory is mapped.
> > > >                        */
> > > > -                     if (!memblock_limit) {
> > > > +                     if (memblock_limit < PMD_SIZE) {
> > > >                               if (!IS_ALIGNED(block_start, PMD_SIZE))
> > > >                                       memblock_limit = block_start;
> > > >                               else if (!IS_ALIGNED(block_end, PMD_SIZE))
> > > >
> > >
> > > I applied this patch as well and it works well on rpi-2 model B.
> > >
> >
> > Thanks, Chester, that is good to know.
> >
> > However, afaict, this only affects systems where physical memory
> > starts at address 0x0, so I think we need a better fix.
>
> This hack can be easily extended to handle systems with arbitrary start
> address, but it's still a hack...
>
> > I know Mike has been looking into the NOMAP stuff lately, and your
> > original patch contains a hunk that makes this code (?) disregard
> > nomap memblocks. That might be a better approach.
>
> I was actually looking how to replace NOMAP with something else to make
> memblock.memory consistent with actual physical memory banks. But this work
> is stashed for now.
>
> I'm not sure that skipping NOMAP regions would be good enough.
> If I understand corrrectly, with Chester's original patch the reservation
> of PMD aligned chunk of 32M for the kernel made the first conv-mem region
> PMD aligned and then memblock_limit will be set to the end of this region.
>
> Is there a reason for marking EFI_RESERVED_TYPE as NOMAP rather than simply
> reserve them with memblock_reserve()?
>

Yes.

On ARM systems, reserved memory regions should never be mapped by
default, since the cacheable mappings we use in the linear region may
conflict with the mapping attributes used by the firmware or driver
components that are using this memory.

In this particular case, we are talking about things like spin tables
and pens for secondaries that boot up with their caches disabled, and
having a cacheable mapping on the primary CPU might cause a loss of
coherency.