Re: issue with MEMBLOCK_NOMAP

[Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Fri, 2016-01-29 at 17:16 +0100, Ard Biesheuvel wrote:
> On 29 January 2016 at 16:53, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 2016-01-29 at 15:06 +0100, Ard Biesheuvel wrote:
> > > On 29 January 2016 at 15:00, Mark Salter <msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > > > Hi Ard,
> > > > 
> > > > I ran into an issue with your MEMBLOCK_NOMAP changes on a particular
> > > > firmware. The symptom is the kernel panics at boot time when it hits
> > > > an unmapped page while unpacking the initramfs. As it turns out, the
> > > > start of the initramfs shares a 64k kernel page with the UEFI memmap.
> > > > I can avoid the problem with:
> > > > 
> > > > @@ -203,7 +203,7 @@ void __init efi_init(void)
> > > > 
> > > >         reserve_regions();
> > > >         early_memunmap(memmap.map, params.mmap_size);
> > > > -       memblock_mark_nomap(params.mmap & PAGE_MASK,
> > > > -                           PAGE_ALIGN(params.mmap_size +
> > > > -                                      (params.mmap & ~PAGE_MASK)));
> > > > +       memblock_reserve(params.mmap & PAGE_MASK,
> > > > +                        PAGE_ALIGN(params.mmap_size +
> > > > +                                   (params.mmap & ~PAGE_MASK)));
> > > >  }
> > > > 
> > > > 
> > > > But it makes me worry about the same potential problem with
> > > > other reserved regions which we nomap. What do you think?
> > > > 
> > > 
> > > So I take it this initramfs allocation is not made by the stub but by
> > > GRUB? Since the stub rounds all allocations to 64 KB ...
> > > 
> > Yes. GRUB.
> > 
> 
> We have already fixed EDK2 a while ago to round up all regions
> returned by AllocatePages() to round up to 64 KB. Do you know if this
> is a GRUB issue (i.e., it traverses the memory map and finds a free
> range and explicitly allocates it) or a firmware issue?

Grub uses AllocatePages() to get memory for the initrd. The firmware
that hit this was fairly old (released last May I think). The problem
didn't show up on newer firmware for same platform but that doesn't
really mean anything definitive.

> 
> > > In any case, regardless of the underlying cause, if any part of the
> > > initramfs turns out not to be covered by the linear mapping, we should
> > > invoke your code to move it. So I think it should be a matter of
> > > refining the logic in relocate_initrd() to do the right thing in this
> > > case
> > 
> > That thought had crossed my mind. I think it would be easy enough to
> > trigger the copy if first or last page of initrd is unmapped.
> 
> Indeed. If some page in the middle is missing, then you're really
> doing something fishy, so I don't see why we should care about that as
> well.
> 
> > Somewhat
> > related to this is that I want to rework this old patch to deal with
> > acpi tables outside mapped ram:
> > 
> >   https://lkml.org/lkml/2015/5/14/357
> > 
> > Basically, we should be able to just do:
> > 
> > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> > index 15e0aad..4ea638c 100644
> > --- a/arch/arm64/include/asm/acpi.h
> > +++ b/arch/arm64/include/asm/acpi.h
> > @@ -32,7 +32,7 @@
> >  static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
> >                                             acpi_size size)
> >  {
> > -       if (!page_is_ram(phys >> PAGE_SHIFT))
> > +       if (!memblock_is_memory(phys))
> >                 return ioremap(phys, size);
> > 
> >         return ioremap_cache(phys, size);
> > 
> 
> I think we should fix acpi_os_ioremap(). IIRC it is used via two
> different code paths that distinguish between memory and I/O, and end
> up using the same function for no good reason.

I remember this being mentioned before. It would be a nice solution.

> 
> > But this doesn't currently work wrt mem= which works by removing
> > the end range of memblocks. If I have mem= use the nomap flag
> > rather than removing the range, the above acpi_os_ioremap change
> > works, but other issues crop up due to memblock_end_of_DRAM()
> > returning end of all DRAM regardless of mem=. So we end up with
> > PFNs and struct pages for memory which will never be in linear
> > map. Fixing memblock_end_of_DRAM() to look at the flags and
> > return end of mapped DRAM gets things working but I wonder about
> > other potential trouble spots with this approach. Any thoughts?
> > 
> 
> Actually, I think mem= should be considered a development feature, not
> a production feature, and if its use is suboptimal in this respect, so
> be it.

It is mostly a devel/debug feature but the production case is
with kdump where the kexec'd kernel gathering the dump info has
to be restricted to its own sandbox.

> 
> But to address this particular issue, it would probably be better to
> fix page_is_ram(). I have made some attempts in that direction in the
> past, but that never landed anywhere. Since ACPI on arm64 is tightly
> coupled to UEFI, implementing page_is_ram() as something that
> interrogates the UEFI memory map if efi_enabled(EFI_MEMMAP) would be
> reasonable imo. (Or perhaps putting that in acpi_os_ioremap()
> directly?)
> 
> > 
> > > 
> > > Your suggested change will break 32-bit ARM, since we use
> > > ioremap_nocache() to map the UEFI memory map, and ARM does not allow
> > > that on ranges that are part of the linear mapping.
> > 
> > okay. I'll put together a patch to the initrd relocating code.
> > 
> 
> Great!