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

Message ID 20170905204114.9462-4-leif.lindholm@linaro.org
State New
Headers show
Series
  • x86,efi: prerequisites for ARM* cleanup series
Related show

Commit Message

Leif Lindholm Sept. 5, 2017, 8:41 p.m.
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.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

---
 grub-core/kern/efi/mm.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/grub/efi/efi.h  |  1 +
 2 files changed, 48 insertions(+)

-- 
2.11.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Comments

Daniel Kiper Sept. 13, 2017, 4:39 a.m. | #1
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 <leif.lindholm@linaro.org>

> ---

>  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?

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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm Sept. 13, 2017, 8:41 a.m. | #2
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 <leif.lindholm@linaro.org>

> > ---

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


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Patch

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;
+
+  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);
+  return mmap_size;
+}
+
 /* Get the memory map as defined in the EFI spec. Return 1 if successful,
    return 0 if partial, or return -1 if an error occurs.  */
 int
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 3fa082816..8f1f36c8c 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -49,6 +49,7 @@  void *
 EXPORT_FUNC(grub_efi_allocate_any_pages) (grub_efi_uintn_t pages);
 void EXPORT_FUNC(grub_efi_free_pages) (grub_efi_physical_address_t address,
 				       grub_efi_uintn_t pages);
+grub_efi_uintn_t EXPORT_FUNC(grub_efi_find_mmap_size) (void);
 int
 EXPORT_FUNC(grub_efi_get_memory_map) (grub_efi_uintn_t *memory_map_size,
 				      grub_efi_memory_descriptor_t *memory_map,