[v3,1/6] efi: add central copy of grub_efi_find_mmap_size

Message ID 20180627171720.27028-2-leif.lindholm@linaro.org
State Superseded
Headers show
Series
  • efi: arm linux loader unification and correctness
Related show

Commit Message

Leif Lindholm June 27, 2018, 5:17 p.m.
There are several implementations of this function in the tree.
Add a central version in grub-core/efi/mm.c.

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

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

-- 
2.11.0


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

Comments

Daniel Kiper July 6, 2018, 3 p.m. | #1
On Wed, Jun 27, 2018 at 06:17:15PM +0100, Leif Lindholm wrote:
> There are several implementations of this function in the tree.

> Add a central version in grub-core/efi/mm.c.


I am happy with the code itself but I am not sure why you are
not replacing these "several implementations of this function"
in the existing code. I would like to see a patch which does that.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm July 6, 2018, 4:21 p.m. | #2
On Fri, Jul 06, 2018 at 05:00:45PM +0200, Daniel Kiper wrote:
> On Wed, Jun 27, 2018 at 06:17:15PM +0100, Leif Lindholm wrote:

> > There are several implementations of this function in the tree.

> > Add a central version in grub-core/efi/mm.c.

> 

> I am happy with the code itself but I am not sure why you are

> not replacing these "several implementations of this function"

> in the existing code. I would like to see a patch which does that.


I am happy to submit further cleanup patches once this set gets in,
but this is a fundamental change in behaviour compared to those other
bits of code (which I do not exercise or use). It is also a
fundamental change in behaviour compared to the previous version
submitted. I feel a temporary duplication minimises risk of further
disruption.

Best Regards,

Leif

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper July 6, 2018, 4:55 p.m. | #3
On Fri, Jul 06, 2018 at 05:21:17PM +0100, Leif Lindholm wrote:
> On Fri, Jul 06, 2018 at 05:00:45PM +0200, Daniel Kiper wrote:

> > On Wed, Jun 27, 2018 at 06:17:15PM +0100, Leif Lindholm wrote:

> > > There are several implementations of this function in the tree.

> > > Add a central version in grub-core/efi/mm.c.

> >

> > I am happy with the code itself but I am not sure why you are

> > not replacing these "several implementations of this function"

> > in the existing code. I would like to see a patch which does that.

>

> I am happy to submit further cleanup patches once this set gets in,

> but this is a fundamental change in behaviour compared to those other

> bits of code (which I do not exercise or use). It is also a

> fundamental change in behaviour compared to the previous version

> submitted. I feel a temporary duplication minimises risk of further

> disruption.


OK, I will take this patch if you promise to fix this in separate patch
series later. Later means weeks not years. Of course I am happy to help
with this.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm July 6, 2018, 5:13 p.m. | #4
On Fri, Jul 06, 2018 at 06:55:51PM +0200, Daniel Kiper wrote:
> On Fri, Jul 06, 2018 at 05:21:17PM +0100, Leif Lindholm wrote:

> > On Fri, Jul 06, 2018 at 05:00:45PM +0200, Daniel Kiper wrote:

> > > On Wed, Jun 27, 2018 at 06:17:15PM +0100, Leif Lindholm wrote:

> > > > There are several implementations of this function in the tree.

> > > > Add a central version in grub-core/efi/mm.c.

> > >

> > > I am happy with the code itself but I am not sure why you are

> > > not replacing these "several implementations of this function"

> > > in the existing code. I would like to see a patch which does that.

> >

> > I am happy to submit further cleanup patches once this set gets in,

> > but this is a fundamental change in behaviour compared to those other

> > bits of code (which I do not exercise or use). It is also a

> > fundamental change in behaviour compared to the previous version

> > submitted. I feel a temporary duplication minimises risk of further

> > disruption.

> 

> OK, I will take this patch if you promise to fix this in separate patch

> series later. Later means weeks not years. Of course I am happy to help

> with this.


I promise to send an RFC out next week.
After that, I'll be basically offline for two weeks.

What I cannot promise is validating my patches outside of QEMU/Ovmf,
or figuring out special workarounds if it turns out this new mechanism
does not work on some (broken) UEFI implementations. If we come across
anything like that, then I would suggest we keep the duplication.

The RFC will remove all caching of the memory map, which always felt a
bit dubious to me.

/
    Leif

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper July 6, 2018, 6:26 p.m. | #5
On Fri, Jul 06, 2018 at 06:13:30PM +0100, Leif Lindholm wrote:
> On Fri, Jul 06, 2018 at 06:55:51PM +0200, Daniel Kiper wrote:

> > On Fri, Jul 06, 2018 at 05:21:17PM +0100, Leif Lindholm wrote:

> > > On Fri, Jul 06, 2018 at 05:00:45PM +0200, Daniel Kiper wrote:

> > > > On Wed, Jun 27, 2018 at 06:17:15PM +0100, Leif Lindholm wrote:

> > > > > There are several implementations of this function in the tree.

> > > > > Add a central version in grub-core/efi/mm.c.

> > > >

> > > > I am happy with the code itself but I am not sure why you are

> > > > not replacing these "several implementations of this function"

> > > > in the existing code. I would like to see a patch which does that.

> > >

> > > I am happy to submit further cleanup patches once this set gets in,

> > > but this is a fundamental change in behaviour compared to those other

> > > bits of code (which I do not exercise or use). It is also a

> > > fundamental change in behaviour compared to the previous version

> > > submitted. I feel a temporary duplication minimises risk of further

> > > disruption.

> >

> > OK, I will take this patch if you promise to fix this in separate patch

> > series later. Later means weeks not years. Of course I am happy to help

> > with this.

>

> I promise to send an RFC out next week.

> After that, I'll be basically offline for two weeks.

>

> What I cannot promise is validating my patches outside of QEMU/Ovmf,

> or figuring out special workarounds if it turns out this new mechanism

> does not work on some (broken) UEFI implementations. If we come across

> anything like that, then I would suggest we keep the duplication.

>

> The RFC will remove all caching of the memory map, which always felt a

> bit dubious to me.


That works for me. Thank you!

Have a nice weekend,

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 c48e9b5c7..fd39d23b4 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -286,6 +286,26 @@  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)
+{
+  grub_efi_uintn_t mmap_size = 0;
+  grub_efi_uintn_t desc_size;
+
+  if (grub_efi_get_memory_map (&mmap_size, NULL, NULL, &desc_size, 0) < 0)
+    {
+      grub_error (GRUB_ERR_IO, "cannot get EFI memory map size");
+      return 0;
+    }
+
+  /* Add an extra page, since UEFI can alter the memory map itself on
+     callbacks or explicit calls, including console output. */
+  return ALIGN_UP (mmap_size + GRUB_EFI_PAGE_SIZE, GRUB_EFI_PAGE_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 c996913e5..1021273c1 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,