| Message ID | 20170612145341.3351-3-leif.lindholm@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi: improved correctness, arm unification, and cleanup | expand |
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
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
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
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,
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