Re: [Linaro-uefi] [PATCH v3 2/3] arm64: Make some shared functions for multiboot.c in linux.c

On Mon, Oct 27, 2014 at 04:44:31PM +0800, Fu Wei wrote:

arm64: Make some shared functions for multiboot.c Move some #define from grub-core/loader/arm64/linux.c to include/grub/arm64/linux.h Reason: (1)make code more orderly (2)these #define can be used by other files which includs include/grub/arm64/linux.h

Signed-off-by: Fu Wei fu.wei@linaro.org

grub-core/loader/arm64/linux.c | 160 ++++++++++++++++++++--------------------- include/grub/arm64/linux.h | 15 +++- 2 files changed, 90 insertions(+), 85 deletions(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c index 564a75a..b030b05 100644 --- a/grub-core/loader/arm64/linux.c +++ b/grub-core/loader/arm64/linux.c @@ -33,12 +33,6 @@ GRUB_MOD_LICENSE ("GPLv3+"); -#define GRUB_EFI_PAGE_SHIFT 12 -#define BYTES_TO_PAGES(bytes) (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT) -#define GRUB_EFI_PE_MAGIC 0x5A4D

-static grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;

static grub_dl_t my_mod; static int loaded; @@ -58,6 +52,7 @@ static void * get_firmware_fdt (void) { grub_efi_configuration_table_t *tables;

• grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID; void *firmware_fdt = NULL; unsigned int i;

@@ -75,8 +70,8 @@ get_firmware_fdt (void) return firmware_fdt; } -static void -get_fdt (void) +void * +grub_linux_get_fdt (void) { void *raw_fdt; grub_size_t size; @@ -99,7 +94,7 @@ get_fdt (void) grub_dprintf ("linux", "allocating %ld bytes for fdt\n", size); fdt = grub_efi_allocate_pages (0, BYTES_TO_PAGES (size)); if (!fdt)

• return;

• return NULL;

if (raw_fdt) { @@ -110,13 +105,14 @@ get_fdt (void) { grub_fdt_create_empty_tree (fdt, size); }

• return (fdt);

Just 'return fdt;'.

} -static grub_err_t -check_kernel (struct grub_arm64_linux_kernel_header *lh) +grub_err_t +grub_arm64_uefi_check_image (struct grub_arm64_linux_kernel_header * lh) { if (lh->magic != GRUB_ARM64_LINUX_MAGIC)

• return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");

• return grub_error (GRUB_ERR_BAD_OS, "invalid magic number");

This style fixup is unrelated to the new functionality. All such instances should be broken out into a separate patch, preceding this one in the series.

if ((lh->code0 & 0xffff) != GRUB_EFI_PE_MAGIC) return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, @@ -130,15 +126,74 @@ check_kernel (struct grub_arm64_linux_kernel_header *lh) return GRUB_ERR_NONE; } +grub_err_t +grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size, char *args)

If you move this function down to the original location of grub_linux_boot(), this will make the diff much smaller (and more clear).

+{

• grub_efi_memory_mapped_device_path_t *mempath;
• grub_efi_handle_t image_handle;
• grub_efi_boot_services_t *b;
• grub_efi_status_t status;
• grub_efi_loaded_image_t *loaded_image;
• int len;
• mempath = grub_malloc (2 * sizeof (grub_efi_memory_mapped_device_path_t));
• if (!mempath)
• return grub_errno;
• mempath[0].header.type = GRUB_EFI_HARDWARE_DEVICE_PATH_TYPE;
• mempath[0].header.subtype = GRUB_EFI_MEMORY_MAPPED_DEVICE_PATH_SUBTYPE;
• mempath[0].header.length = grub_cpu_to_le16_compile_time (sizeof (*mempath));
• mempath[0].memory_type = GRUB_EFI_LOADER_DATA;
• mempath[0].start_address = addr;
• mempath[0].end_address = addr + size;
• mempath[1].header.type = GRUB_EFI_END_DEVICE_PATH_TYPE;
• mempath[1].header.subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
• mempath[1].header.length = sizeof (grub_efi_device_path_t);
• b = grub_efi_system_table->boot_services;
• status = b->load_image (0, grub_efi_image_handle,

• 	  (grub_efi_device_path_t *) mempath,
  

• 	  (void *) addr, size, &image_handle);
  

• if (status != GRUB_EFI_SUCCESS)
• return grub_error (GRUB_ERR_BAD_OS, "cannot load image");
• grub_dprintf ("linux", "linux command line: '%s'\n", args);
• /* Convert command line to UCS-2 */
• loaded_image = grub_efi_get_loaded_image (image_handle);
• loaded_image->load_options_size = len =
• (grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
• loaded_image->load_options =
• grub_efi_allocate_pages (0,

• 	     BYTES_TO_PAGES (loaded_image->load_options_size));
  

• if (!loaded_image->load_options)
• return grub_errno;
• loaded_image->load_options_size =
• 2 * grub_utf8_to_utf16 (loaded_image->load_options, len,

• 	    (grub_uint8_t *) args, len, NULL);
  

• grub_dprintf ("linux", "starting image %p\n", image_handle);
• status = b->start_image (image_handle, 0, NULL);
• /* When successful, not reached */
• b->unload_image (image_handle);
• grub_efi_free_pages ((grub_efi_physical_address_t) loaded_image->load_options,

•        BYTES_TO_PAGES (loaded_image->load_options_size));
  

• return grub_errno;

+}

static grub_err_t -finalize_params (void) +finalize_params_linux (void) { grub_efi_boot_services_t *b;

• grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID; grub_efi_status_t status; int node, retval;

• get_fdt ();
• if (!fdt)

• if (!grub_linux_get_fdt ()) goto failure;

node = grub_fdt_find_subnode (fdt, 0, "chosen"); @@ -189,13 +244,6 @@ grub_cmd_devicetree (grub_command_t cmd __attribute__ ((unused)), void *blob = NULL; int size;

• if (!loaded)
• {

•  grub_error (GRUB_ERR_BAD_ARGUMENT,
  

•   N_("you need to load the kernel first"));
  

•  return GRUB_ERR_BAD_OS;
  

• }

This would make sense as a separate patch - it is not logically connected with the new functionality.

if (argc != 1) return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected")); @@ -243,65 +291,11 @@ out: static grub_err_t grub_linux_boot (void)

Again, if you moved grub_arm64_uefi_boot_image() down here, most of the diff would disappear.

{

• grub_efi_memory_mapped_device_path_t *mempath;
• grub_efi_handle_t image_handle;
• grub_efi_boot_services_t *b;
• grub_efi_status_t status;
• grub_err_t retval;
• grub_efi_loaded_image_t *loaded_image;
• int len;
• retval = finalize_params();
• if (retval != GRUB_ERR_NONE)
• return retval;
• mempath = grub_malloc (2 * sizeof (grub_efi_memory_mapped_device_path_t));
• if (!mempath)
• return grub_errno;
• mempath[0].header.type = GRUB_EFI_HARDWARE_DEVICE_PATH_TYPE;
• mempath[0].header.subtype = GRUB_EFI_MEMORY_MAPPED_DEVICE_PATH_SUBTYPE;
• mempath[0].header.length = grub_cpu_to_le16_compile_time (sizeof (*mempath));
• mempath[0].memory_type = GRUB_EFI_LOADER_DATA;
• mempath[0].start_address = (grub_addr_t) kernel_addr;
• mempath[0].end_address = (grub_addr_t) kernel_addr + kernel_size;
• mempath[1].header.type = GRUB_EFI_END_DEVICE_PATH_TYPE;
• mempath[1].header.subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
• mempath[1].header.length = sizeof (grub_efi_device_path_t);
• b = grub_efi_system_table->boot_services;
• status = b->load_image (0, grub_efi_image_handle,

• 	  (grub_efi_device_path_t *) mempath,
  

•                      kernel_addr, kernel_size, &image_handle);
  

• if (status != GRUB_EFI_SUCCESS)
• return grub_error (GRUB_ERR_BAD_OS, "cannot load image");
• grub_dprintf ("linux", "linux command line: '%s'\n", linux_args);
• /* Convert command line to UCS-2 */
• loaded_image = grub_efi_get_loaded_image (image_handle);
• loaded_image->load_options_size = len =
• (grub_strlen (linux_args) + 1) * sizeof (grub_efi_char16_t);
• loaded_image->load_options =
• grub_efi_allocate_pages (0,

• 	     BYTES_TO_PAGES (loaded_image->load_options_size));
  

• if (!loaded_image->load_options)

• if (finalize_params_linux () != GRUB_ERR_NONE) return grub_errno;

• loaded_image->load_options_size =
• 2 * grub_utf8_to_utf16 (loaded_image->load_options, len,

• 	    (grub_uint8_t *) linux_args, len, NULL);
  

• grub_dprintf("linux", "starting image %p\n", image_handle);
• status = b->start_image (image_handle, 0, NULL);
• /* When successful, not reached */
• b->unload_image (image_handle);
• grub_efi_free_pages ((grub_efi_physical_address_t) loaded_image->load_options,

•        BYTES_TO_PAGES (loaded_image->load_options_size));
  

• return grub_errno;

• return (grub_arm64_uefi_boot_image((grub_addr_t)kernel_addr,

•                                 kernel_size, linux_args));
  

} static grub_err_t @@ -367,7 +361,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)), grub_dprintf ("linux", "[addr=%p, size=0x%x]\n", (void *) initrd_start, initrd_size);

• fail:

+fail:

Unrelated style cleanup (add in separate patch with other style fixes).

grub_initrd_close (&initrd_ctx); if (initrd_mem && !initrd_start) grub_efi_free_pages ((grub_efi_physical_address_t) initrd_mem, @@ -400,10 +394,10 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)), if (grub_file_read (file, &lh, sizeof (lh)) < (long) sizeof (lh)) return grub_errno;

• if (check_kernel (&lh) != GRUB_ERR_NONE)

• if (grub_arm64_uefi_check_image (&lh) != GRUB_ERR_NONE) goto fail;

• grub_loader_unset();

• grub_loader_unset ();

Unrelated style cleanup (add in separate patch with other style fixes).

grub_dprintf ("linux", "kernel file size: %lld\n", (long long) kernel_size); kernel_addr = grub_efi_allocate_pages (0, BYTES_TO_PAGES (kernel_size)); @@ -450,8 +444,8 @@ fail: if (grub_errno != GRUB_ERR_NONE) {

•  grub_dl_unref (my_mod);
   loaded = 0;
  

•  grub_dl_unref (my_mod);
  

(This one could arguably go with the style cleanup - but in general, it is unrelated to the new functionality and could be submitted completely separately.)

 }

if (linux_args && !loaded) diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h index 864e5dc..81f050b 100644 --- a/include/grub/arm64/linux.h +++ b/include/grub/arm64/linux.h @@ -21,14 +21,18 @@ #include <grub/efi/efi.h> -#define GRUB_ARM64_LINUX_MAGIC 0x644d5241 /* 'ARM\x64' */ +#define GRUB_ARM64_LINUX_MAGIC 0x644d5241 /* 'ARM\x64' */

Unrelated style cleanup (add in separate patch with other style fixes). I assume this one was just caused by running indent?

+#define GRUB_EFI_PAGE_SHIFT 12 +#define BYTES_TO_PAGES(bytes) (((bytes) + 0xfff) >> GRUB_EFI_PAGE_SHIFT) +#define GRUB_EFI_PE_MAGIC 0x5A4D /* From linux/Documentation/arm64/booting.txt */ struct grub_arm64_linux_kernel_header { grub_uint32_t code0; /* Executable code */ grub_uint32_t code1; /* Executable code */

• grub_uint64_t text_offset; /* Image load offset */

• grub_uint64_t text_offset; /* Image load offset */

Hmm, this one looks weird. If the different comments still line up, this should still be a separate style fixup patch.

grub_uint64_t res0; /* reserved */ grub_uint64_t res1; /* reserved */ grub_uint64_t res2; /* reserved */ @@ -38,4 +42,11 @@ struct grub_arm64_linux_kernel_header grub_uint32_t hdr_offset; /* Offset of PE/COFF header */ }; +/* Declare the accessor functions for getting dtb and checking/booting image */

(Not accessor functions anymore - just drop the work accessor.)

+void *grub_linux_get_fdt (void); +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 */

1.9.1