[2/7] efi: refactor grub_efi_allocate_pages

Message ID 20170612145341.3351-3-leif.lindholm@linaro.org
State Superseded
Headers show
Series
  • efi: improved correctness, arm unification, and cleanup
Related show

Commit Message

Leif Lindholm June 12, 2017, 2:53 p.m.
Expose a new function, grub_efi_allocate_pages_real(), making it possible
to specify allocation type and memory type as supported by the UEFI
AllocatePages boot service.

Make grub_efi_allocate_pages() a consumer of the new function,
maintaining its old functionality.

Also delete some left-around #if 1/#else blocks in the affected
functions.

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

---
 grub-core/kern/efi/mm.c | 46 ++++++++++++++++++++++++----------------------
 include/grub/efi/efi.h  |  5 +++++
 2 files changed, 29 insertions(+), 22 deletions(-)

-- 
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:46 p.m. | #1
On Mon, Jun 12, 2017 at 03:53:36PM +0100, Leif Lindholm wrote:
> Expose a new function, grub_efi_allocate_pages_real(), making it possible

> to specify allocation type and memory type as supported by the UEFI

> AllocatePages boot service.

>

> Make grub_efi_allocate_pages() a consumer of the new function,

> maintaining its old functionality.

>

> Also delete some left-around #if 1/#else blocks in the affected

> functions.

>

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

> ---

>  grub-core/kern/efi/mm.c | 46 ++++++++++++++++++++++++----------------------

>  include/grub/efi/efi.h  |  5 +++++

>  2 files changed, 29 insertions(+), 22 deletions(-)

>

> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c

> index 460a4b763..7b1763bc5 100644

> --- a/grub-core/kern/efi/mm.c

> +++ b/grub-core/kern/efi/mm.c

> @@ -51,36 +51,20 @@ int grub_efi_is_finished = 0;

>

>  /* Allocate pages. Return the pointer to the first of allocated pages.  */

>  void *

> -grub_efi_allocate_pages (grub_efi_physical_address_t address,

> -			 grub_efi_uintn_t pages)

