Re: [PATCH 2/7] efi: refactor grub_efi_allocate_pages

[Daniel Kiper]

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 <address@hidden>
> > > ---
> > >  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