Re: [PATCH v4 2/6] linux/arm: unify ARM/arm64 vs Xen PE/COFF header handling

[Daniel Kiper]

On Thu, Sep 08, 2022 at 03:30:13PM +0200, Ard Biesheuvel wrote:
> Xen has its own version of the image header, to account for the
> additional PE/COFF header fields. Since we are adding references to
> those in the shared EFI loader code, update the common definitions
> and drop the Xen specific one which no longer has a purpose.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  grub-core/loader/arm64/linux.c    | 12 +++++-----
>  grub-core/loader/arm64/xen_boot.c | 23 ++++----------------
>  include/grub/arm/linux.h          |  6 +++++
>  include/grub/arm64/linux.h        |  4 ++++
>  include/grub/efi/efi.h            |  4 +++-
>  5 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index b5b559c236e0..7c0f17cf933d 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -49,8 +49,13 @@ static grub_addr_t initrd_start;
>  static grub_addr_t initrd_end;
>
>  grub_err_t
> -grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh)
> +grub_arch_efi_linux_load_image_header (grub_file_t file,
> +                                      struct linux_arch_kernel_header * lh)
>  {
> +  grub_file_seek (file, 0);
> +  if (grub_file_read (file, lh, sizeof (*lh)) < (long) sizeof (*lh))

s/(long)/(grub_ssize_t)/

Please mention this change in the commit message.

> +    return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read Linux image 
> header");
> +
>    if ((lh->code0 & 0xffff) != GRUB_PE32_MAGIC)
>      return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
>                      N_("plain image kernel not supported - rebuild with 
> CONFIG_(U)EFI_STUB enabled"));
> @@ -301,10 +306,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ 
> ((unused)),
>
>    kernel_size = grub_file_size (file);
>
> -  if (grub_file_read (file, &lh, sizeof (lh)) < (long) sizeof (lh))
> -    return grub_errno;
> -
> -  if (grub_arch_efi_linux_check_image (&lh) != GRUB_ERR_NONE)
> +  if (grub_arch_efi_linux_load_image_header (file, &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 22cc25eccd9d..e5895ee78218 100644
> --- a/grub-core/loader/arm64/xen_boot.c
> +++ b/grub-core/loader/arm64/xen_boot.c
> @@ -31,7 +31,6 @@
>  #include <grub/efi/efi.h>
>  #include <grub/efi/fdtload.h>
>  #include <grub/efi/memory.h>
> -#include <grub/efi/pe32.h>   /* required by struct xen_hypervisor_header */
>  #include <grub/i18n.h>
>  #include <grub/lib/cmdline.h>
>
> @@ -65,18 +64,6 @@ enum module_type
>  };
>  typedef enum module_type module_type_t;
>
> -struct xen_hypervisor_header
> -{
> -  struct linux_arm64_kernel_header efi_head;
> -
> -  /* This is always PE\0\0.  */
> -  grub_uint8_t signature[GRUB_PE32_SIGNATURE_SIZE];
> -  /* The COFF file header.  */
> -  struct grub_pe32_coff_header coff_header;
> -  /* The Optional header.  */
> -  struct grub_pe64_optional_header optional_header;
> -};
> -
>  struct xen_boot_binary
>  {
>    struct xen_boot_binary *next;
> @@ -452,7 +439,7 @@ static grub_err_t
>  grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)),
>                        int argc, char *argv[])
>  {
> -  struct xen_hypervisor_header sh;
> +  struct linux_arm64_kernel_header lh;
>    grub_file_t file = NULL;
>
>    grub_dl_ref (my_mod);
> @@ -467,10 +454,7 @@ grub_cmd_xen_hypervisor (grub_command_t cmd 
> __attribute__ ((unused)),
>    if (!file)
>      goto fail;
>
> -  if (grub_file_read (file, &sh, sizeof (sh)) != (long) sizeof (sh))
> -    goto fail;
> -  if (grub_arch_efi_linux_check_image
> -      ((struct linux_arch_kernel_header *) &sh) != GRUB_ERR_NONE)
> +  if (grub_arch_efi_linux_load_image_header (file, &lh) != GRUB_ERR_NONE)
>      goto fail;
>    grub_file_seek (file, 0);
>
> @@ -484,7 +468,8 @@ grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ 
> ((unused)),
>      return grub_errno;
>
>    xen_hypervisor->is_hypervisor = 1;
> -  xen_hypervisor->align = (grub_size_t) sh.optional_header.section_alignment;
> +  xen_hypervisor->align
> +    = (grub_size_t) lh.coff_image_header.optional_header.section_alignment;
>
>    xen_boot_binary_load (xen_hypervisor, file, argc, argv);
>    if (grub_errno == GRUB_ERR_NONE)
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index bcd5a7eb186e..ea815db13417 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -22,6 +22,8 @@
>
>  #include "system.h"
>
> +#include <grub/efi/pe32.h>
> +
>  #define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
>
>  struct linux_arm_kernel_header {
> @@ -32,6 +34,10 @@ struct linux_arm_kernel_header {
>    grub_uint32_t end;   /* _edata */
>    grub_uint32_t reserved2[3];
>    grub_uint32_t hdr_offset;
> +

Please drop this empty line.

> +#if defined GRUB_MACHINE_EFI
> +  struct grub_coff_image_header coff_image_header;
> +#endif
>  };
>
>  #if defined(__arm__)
> diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
> index 7e22b4ab6990..c5ea9e8f503a 100644
> --- a/include/grub/arm64/linux.h
> +++ b/include/grub/arm64/linux.h
> @@ -21,6 +21,8 @@
>
>  #include <grub/types.h>
>

Please drop this empty line.

> +#include <grub/efi/pe32.h>
> +
>  #define GRUB_LINUX_ARM64_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
>
>  /* From linux/Documentation/arm64/booting.txt */
> @@ -36,6 +38,8 @@ struct linux_arm64_kernel_header
>    grub_uint64_t res4;                /* reserved */
>    grub_uint32_t magic;               /* Magic number, little endian, 
> "ARM\x64" */
>    grub_uint32_t hdr_offset;  /* Offset of PE/COFF header */
> +

Please drop this empty line.

> +  struct grub_coff_image_header coff_image_header;

Daniel