On 10/29/2014 06:33 AM, Leif Lindholm wrote:
On Wed, Oct 29, 2014 at 12:53:05AM +0800, Fu Wei wrote:
Hi Leif,
Great thanks for your review! I have made some improvements(which marked DONE below), following your suggestion.
But I still have some question or explanation, please the feedback inline below.
On 10/27/2014 11:51 PM, Leif Lindholm wrote:
On Mon, Oct 27, 2014 at 04:49:17PM +0800, Fu Wei wrote:
arm64: Add multiboot support file grub-core/loader/arm64/multiboot.c include/grub/arm64/multiboot.h
Add commands register code and hearder file into grub-core/loader/arm64/linux.c
This multiboot support is built into linux module for aarch64.
The implementation for Xen is following <Multiboot on ARM Specification>: http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot and xen/docs/misc/arm/device-tree/booting.txt in Xen source code.
This adds support for the Xen Multiboot on ARM specification for arm64, enabling config file portability across the architectures.
The multiboot command is currently x86-only, so reusing these command names should not conflict with any future additions of ARM support to multiboot2.
The reason of adding this functionality to the existing "linux" module rather than "multiboot(2)" (1)multiboot is x86 only (2)Multiboot is added to "linux" module because it reuses existing code.
Signed-off-by: Fu Wei fu.wei@linaro.org
grub-core/Makefile.core.def | 1 + grub-core/loader/arm64/linux.c | 6 + grub-core/loader/arm64/multiboot.c | 623 +++++++++++++++++++++++++++++++++++++ include/grub/arm64/multiboot.h | 114 +++++++ 4 files changed, 744 insertions(+)
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def index 42443bc..b963a62 100644 --- a/grub-core/Makefile.core.def +++ b/grub-core/Makefile.core.def @@ -1673,6 +1673,7 @@ module = { ia64_efi = loader/ia64/efi/linux.c; arm = loader/arm/linux.c; arm64 = loader/arm64/linux.c;
- arm64 = loader/arm64/multiboot.c; fdt = lib/fdt.c; common = loader/linux.c; common = lib/cmdline.c;
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c index b030b05..4f10a04 100644 --- a/grub-core/loader/arm64/linux.c +++ b/grub-core/loader/arm64/linux.c @@ -26,6 +26,7 @@ #include <grub/mm.h> #include <grub/types.h> #include <grub/cpu/linux.h> +#include <grub/cpu/multiboot.h> #include <grub/efi/efi.h> #include <grub/efi/pe32.h> #include <grub/i18n.h> @@ -470,6 +471,9 @@ GRUB_MOD_INIT (linux) cmd_devicetree = grub_register_command ("devicetree", grub_cmd_devicetree, 0, N_("Load DTB file."));
- grub_arm64_linux_register_multiboot_command (mod);
- my_mod = mod;
} @@ -478,4 +482,6 @@ GRUB_MOD_FINI (linux) grub_unregister_command (cmd_linux); grub_unregister_command (cmd_initrd); grub_unregister_command (cmd_devicetree);
- grub_arm64_linux_unregister_multiboot_command ();
} diff --git a/grub-core/loader/arm64/multiboot.c b/grub-core/loader/arm64/multiboot.c new file mode 100644 index 0000000..5e4f1ae --- /dev/null +++ b/grub-core/loader/arm64/multiboot.c @@ -0,0 +1,623 @@ +/*
- GRUB -- GRand Unified Bootloader
- Copyright (C) 2014 Free Software Foundation, Inc.
- GRUB is free software: you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation, either version 3 of the License, or
- (at your option) any later version.
- GRUB is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with GRUB. If not, see http://www.gnu.org/licenses/.
- */
+#include <grub/cache.h> +#include <grub/charset.h> +#include <grub/command.h> +#include <grub/err.h> +#include <grub/file.h> +#include <grub/fdt.h> +#include <grub/linux.h> +#include <grub/list.h> +#include <grub/loader.h> +#include <grub/misc.h> +#include <grub/mm.h> +#include <grub/types.h> +#include <grub/cpu/linux.h> +#include <grub/cpu/multiboot.h> +#include <grub/efi/efi.h> +#include <grub/efi/pe32.h> +#include <grub/i18n.h> +#include <grub/lib/cmdline.h>
+static grub_dl_t linux_mod; +static int loaded;
+static multiboot_module_t kernel = NULL; +static multiboot_module_t module_head = NULL; +static module_type_t module_type = MODULE_IMAGE;
This global "module_type" and the operations on it really confuse me when reading the code below. Can the global variable be removed?
Yes, you are right, I can move it into set_module_type as a static variable. DONE
+static const grub_size_t module_default_align[] = {
- MODULE_IMAGE_MIN_ALIGN,
- MODULE_INITRD_MIN_ALIGN,
- MODULE_OTHER_MIN_ALIGN,
- MODULE_CUSTOM_MIN_ALIGN
+};
+static void *multiboot_fdt = NULL; +static const compatible_struct_t default_compatible[] = {
- FDT_COMPATIBLE (MODULE_IMAGE_COMPATIBLE),
- FDT_COMPATIBLE (MODULE_INITRD_COMPATIBLE),
- FDT_COMPATIBLE (MODULE_OTHER_COMPATIBLE)
+};
+static grub_err_t +clean_extra_info (multiboot_module_t module) +{
- if (module && module->extra_info)
- {
if (((fdt_node_info_t) module->extra_info)->type == MODULE_CUSTOM)- grub_free (((fdt_node_info_t) module->extra_info)->compatible_custom);
grub_free (module->extra_info);module->extra_info = NULL;- }
- return (GRUB_ERR_NONE);
+}
+static grub_err_t +set_module_type (multiboot_module_t module, int *argc, char **argv[]) +{
- fdt_node_info_t node_info = (fdt_node_info_t) module->extra_info;
- char *temp = NULL;
- grub_size_t temp_size = 0, total_size = 0;
- int compatible_counter = 0, i = 0;
What is compatible_counter used for? Its meaning is not clear to me.
if the compatible is custom, this variable is used to log the number of the string.
--type "multiboot,ramdisk" --type "multiboot,module"
so ,compatible_counter == 2
Ok. I think the code would actually be more readable if this was called "num_types" or "num_options" or something similar.
Ah, thank you, I like num_types :-)
DONE
- char **compatible_temp_array =
- (char **) grub_zalloc (sizeof (char *) * (*argc));
What is compatible_temp_array used for? Its meaning is not clear to me.
if the compatible is custom, this variable is used to log the strings pointers.
if --type "multiboot,ramdisk" --type "multiboot,module"
compatible_temp_array[0] = "multiboot,ramdisk" compatible_temp_array[1] = "multiboot,module"
But we still free it at the end of this function. Could we instead just save pointers here?
yes, I also want to make a auto variable here, but we don't know the size of the array(and the size of compatible_custom). That is decided by the number of options(and the size of each --type options). so that is why I do "grub_zalloc (sizeof (char *) * (*argc))" , the number of options must be less than argc.
I used a MAX define for the compatible_custom, but there is not that limit in ePAPR. so I am using dynamic way.
- if ((*argc) >= 3)
- {
while ((*argc) > 1)- {
if (grub_strcmp (*argv[0], "--type") == 0){node_info->type = MODULE_CUSTOM;(*argc)--;(*argv)++;temp_size = grub_strlen (*argv[0]) + 1;temp = (char *) grub_zalloc (temp_size);grub_strcpy (temp, *argv[0]);compatible_temp_array[compatible_counter] = temp;total_size += temp_size;compatible_counter++;(*argc)--;(*argv)++;}elsebreak;- }
- }
- if (node_info->type != MODULE_CUSTOM)
- {
node_info->type = module_type++;While logically correct, this is very difficult to read code. What is the special case we are checking for here?.
if (module_type > MODULE_OTHER)- module_type = MODULE_OTHER;
Indentation looks a bit funky.
If the module_type has been corrupted, should we not abort with an error rather than try to fix up its type field?
can we do this :
if (node_info->type != MODULE_CUSTOM) { node_info->type = module_type++;
The statement module_type++ is still very confusing. If we do explicit assigments of types instead of shifting to adjacent types, the code will be understandable by someone who does not know the protocol by heart.
if (module_type = (MODULE_OTHER +1))module_type = MODULE_OTHER; if (module_type > (MODULE_OTHER +1)) REEOE!!
Why are we performing arithmetic on a type?
OK . let me try switch-case this time. :-)
let me know if you like it :-)
if (module->node_info.type != MODULE_CUSTOM) { module->node_info.type = type_by_order; switch (type_by_order) { case MODULE_IMAGE: type_by_order = MODULE_INITRD; break;
case MODULE_INITRD: type_by_order = MODULE_OTHER; break;
case MODULE_OTHER: break;
default: type_by_order = MODULE_IMAGE; /* error, reset the type */ return (grub_error (GRUB_ERR_BAD_ARGUMENT, N_("error module type"))); } }
but I don't think we need to do this
all this is for following the http://wiki.xen.org/wiki/Xen_ARM_with_Virtualization_Extensions/Multiboot#Co...
(1)"Each module will be given a default compatibility property based on the order in which the modules are added." (2)Possible extension: Allow the module command to take an optional: module --type "multiboot,ramdisk" --type "multiboot,module" <...>
Understood. It is still the legibility of the code that concerns me, not its function.
Yes, got you
- }
- else
- {
node_info->compatible_custom = temp = (char *) grub_zalloc (total_size);node_info->compatible_size = total_size;for (i = 0; compatible_counter > 0; compatible_counter--, i++, temp++)- {
grub_strcpy (temp, compatible_temp_array[i]);temp += grub_strlen (compatible_temp_array[i]);grub_free (compatible_temp_array[i]);- }
- }
- grub_free (compatible_temp_array);
- return (GRUB_ERR_NONE);
+}
+static grub_err_t +prepare_kernel_params (multiboot_module_t module) +{
- int retval;
- int chosen_node = 0;
- multiboot_fdt = grub_linux_get_fdt ();
- if (!multiboot_fdt)
- return (grub_error (GRUB_ERR_BAD_OS, "failed to get FDT"));
- chosen_node = grub_fdt_find_subnode (multiboot_fdt, 0, "chosen");
- if (chosen_node < 0)
- chosen_node = grub_fdt_add_subnode (multiboot_fdt, 0, "chosen");
- if (chosen_node < 1)
- return (grub_error (GRUB_ERR_BAD_OS, "failed to get chosen node"));
- grub_dprintf ("multiboot_loader",
"Multiboot Kernel cmdline : %s @ %p size:%d\n",module->cmdline, module->cmdline, module->cmdline_size);- retval = grub_fdt_set_prop (multiboot_fdt, chosen_node,
"bootargs", module->cmdline,module->cmdline_size);Should fit on two lines?
yes, it should be, but "indent" did this way. I have fixed it manually.
Huh? Crazy.
DONE
- if (retval)
- return (grub_error (GRUB_ERR_BAD_OS,
"failed to set kernel cmdline info to chosen node"));Reuse existing message from Linux loader: "failed to install/update FDT".
DONE
- return (GRUB_ERR_NONE);
+}
+static grub_err_t +prepare_module_params (multiboot_module_t module) +{
- int retval, chosen_node = 0, module_node = 0;
- fdt_node_info_t node_info = NULL;
- char module_name[FDT_NODE_NAME_MAX_SIZE];
- if (module && module->extra_info)
- {
node_info = (fdt_node_info_t) module->extra_info;- }
- else
- {
return (grub_error (GRUB_ERR_BAD_ARGUMENT, N_("failed to get module")));- }
- retval = grub_snprintf (module_name, FDT_NODE_NAME_MAX_SIZE,
"module@%lx",multiboot_address_align (module->start,module->align));- grub_dprintf ("multiboot_loader", "Module node name %s \n", module_name);
- if (retval < (int) sizeof ("module@"))
- return (grub_error (GRUB_ERR_BAD_OS,
N_("failed to set node name for module")));- chosen_node = grub_fdt_find_subnode (multiboot_fdt, 0, "chosen");
- if (chosen_node < 0)
- chosen_node = grub_fdt_add_subnode (multiboot_fdt, 0, "chosen");
- if (chosen_node < 1)
- return (grub_error (GRUB_ERR_BAD_OS, "failed to get chosen node"));
- module_node =
- grub_fdt_find_subnode (multiboot_fdt, chosen_node, module_name);
- if (module_node < 0)
- module_node =
grub_fdt_add_subnode (multiboot_fdt, chosen_node, module_name);- retval = grub_fdt_set_prop (multiboot_fdt, module_node, "compatible",
node_info->compatible,(grub_uint32_t) node_info->compatible_size);- if (retval)
- return (grub_error (GRUB_ERR_BAD_OS,
N_("failed to get module node in chosen node")));- grub_dprintf ("multiboot_loader",
"Module %s compatible = %s size = 0x%lx\n", module->name,node_info->compatible, node_info->compatible_size);- retval = grub_fdt_set_reg64 (multiboot_fdt, module_node,
multiboot_address_align (module->start,module->align),module->size);- if (retval)
- return (grub_error (GRUB_ERR_BAD_OS,
N_("failed to set reg info in module node")));- if ((module->cmdline) && (module->cmdline_size > 0))
- {
grub_dprintf ("multiboot_loader","Module %s cmdline : %s @ %p size:%d\n", module->name,module->cmdline, module->cmdline, module->cmdline_size);retval = grub_fdt_set_prop (multiboot_fdt, module_node,"bootargs", module->cmdline,module->cmdline_size + 1);if (retval)- return (grub_error (GRUB_ERR_BAD_OS,
"failed to set module cmdline info to chosen node"));- }
- else
- {
grub_dprintf ("multiboot_loader", "Module %s has not bootargs!\n",module->name);- }
- return (GRUB_ERR_NONE);
+}
+static grub_err_t +install_all_params (void) +{
- grub_efi_boot_services_t *b;
- grub_efi_status_t status;
- grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
- b = grub_efi_system_table->boot_services;
- status = b->install_configuration_table (&fdt_guid, multiboot_fdt);
- if (status != GRUB_EFI_SUCCESS)
- {
grub_dprintf ("multiboot_loader","Installed/updated FDT configuration table fail %p\n",multiboot_fdt);Copy the message from linux.c: "failed to install/update FDT"
DONE
return (GRUB_ERR_BAD_OS);- }
- grub_dprintf ("multiboot_loader",
"Installed/updated FDT configuration table @ %p\n",multiboot_fdt);- return (GRUB_ERR_NONE);
+}
+static grub_err_t +clean_all_params (void) +{
- if (multiboot_fdt)
- {
grub_efi_free_pages ((grub_efi_physical_address_t) multiboot_fdt,BYTES_TO_PAGES (grub_fdt_get_totalsize(multiboot_fdt)));multiboot_fdt = NULL;- }
- return (GRUB_ERR_NONE);
+}
+static grub_err_t +init_module_info (multiboot_module_t module, int *argc, char **argv[]) +{
I don't think this needs to be a separate function anymore - just merge into cmd_module().
Yes, I have merged it. DONE
- fdt_node_info_t node_info =
- (fdt_node_info_t) grub_zalloc (sizeof (struct fdt_node_info));
- if (!node_info)
- return (grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")));
- module->extra_info = (void *) node_info;
- /* get module type */
- grub_errno = set_module_type (module, argc, argv);
- if (grub_errno != GRUB_ERR_NONE)
- return (grub_errno);
- switch (node_info->type)
- {
- case MODULE_IMAGE:
- case MODULE_INITRD:
- case MODULE_OTHER:
node_info->compatible = default_compatible[node_info->type].compatible;node_info->compatible_size = default_compatible[node_info->type].size;break;- case MODULE_CUSTOM:
/* node_info->compatible_size has set in set_module_type() */node_info->compatible = node_info->compatible_custom;break;- default:
return (grub_error (GRUB_ERR_BAD_ARGUMENT, N_("error module type")));- }
- module->name = node_info->compatible;
- module->align = module_default_align[node_info->type];
- grub_dprintf ("multiboot_loader", "Init %s module and node info:\n"
"compatible %s\ncompatible_size 0x%lx\n",module->name,node_info->compatible, node_info->compatible_size);- return (GRUB_ERR_NONE);
+}
+static grub_err_t +finalize_params_multiboot (void) +{
- multiboot_module_t module;
- grub_err_t retval;
- /* Set Multiboot Kernel params info */
- module =
- (multiboot_module_t)
- grub_named_list_find (GRUB_AS_NAMED_LIST (module_head),
GRUB_MULTIBOOT_KERNEL_NAME);- if (module)
- {
kernel = module;retval = prepare_kernel_params (module);if (retval != GRUB_ERR_NONE)- goto failure_kernel;
grub_list_remove (GRUB_AS_LIST (module));- }
- else
- {
grub_dprintf ("multiboot_loader","Please load Multiboot Kernel first!\n");goto failure;- }
- /* Set module params info */
- FOR_LIST_ELEMENTS (module, module_head)
- {
- if (module->start && module->size > 0)
{- grub_dprintf ("multiboot_loader", "Module %s @ 0x%lx size:0x%lx\n",
module->name,multiboot_address_align (module->start, module->align),module->size);- retval = prepare_module_params (module);
- if (retval != GRUB_ERR_NONE)
goto failure_module;}- else
{- grub_dprintf ("multiboot_loader", "Module info error: %s!\n",
module->name);- goto failure_module;
}- }
- grub_list_push (GRUB_AS_LIST_P (&module_head), GRUB_AS_LIST (kernel));
- if (install_all_params () != GRUB_ERR_NONE)
- {
goto failure_kernel;- }
- return (GRUB_ERR_NONE);
+failure_module:
- grub_list_push (GRUB_AS_LIST_P (&module_head), GRUB_AS_LIST (kernel));
+failure_kernel:
- clean_all_params ();
+failure:
- return (grub_error (GRUB_ERR_BAD_OS, "failed to install/update FDT"));
+}
+static grub_err_t +grub_multiboot_boot (void)
There is no need for a static function to have a grub_ prefix.
for this function, it is just like the "grub_linux_boot" in linux.c I think it is OK to keep this prefix.
if not, maybe we also need to modify "grub_linux_boot" to "linux_boot"
do you agree??
I agree, but I don't think the Linux one strictly needs to change. (I would simply not call it that if I created it again.)
So you prefer multiboot_boot in multiboot.c and grub_linux_boot in linux.c or just keep grub_multiboot_boot ??
+{
- if (finalize_params_multiboot () != GRUB_ERR_NONE)
- return (grub_errno);
- return (grub_arm64_uefi_boot_image
(kernel->start, kernel->size, kernel->cmdline));+}
+static void +single_module_unload (multiboot_module_t module) +{
- if (module && module->start && module->size > 0)
- {
grub_efi_free_pages ((grub_efi_physical_address_t) module->start,BYTES_TO_PAGES (module->size + module->align));- }
- if (module && module->cmdline && module->cmdline_size > 0)
- {
grub_free (module->cmdline);grub_dprintf ("multiboot_loader","Module %s cmdline memory free @ %p size: %d\n",module->name, module->cmdline, module->cmdline_size);- }
- if (module)
- {
clean_extra_info (module);grub_list_remove (GRUB_AS_LIST (module));grub_dprintf ("multiboot_loader","Module %s struct memory free @ %p size: 0x%lx\n",module->name, module, sizeof (module));grub_free (module);- }
- return;
+}
+static grub_err_t +grub_module_unload_all (void)
There is no need for a static function to have a grub_ prefix.
yes, you are right, grub_module_unload_all --> all_modules_unload
I think that name is better, because there is a single_module_unload above.
Agreed.
DONE
+{
- multiboot_module_t module;
- FOR_LIST_ELEMENTS (module, module_head)
- {
- single_module_unload (module);
- }
- return (GRUB_ERR_NONE);
+}
+static grub_err_t +grub_multiboot_unload (void) +{
- loaded = 0;
- grub_module_unload_all ();
- clean_all_params ();
- grub_dl_unref (linux_mod);
- return (GRUB_ERR_NONE);
+}
+static grub_err_t +grub_module_load (multiboot_module_t module, grub_file_t file,
int argc, char *argv[])There is no need for a static function to have a grub_ prefix, and in this case it can easily be confused with grub_dl_load().
yes, you are right, grub_module_load --> multiboot_module_load
Good.
DONE
+{
- module->size = grub_file_size (file);
- grub_dprintf ("multiboot_loader", "Multiboot %s file size: 0x%lx\n",
module->name, module->size);- module->start = (grub_addr_t) grub_efi_allocate_pages (0,
(BYTES_TO_PAGES(module->size +module->align)));- if (!module->start)
- return (grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")));
- grub_dprintf ("multiboot_loader", "Multiboot %s numpages: 0x%lx\n",
module->name, BYTES_TO_PAGES (module->size + module->align));- if (grub_file_read (file,
(void *) multiboot_address_align (module->start,module->align),module->size) < (grub_ssize_t) module->size)- {
single_module_unload (module);return (grub_error (GRUB_ERR_BAD_OS,N_("premature end of file %s"), argv[0]));- }
- /* Skip the multiboot module file name */
- argc--;
- argv++;
- if (argc > 0)
- {
module->cmdline_size = grub_loader_cmdline_size (argc, argv);module->cmdline = grub_zalloc (module->cmdline_size);if (!module->cmdline)- {
single_module_unload (module);return (grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")));- }
grub_create_loader_cmdline (argc, argv,module->cmdline, module->cmdline_size);grub_dprintf ("multiboot_loader","Multiboot %s cmdline @ %p %s, size: %d\n", module->name,module->cmdline, module->cmdline, module->cmdline_size);- }
- else
- {
module->cmdline_size = 0;module->cmdline = NULL;- }
- return (GRUB_ERR_NONE);
+}
+static grub_err_t +grub_cmd_module (grub_command_t cmd __attribute__ ((unused)),
int argc, char *argv[])+{
- grub_file_t file = 0;
- multiboot_module_t module = NULL;
- if (argc == 0)
- {
grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));goto fail;- }
- if (!loaded)
- {
grub_error (GRUB_ERR_BAD_ARGUMENT,N_("Please use multiboot command first"));goto fail;- }
- module =
- (multiboot_module_t) grub_zalloc (sizeof (struct multiboot_module));
- if (!module)
- return (grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")));
- grub_errno = init_module_info (module, &argc, &argv);
Why pass pointers to argc/argv?
I have merged init_module_info into here, but set_module_type (module, &argc, &argv) still use that.
Reason: the function will process all the options (just --type, for now, but maybe we will have --align, maybe), and make the following program just assume argv[0] is module binary file and the bootargs start with argv[1].
maybe we can improve it , any idea??
Well, my point is simply that it becomes quite unintuitive when something static and fixed can change. argc/argv should remain unmodified, so there is no value in passing them as pointers. The law of least surprise.
yes, just for set_module_type, we do not need to pass pointers of argc/argv. but set_module_type also help us to skip all the options. If we just pass argc/argv, we still need to skip the options using another function or code.
So maybe we can do that: (1)pass argc/argv to set_module_type (2) make a new function ,maybe call filter_options/skip_options to make argc_option_free/argv_option_free variable (3) the following code just use argc_option_free/argv_option_free, if they don't want the options string.
any idea??
- if (grub_errno != GRUB_ERR_NONE)
- goto fail;
- file = grub_file_open (argv[0]);
- if (!file)
- goto fail;
- grub_errno = grub_module_load (module, file, argc, argv);
Why do we use grub_module_load to load Xen? Does it need to be added to the multiboot list?
this is in grub_cmd_module. but yes ,I treat Xen kernel as a module when I load it into RAM, because at that time, all the things are just like a normal module.
we can put Xen kernel out side the named list, but do we need to ?
I am not sure - it just did not look like what I was expecting. Does Xen do anything with itself based on the information in the list?
No, just GRUB process the list, Xen just need dtb.
At the beginning, I precess Xen kernel and modules separately, but I found there is too much common code. so I decide to make a common function to load all the binaries and link them in a same list(but Xen kernel has a special name MULTIBOOT_KERNEL_NAME, so we can find it's info when we prepare the dtb info for xen kernel),
But for the function name : multiboot_module_load, I think I should call it "multiboot_binary_load" or "multiboot_file_load"
any suggestion ??
- if (grub_errno == GRUB_ERR_NONE)
- grub_list_push (GRUB_AS_LIST_P (&module_head), GRUB_AS_LIST (module));
+fail:
- if (file)
- grub_file_close (file);
- if (grub_errno != GRUB_ERR_NONE)
- single_module_unload (module);
- return (grub_errno);
+}
+static grub_err_t +grub_cmd_multiboot (grub_command_t cmd __attribute__ ((unused)),
int argc, char *argv[])+{
- grub_file_t file = NULL;
- multiboot_module_t module = NULL;
- struct stub_header sh;
Just call it struct kernel_header.
DONE
- grub_dl_ref (linux_mod);
- if (argc == 0)
- {
grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));goto fail;- }
- file = grub_file_open (argv[0]);
- if (!file)
- goto fail;
- 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)- goto fail;
- grub_file_seek (file, 0);
- grub_loader_unset ();
- module =
- (multiboot_module_t) grub_zalloc (sizeof (struct multiboot_module));
- if (!module)
- return (grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")));
- module->name = GRUB_MULTIBOOT_KERNEL_NAME;
- module->extra_info = NULL;
- module->align = (grub_size_t) sh.optional_header.section_alignment;
- grub_errno = grub_module_load (module, file, argc, argv);
Why are we loading the Xen image as a module?
Sorry, as I said at loading time, all the things of xen image are just like a normal module.
Same as above.
- if (grub_errno == GRUB_ERR_NONE)
- {
grub_list_push (GRUB_AS_LIST_P (&module_head), GRUB_AS_LIST (module));grub_loader_set (grub_multiboot_boot, grub_multiboot_unload, 0);loaded = 1;- }
+fail:
- if (file)
- grub_file_close (file);
- if (grub_errno != GRUB_ERR_NONE)
- {
loaded = 0;grub_module_unload_all ();grub_dl_unref (linux_mod);- }
- return (grub_errno);
+}
+static grub_command_t cmd_multiboot, cmd_module;
+void +grub_arm64_linux_register_multiboot_command (grub_dl_t mod) +{
- cmd_multiboot =
- grub_register_command ("multiboot", grub_cmd_multiboot, 0,
N_("Load a multiboot kernel."));- cmd_module =
- grub_register_command ("module", grub_cmd_module, 0,
N_("Load a multiboot module."));- linux_mod = mod;
+}
+void +grub_arm64_linux_unregister_multiboot_command (void) +{
- grub_unregister_command (cmd_multiboot);
- grub_unregister_command (cmd_module);
+} diff --git a/include/grub/arm64/multiboot.h b/include/grub/arm64/multiboot.h new file mode 100644 index 0000000..e506b46 --- /dev/null +++ b/include/grub/arm64/multiboot.h @@ -0,0 +1,114 @@ +/*
- multiboot.h - Multiboot header file for Xen multiboot via FDT
- on AArch64 architecture.
- Copyright (C) 2014 Free Software Foundation, Inc.
- GRUB is free software: you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation, either version 3 of the License, or
- (at your option) any later version.
- GRUB is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with GRUB. If not, see http://www.gnu.org/licenses/.
- */
+#ifndef MULTIBOOT_HEADER +#define MULTIBOOT_HEADER 1
+#include <grub/list.h> +#include <grub/types.h> +#include <grub/efi/pe32.h> /* required by struct stub_header */
+#define GRUB_MULTIBOOT_KERNEL_NAME "multiboot_kernel"
+#define MODULE_DEFAULT_ALIGN (0x0) +#define MODULE_IMAGE_MIN_ALIGN MODULE_DEFAULT_ALIGN +#define MODULE_INITRD_MIN_ALIGN MODULE_DEFAULT_ALIGN +#define MODULE_OTHER_MIN_ALIGN MODULE_DEFAULT_ALIGN +#define MODULE_CUSTOM_MIN_ALIGN MODULE_DEFAULT_ALIGN
+#define MODULE_IMAGE_COMPATIBLE "multiboot,kernel\0multiboot,module" +#define MODULE_INITRD_COMPATIBLE "multiboot,ramdisk\0multiboot,module" +#define MODULE_OTHER_COMPATIBLE "multiboot,module"
+/* This maximum size is defined in Power.org ePAPR V1.1
- 2.2.1.1 Node Name Requirements
- node-name@unit-address
- 31 + 1(@) + 16(64bit address in hex format) + 1(\0) = 49
- */
+#define FDT_NODE_NAME_MAX_SIZE (49)
+struct compatible_struct +{
- grub_size_t size;
- const char *compatible;
+}; +typedef struct compatible_struct compatible_struct_t; +#define FDT_COMPATIBLE(x) {.size = sizeof(x), .compatible = x}
+enum module_type +{
- MODULE_IMAGE,
- MODULE_INITRD,
- MODULE_OTHER,
- MODULE_CUSTOM
+}; +typedef enum module_type module_type_t;
+struct fdt_node_info +{
- module_type_t type;
- const char *compatible;
- char *compatible_custom;
Why do we have compatible and compatible_custom? Can they both be set for a single node?
Because default compatibles' string have const property. we must use const char *compatible to store it.
if someone use "--type" to define a custom compatible, we need to allocate and free memory for storing the string. the pointer of this string must be stored in char *. otherwise, we will meet problem when we free them.
But we just use compatible at the end: if the compatible of this node is custom ,I will make compatible = compatible_custom after I get memory
you may have better idea, please let me know :-)
But if it is only the custom compatible strings that are dynamic, we know that these must be freed - so we don't need a separate ponter for it.
The const problem can be worked around - but if we use only one pointer, then there are fewer special tests needed, and the code becomes more readable.
- grub_size_t compatible_size;
Is compatible not a string? Can we not just get this using grub_strlen() on "compatible"??
No, we can not do that, because compatible will be "multiboot,ramdisk\0multiboot,module" something like this, So grub_strlen() just can count a part of that string.
OK, that makes sense. But the name is (to me) a little confusing, so could it be called "compat_string_size" or "compat_prop_size"?
In general, if all variables/members called compatible<something> could be called compat_string<something> or compat_prop<something>, this would make the code more readable.
OK, I will call it compat_string<something>.
DONE
+}; +typedef struct fdt_node_info *fdt_node_info_t;
+struct stub_header +{
- struct grub_arm64_linux_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;
+}; +typedef struct stub_header *stub_header_t;
Please, no typedef of pointer types (it's not even used anywhere).
Sorry, Yes, you are right, deleted
DONE
+struct multiboot_module +{
- struct multiboot_module *next;
- struct multiboot_module **prev;
- const char *name;
- grub_addr_t start;
- grub_size_t size;
- grub_size_t align;
- char *cmdline;
- int cmdline_size;
- /* for more info */
- void *extra_info;
Why do we have this "extra_info" thing? I think fdt_node_info should just be merged in here.
yes, Actually, the extra_info and fdt_node_info pair is originally for the function extension. But for now, it is useless.
DONE
Thx.
+}; +typedef struct multiboot_module *multiboot_module_t;
Please, no typedef of pointer types - it makes it very hard to see what is a pointer and what isn't.
Sorry, DONE
+void grub_arm64_linux_register_multiboot_command (grub_dl_t mod); +void grub_arm64_linux_unregister_multiboot_command (void);
+static __inline grub_addr_t +multiboot_address_align (grub_addr_t start, grub_size_t align) +{
- return (align ? (ALIGN_UP (start, align)) : start);
+}
+#endif /* ! MULTIBOOT_HEADER */
1.9.1
We're getting there - thanks for helping explain all these bits.
Great thanks for your review, that help a lot. :-)
Regards,
Leif