Re: [PATCH 1/7] efi: add grub_efi_get_dram_base() function for arm*

[Leif Lindholm]

On Thu, Jul 27, 2017 at 04:27:26PM +0200, Daniel Kiper wrote:
> On Mon, Jun 12, 2017 at 03:53:35PM +0100, Leif Lindholm wrote:
> > Since ARM platforms do not have a common memory map, add a helper
> > function that finds the lowest address region with the EFI_MEMORY_WB
> > attribute set in the UEFI memory map.
> >
> > Required for the arm/arm64 linux loader to restrict the initrd
> > location to where it will be accessible by the kernel at runtime.
> >
> > Signed-off-by: Leif Lindholm <address@hidden>
> > ---
> >  grub-core/kern/efi/mm.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/grub/efi/efi.h  |  1 +
> >  2 files changed, 43 insertions(+)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index 20a47aaf5..460a4b763 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -525,3 +525,45 @@ grub_efi_mm_init (void)
> >    grub_efi_free_pages ((grub_addr_t) memory_map,
> >                    2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
> >  }
> > +
> > +#if defined (__arm__) || defined (__aarch64__)
> > +grub_err_t
> > +grub_efi_get_dram_base(grub_addr_t *base_addr)
> 
> Please make this function more generic and get
> attribute as an argument.

While I am normally a huge fan of making everything as generic as
possible, this really is a very special case - and as far as I know,
it is really only relevant to ARM/AArch64. I would expect other
architectures to just have a static inline, return 0 in efi/memory.h,
if we ended up merging even more Linux EFI stub loaders together.

Or are you looking for some more multidimensional memory map lookup
thing? If so, could you give me an example invocation to work
against?

> > +{
> > +  grub_efi_memory_descriptor_t *memory_map;
> > +  grub_efi_memory_descriptor_t *desc;
> > +  grub_efi_uintn_t mmap_size;
> > +  grub_efi_uintn_t desc_size;
> > +
> > +  mmap_size = (1 << GRUB_EFI_PAGE_SHIFT);
> 
> Could not you define GRUB_EFI_PAGE and use it here?

Sure.

> And why GRUB_EFI_PAGE_SHIFT and other stuff is defined
> in include/grub/arm64/fdtload.h? It looks like generic
> thing and should be in genric place too. Please fix it
> if possible.

Sure, I'll reorder that hunk (as you spotted in a later patch).

> > +  while (1)
> > +    {
> > +      int ret;
> > +
> > +      memory_map = grub_malloc (mmap_size);
> > +      if (! memory_map)
> > +        return GRUB_ERR_OUT_OF_MEMORY;
> > +      ret = grub_efi_get_memory_map (&mmap_size, memory_map, NULL,
> > +                                &desc_size, NULL);
> > +      if (ret > 0)
> > +   break;
> > +
> > +      grub_free (memory_map);
> > +      if (ret == 0)
> > +   return GRUB_ERR_BUG;
> > +
> > +      mmap_size += (1 << GRUB_EFI_PAGE_SHIFT);
> > +    }
> 
> Hmmm... This is not optimal. Please take a look at e.g.
> grub_efi_finish_boot_services() how buffer for memory
> map should be allocated.
> 
> And going further... Could you take a look at
> grub_relocator_alloc_chunk_align() and
> grub_relocator_alloc_chunk_addr(). Good example
> is in grub-core/loader/multiboot_mbi2.c and
> grub-core/loader/multiboot_elfxx.c. If you
> use this machinery then there is a chance
> that you do not duplicate code so much.

Yeah, I can move the relevant bits from there to kern/efi/mm.c to
start getting rid of the existing duplication.

/
    Leif