Re: [PATCH 3/4] efi: add central copy of grub_efi_find_mmap_size

[Leif Lindholm]

On Wed, Sep 13, 2017 at 06:39:11AM +0200, Daniel Kiper wrote:
> On Tue, Sep 05, 2017 at 09:41:13PM +0100, Leif Lindholm wrote:
> > There are several implementations of this function in the tree.
> > Add a central version in grub-core/efi/mm.c.
> >
> > Taken from grub-core/loader/i386/linux.c, changing some hard-coded constants
> > to use macros from efi/memory.h.
> 
> I am OK with the idea but...
> 
> > Signed-off-by: Leif Lindholm <address@hidden>
> > ---
> >  grub-core/kern/efi/mm.c | 47 
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/grub/efi/efi.h  |  1 +
> >  2 files changed, 48 insertions(+)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index ac2a4c556..8795aa1e0 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -218,6 +218,53 @@ grub_efi_finish_boot_services (grub_efi_uintn_t 
> > *outbuf_size, void *outbuf,
> >    return GRUB_ERR_NONE;
> >  }
> >
> > +/* To obtain the UEFI memory map, we must pass a buffer of sufficient size
> > +   to hold the entire map. This function returns a sane start value for
> > +   buffer size.  */
> > +grub_efi_uintn_t
> > +grub_efi_find_mmap_size (void)
> > +{
> > +  static grub_efi_uintn_t mmap_size = 0;
> > +
> > +  if (mmap_size != 0)
> > +    return mmap_size;
> 
> What will happen if memory will be fragmented further after this call
> and number of memory map entries increases above mmap_size?

Oh, that is completely broken, but there is no functional change
introduced by this patch.

I was just hoping there was some point at which I could stop fixing
the world in order to get to the point where upstream grub is able to
boot on arm64 systems with large amounts of memory, or holes in the
memory map.

A journey I commenced on February 28th this year:
http://lists.gnu.org/archive/html/grub-devel/2017-02/msg00156.html

I note that this patch was in itself triggered by your feedback on
"efi: add grub_efi_get_dram_base() function for arm*" on 27 July.

/
    Leif

> > +  mmap_size = 1 * GRUB_EFI_PAGE_SIZE;
> > +  while (1)
> > +    {
> > +      int ret;
> > +      grub_efi_memory_descriptor_t *mmap;
> > +      grub_efi_uintn_t desc_size;
> > +      grub_efi_uintn_t cur_mmap_size = mmap_size;
> > +
> > +      mmap = grub_malloc (cur_mmap_size);
> > +      if (! mmap)
> > +   return 0;
> > +
> > +      ret = grub_efi_get_memory_map (&cur_mmap_size, mmap, 0, &desc_size, 
> > 0);
> > +      grub_free (mmap);
> > +
> > +      if (ret < 0)
> > +   {
> > +     grub_error (GRUB_ERR_IO, "cannot get memory map");
> > +     return 0;
> > +   }
> > +      else if (ret > 0)
> > +   break;
> > +
> > +      if (mmap_size < cur_mmap_size)
> > +   mmap_size = cur_mmap_size;
> > +      mmap_size += GRUB_EFI_PAGE_SIZE;
> > +    }
> > +
> > +  /* Increase the size a bit for safety, because GRUB allocates more on
> > +     later, and EFI itself may allocate more.  */
> > +  mmap_size += 3 * GRUB_EFI_PAGE_SIZE;
> > +
> > +  mmap_size = ALIGN_UP (mmap_size, GRUB_EFI_PAGE_SIZE);
> 
> I prefer if you do something like that:
> 
> map_size = 0;
> 
> if (grub_efi_get_memory_map (&map_size, NULL, NULL, &desc_size, 0) < 0)
> {
>   grub_error (GRUB_ERR_IO, "cannot get EFI memory map size");
>   return 0;
> }
> 
> return ALIGN_UP (map_size + GRUB_EFI_PAGE_SIZE, GRUB_EFI_PAGE_SIZE);
> 
> Daniel