Hi Leif,
Great thanks for your review! I have made some improvements(which marked DONE below), following your suggestion.
I may do one more improvement according to your feedback
There is one question inline below.
On 10/27/2014 09:40 PM, Leif Lindholm wrote:
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;'.
DONE
} -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.
DONE
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).
DONE
+{
- 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.
what do you mean about "a separate patch"? a patch in the same patchset or outside this patchset?
without this change, the multiboot patch won't work.
they don't share loaded status. And this loaded belong to linux.c, so if we do multiboot, this loaded == 0, so boot fail.
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
DONE
{
- 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).
DONE
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).
DONE
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.)
DONE
}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?
Yes ,you are right, DONE
+#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.
DONE
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.)
DONE
+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