Re: [PATCH v2 09/14] arm64 linux loader: rename functions and macros and move to common headers

[Leif Lindholm]

On Thu, Aug 03, 2017 at 04:44:28PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> On Thu, Aug 3, 2017, 12:14 Leif Lindholm <address@hidden> wrote:
> 
> > In preparation for using the linux loader for 32-bit and 64-bit platforms,
> > rename
> > grub_arm64*/GRUB_ARM64* to grub_efi*/GRUB_EFI*.
> >
> > Move prototypes for now-common functions to efi/efi.h.
> >
> > Signed-off-by: Leif Lindholm <address@hidden>
> > ---
> >  grub-core/loader/arm64/linux.c    | 14 +++++++-------
> >  grub-core/loader/arm64/xen_boot.c | 12 ++++++------
> >  include/grub/arm64/linux.h        | 11 ++---------
> >  include/grub/efi/efi.h            |  4 ++++
> >  4 files changed, 19 insertions(+), 22 deletions(-)
> >
> > diff --git a/grub-core/loader/arm64/linux.c
> > b/grub-core/loader/arm64/linux.c
> > index c60469e53..8cd44230d 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -48,9 +48,9 @@ static grub_addr_t initrd_start;
> >  static grub_addr_t initrd_end;
> >
> >  grub_err_t
> > -grub_arm64_uefi_check_image (struct grub_arm64_linux_kernel_header * lh)
> > +grub_efi_linux_check_image (struct grub_linux_kernel_header * lh)
> >  {
> > -  if (lh->magic != GRUB_ARM64_LINUX_MAGIC)
> > +  if (lh->magic != GRUB_LINUX_MAGIC_SIGNATURE)
> >      return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
> >
> >    if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
> > @@ -109,7 +109,7 @@ failure:
> >  }
> >
> >  grub_err_t
> > -grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size, char
> > *args)
> > +grub_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
> >  {
> >    grub_efi_memory_mapped_device_path_t *mempath;
> >    grub_efi_handle_t image_handle;
> > @@ -173,8 +173,8 @@ grub_linux_boot (void)
> >    if (finalize_params_linux () != GRUB_ERR_NONE)
> >      return grub_errno;
> >
> > -  return (grub_arm64_uefi_boot_image((grub_addr_t)kernel_addr,
> > -                                     kernel_size, linux_args));
> > +  return (grub_efi_linux_boot_image((grub_addr_t)kernel_addr,
> > +                                    kernel_size, linux_args));
> >  }
> >
> >  static grub_err_t
> > @@ -250,7 +250,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> > ((unused)),
> >                 int argc, char *argv[])
> >  {
> >    grub_file_t file = 0;
> > -  struct grub_arm64_linux_kernel_header lh;
> > +  struct grub_linux_kernel_header lh;
> >
> >    grub_dl_ref (my_mod);
> >
> > @@ -269,7 +269,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__
> > ((unused)),
> >    if (grub_file_read (file, &lh, sizeof (lh)) < (long) sizeof (lh))
> >      return grub_errno;
> >
> > -  if (grub_arm64_uefi_check_image (&lh) != GRUB_ERR_NONE)
> > +  if (grub_efi_linux_check_image (&lh) != GRUB_ERR_NONE)
> >      goto fail;
> >
> >    grub_loader_unset();
> > diff --git a/grub-core/loader/arm64/xen_boot.c
> > b/grub-core/loader/arm64/xen_boot.c
> > index d092a53ed..e8720584b 100644
> > --- a/grub-core/loader/arm64/xen_boot.c
> > +++ b/grub-core/loader/arm64/xen_boot.c
> > @@ -67,7 +67,7 @@ typedef enum module_type module_type_t;
> >
> >  struct xen_hypervisor_header
> >  {
> > -  struct grub_arm64_linux_kernel_header efi_head;
> > +  struct grub_linux_kernel_header efi_head;
> >
> >    /* This is always PE\0\0.  */
> >    grub_uint8_t signature[GRUB_PE32_SIGNATURE_SIZE];
> > @@ -254,9 +254,9 @@ xen_boot (void)
> >    if (err)
> >      return err;
> >
> > -  return grub_arm64_uefi_boot_image (xen_hypervisor->start,
> > -                                    xen_hypervisor->size,
> > -                                    xen_hypervisor->cmdline);
> > +  return grub_efi_linux_boot_image (xen_hypervisor->start,
> > +                                   xen_hypervisor->size,
> > +                                   xen_hypervisor->cmdline);
> >  }
> >
> >  static void
> > @@ -458,8 +458,8 @@ grub_cmd_xen_hypervisor (grub_command_t cmd
> > __attribute__ ((unused)),
> >
> >    if (grub_file_read (file, &sh, sizeof (sh)) != (long) sizeof (sh))
> >      goto fail;
> > -  if (grub_arm64_uefi_check_image
> > -      ((struct grub_arm64_linux_kernel_header *) &sh) != GRUB_ERR_NONE)
> > +  if (grub_efi_linux_check_image
> > +      ((struct grub_linux_kernel_header *) &sh) != GRUB_ERR_NONE)
> >      goto fail;
> >    grub_file_seek (file, 0);
> >
> > diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
> > index a981df5d1..e53be83b1 100644
> > --- a/include/grub/arm64/linux.h
> > +++ b/include/grub/arm64/linux.h
> > @@ -19,12 +19,10 @@
> >  #ifndef GRUB_LINUX_CPU_HEADER
> >  #define GRUB_LINUX_CPU_HEADER 1
> >
> > -#include <grub/efi/efi.h>
> > -
> > -#define GRUB_ARM64_LINUX_MAGIC 0x644d5241 /* 'ARM\x64' */
> > +#define GRUB_LINUX_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
> >
> This doesn't sound arm64-independent.

No more then the x86 versions are?
The intent was to reuse a common name across architectures, with
architecture-specific content.

That was my interpretation of the i386 definition.
Are you suggesting that that too should be renamed?

> >
> >  /* From linux/Documentation/arm64/booting.txt */
> > -struct grub_arm64_linux_kernel_header
> > +struct grub_linux_kernel_header
> >  {
> >
> This makes it confusing with x86 counterpart.

If it is meant to be an x86-specific struct, why does it have an
architecture-independent sounding name?

/
    Leif

> >    grub_uint32_t code0;         /* Executable code */
> >    grub_uint32_t code1;         /* Executable code */
> > @@ -38,9 +36,4 @@ struct grub_arm64_linux_kernel_header
> >    grub_uint32_t hdr_offset;    /* Offset of PE/COFF header */
> >  };
> >
> > -grub_err_t grub_arm64_uefi_check_image (struct
> > grub_arm64_linux_kernel_header
> > -                                        *lh);
> > -grub_err_t grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size,
> > -                                       char *args);
> > -
> >  #endif /* ! GRUB_LINUX_CPU_HEADER */
> > diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> > index a58774efc..149969941 100644
> > --- a/include/grub/efi/efi.h
> > +++ b/include/grub/efi/efi.h
> > @@ -92,6 +92,10 @@ void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
> >  #endif
> >  #if defined(__aarch64__)
> >  grub_err_t EXPORT_FUNC(grub_efi_get_ram_base)(grub_addr_t *);
> > +#include <grub/cpu/linux.h>
> > +grub_err_t grub_efi_linux_check_image(struct grub_linux_kernel_header
> > *lh);
> > +grub_err_t grub_efi_linux_boot_image(grub_addr_t addr, grub_size_t size,
> > +                                    char *args);
> >  #endif
> >
> >  grub_addr_t grub_efi_modules_addr (void);
> > --
> > 2.11.0
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >

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