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

[Daniel Kiper]

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.

> +{
> +  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?
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.

> +  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.

Daniel