> +grub_efi_allocate_pages_real (grub_efi_physical_address_t address,


Could not you just add an extra argument to grub_efi_allocate_pages()?
It is not called in many places. Do you really need separate
grub_efi_allocate_pages_real()?

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Leif Lindholm July 28, 2017, 5:57 p.m. | #2
On Thu, Jul 27, 2017 at 04:46:14PM +0200, Daniel Kiper wrote:
> On Mon, Jun 12, 2017 at 03:53:36PM +0100, Leif Lindholm wrote:

> > Expose a new function, grub_efi_allocate_pages_real(), making it possible

> > to specify allocation type and memory type as supported by the UEFI

> > AllocatePages boot service.

> >

> > Make grub_efi_allocate_pages() a consumer of the new function,

> > maintaining its old functionality.

> >

> > Also delete some left-around #if 1/#else blocks in the affected

> > functions.

> >

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

> > ---

> >  grub-core/kern/efi/mm.c | 46 ++++++++++++++++++++++++----------------------

> >  include/grub/efi/efi.h  |  5 +++++

> >  2 files changed, 29 insertions(+), 22 deletions(-)

> >

> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c

> > index 460a4b763..7b1763bc5 100644

> > --- a/grub-core/kern/efi/mm.c

> > +++ b/grub-core/kern/efi/mm.c

> > @@ -51,36 +51,20 @@ int grub_efi_is_finished = 0;

> >

> >  /* Allocate pages. Return the pointer to the first of allocated pages.  */

> >  void *

> > -grub_efi_allocate_pages (grub_efi_physical_address_t address,

> > -			 grub_efi_uintn_t pages)

> > +grub_efi_allocate_pages_real (grub_efi_physical_address_t address,

> 

> Could not you just add an extra argument to grub_efi_allocate_pages()?

> It is not called in many places. Do you really need separate

> grub_efi_allocate_pages_real()?


Need? No.

It gave an excuse to break some of the implicit semantics of the
existing function out separately from the bit that actually allocates
memory.

I also instinctively dislike functions with > 4 arguments due to
32-bit ARM ABI :)

/
    Leif

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper Aug. 1, 2017, noon | #3
On Fri, Jul 28, 2017 at 06:57:36PM +0100, Leif Lindholm wrote:
> On Thu, Jul 27, 2017 at 04:46:14PM +0200, Daniel Kiper wrote:

> > On Mon, Jun 12, 2017 at 03:53:36PM +0100, Leif Lindholm wrote:

> > > Expose a new function, grub_efi_allocate_pages_real(), making it possible

> > > to specify allocation type and memory type as supported by the UEFI

> > > AllocatePages boot service.

> > >

> > > Make grub_efi_allocate_pages() a consumer of the new function,

> > > maintaining its old functionality.

> > >

> > > Also delete some left-around #if 1/#else blocks in the affected

> > > functions.

> > >

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

> > > ---

> > >  grub-core/kern/efi/mm.c | 46 ++++++++++++++++++++++++----------------------

> > >  include/grub/efi/efi.h  |  5 +++++

> > >  2 files changed, 29 insertions(+), 22 deletions(-)

> > >

> > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c

> > > index 460a4b763..7b1763bc5 100644

> > > --- a/grub-core/kern/efi/mm.c

> > > +++ b/grub-core/kern/efi/mm.c

> > > @@ -51,36 +51,20 @@ int grub_efi_is_finished = 0;

> > >

> > >  /* Allocate pages. Return the pointer to the first of allocated pages.  */

> > >  void *

> > > -grub_efi_allocate_pages (grub_efi_physical_address_t address,

> > > -			 grub_efi_uintn_t pages)

> > > +grub_efi_allocate_pages_real (grub_efi_physical_address_t address,

> >

> > Could not you just add an extra argument to grub_efi_allocate_pages()?

> > It is not called in many places. Do you really need separate

> > grub_efi_allocate_pages_real()?

>

> Need? No.

>

> It gave an excuse to break some of the implicit semantics of the

> existing function out separately from the bit that actually allocates

> memory.


OK.

> I also instinctively dislike functions with > 4 arguments due to

> 32-bit ARM ABI :)


But after the suggested change you will have exactly 4 args. So?

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 460a4b763..7b1763bc5 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -51,36 +51,20 @@  int grub_efi_is_finished = 0;
 
 /* Allocate pages. Return the pointer to the first of allocated pages.  */
 void *
-grub_efi_allocate_pages (grub_efi_physical_address_t address,
-			 grub_efi_uintn_t pages)
+grub_efi_allocate_pages_real (grub_efi_physical_address_t address,
+			      grub_efi_uintn_t pages,
+			      grub_efi_allocate_type_t alloctype,
+			      grub_efi_memory_type_t memtype)
 {
-  grub_efi_allocate_type_t type;
   grub_efi_status_t status;
   grub_efi_boot_services_t *b;
 
-#if 1
   /* Limit the memory access to less than 4GB for 32-bit platforms.  */
   if (address > GRUB_EFI_MAX_USABLE_ADDRESS)
     return 0;
-#endif
-
-#if 1
-  if (address == 0)
-    {
-      type = GRUB_EFI_ALLOCATE_MAX_ADDRESS;
-      address = GRUB_EFI_MAX_USABLE_ADDRESS;
-    }
-  else
-    type = GRUB_EFI_ALLOCATE_ADDRESS;
-#else
-  if (address == 0)
-    type = GRUB_EFI_ALLOCATE_ANY_PAGES;
-  else
-    type = GRUB_EFI_ALLOCATE_ADDRESS;
-#endif
 
   b = grub_efi_system_table->boot_services;
-  status = efi_call_4 (b->allocate_pages, type, GRUB_EFI_LOADER_DATA, pages, &address);
+  status = efi_call_4 (b->allocate_pages, alloctype, memtype, pages, &address);
   if (status != GRUB_EFI_SUCCESS)
     return 0;
 
@@ -89,7 +73,7 @@  grub_efi_allocate_pages (grub_efi_physical_address_t address,
       /* Uggh, the address 0 was allocated... This is too annoying,
 	 so reallocate another one.  */
       address = GRUB_EFI_MAX_USABLE_ADDRESS;
-      status = efi_call_4 (b->allocate_pages, type, GRUB_EFI_LOADER_DATA, pages, &address);
+      status = efi_call_4 (b->allocate_pages, alloctype, memtype, pages, &address);
       grub_efi_free_pages (0, pages);
       if (status != GRUB_EFI_SUCCESS)
 	return 0;
@@ -98,6 +82,24 @@  grub_efi_allocate_pages (grub_efi_physical_address_t address,
   return (void *) ((grub_addr_t) address);
 }
 
+void *
+grub_efi_allocate_pages (grub_efi_physical_address_t address,
+			 grub_efi_uintn_t pages)
+{
+  grub_efi_allocate_type_t alloctype;
+
+  if (address == 0)
+    {
+      alloctype = GRUB_EFI_ALLOCATE_MAX_ADDRESS;
+      address = GRUB_EFI_MAX_USABLE_ADDRESS;
+    }
+  else
+    alloctype = GRUB_EFI_ALLOCATE_ADDRESS;
+
+  return grub_efi_allocate_pages_real (address, pages, alloctype,
+				       GRUB_EFI_LOADER_DATA);
+}
+
 /* Free pages starting from ADDRESS.  */
 void
 grub_efi_free_pages (grub_efi_physical_address_t address,
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 845fc2438..904722174 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -38,6 +38,11 @@  void *EXPORT_FUNC(grub_efi_open_protocol) (grub_efi_handle_t handle,
 int EXPORT_FUNC(grub_efi_set_text_mode) (int on);
 void EXPORT_FUNC(grub_efi_stall) (grub_efi_uintn_t microseconds);
 void *
+EXPORT_FUNC(grub_efi_allocate_pages_real) (grub_efi_physical_address_t address,
+				           grub_efi_uintn_t pages,
+					   grub_efi_allocate_type_t alloctype,
+					   grub_efi_memory_type_t memtype);
+void *
 EXPORT_FUNC(grub_efi_allocate_pages) (grub_efi_physical_address_t address,
 				      grub_efi_uintn_t pages);
 void EXPORT_FUNC(grub_efi_free_pages) (grub_efi_physical_address_t address,