diff mbox series

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

Message ID 20170612145341.3351-2-leif.lindholm@linaro.org
State New
Headers show
Series efi: improved correctness, arm unification, and cleanup | expand

Commit Message

Leif Lindholm June 12, 2017, 2:53 p.m. UTC 
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 <leif.lindholm@linaro.org>

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

-- 
2.11.0


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

Comments

Daniel Kiper July 27, 2017, 2:27 p.m. UTC | #1 
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 <leif.lindholm@linaro.org>

> ---

>  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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm July 28, 2017, 5:38 p.m. UTC | #2 
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 <leif.lindholm@linaro.org>

> > ---

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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper Aug. 1, 2017, 11:41 a.m. UTC | #3 
On Fri, Jul 28, 2017 at 06:38:22PM +0100, Leif Lindholm wrote:
> 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 <leif.lindholm@linaro.org>

> > > ---

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


OK, let's leave it as is. Just change function name to grub_efi_get_ram_base().

Daniel

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

Patch

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)
+{
+  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);
+  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);
+    }
+
+  for (desc = memory_map, *base_addr = GRUB_UINT_MAX;
+       (grub_addr_t) desc < ((grub_addr_t) memory_map + mmap_size);
+       desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
+    {
+      if (desc->attribute & GRUB_EFI_MEMORY_WB)
+	if (desc->physical_start < *base_addr)
+	  *base_addr = desc->physical_start;
+    }
+
+  return GRUB_ERR_NONE;
+}
+#endif
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index e9c601f34..845fc2438 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -83,6 +83,7 @@  extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
 
 #if defined(__arm__) || defined(__aarch64__)
 void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
+grub_err_t EXPORT_FUNC(grub_efi_get_dram_base)(grub_addr_t *);
 #endif
 
 grub_addr_t grub_efi_modules_addr (void);