On 18.12.2014 19:12, Fu Wei wrote: > 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 multiboot2 isn't. But what you implement here is not multiboot. It's xen-specific protocol. So it should be called xen_hypervisor and xen_module. It's ok to add xen_hypervisor as an alias to multiboot on x86 > (2)Multiboot is added to "linux" module because it reuses existing code. > Not a reason. You can move the code you reuse to separate module.
> 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;
Ditto
> +
> + grub_arm64_linux_register_multiboot_command (mod);
> +
Ditto
> 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 ();
Ditto
> +static grub_dl_t linux_mod;
> +static int loaded;
> +
> +static struct multiboot_binary *multiboot_kernel = NULL;
> +static struct multiboot_binary *module_head = NULL;
> +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 compat_string_struct_t default_compat_string[] = {
> + FDT_COMPATIBLE (MODULE_IMAGE_COMPATIBLE),
> + FDT_COMPATIBLE (MODULE_INITRD_COMPATIBLE),
> + FDT_COMPATIBLE (MODULE_OTHER_COMPATIBLE)
> +};
> +
> +static grub_err_t
> +set_module_type (struct multiboot_binary *module,
> + int argc, char *argv[], int *file_name_index)
> +{
> + static module_type_t default_type = MODULE_IMAGE;
> + char *temp = NULL;
> + grub_size_t total_size = 0;
> + int num_types = 0, i;
> + char **compat_string_temp_array =
> + (char **) grub_zalloc (sizeof (char *) * argc);
> +
> + *file_name_index = 0;
> +
> + /* if there are some options we need to process. */
> + if (argc >= 3)
> + {
> + while (argc > 1)
> + {
> + if (grub_strcmp (argv[0], "--type") == 0)
> + {
Can we have separate xen_* commands for different types? It's ok for
them to call similar code paths under the hood
> + module->node_info.type = MODULE_CUSTOM;
> + ARG_SHIFT(argc, argv);
> + total_size += grub_strlen (argv[0]) + 1;
> + compat_string_temp_array[num_types++] = argv[0];
> + ARG_SHIFT(argc, argv);
> + (*file_name_index) += 2;
> + }
> + else
> + /* we can add more options process code here. */
> + break;
> + }
> + }
> +
> + /* For the default module type :
> + * The implementation is following <Multiboot on ARM Specification>:
Could this thing be renamed to "xen ARM boot protocol"?
> + if (module->node_info.type != MODULE_CUSTOM)
> + {
> + /* the module type is set by the load order */
> + module->node_info.type = default_type;
> + switch (default_type)
> + {
> + case MODULE_IMAGE:
> + default_type = MODULE_INITRD;
> + break;
> +
> + case MODULE_INITRD:
> + default_type = MODULE_OTHER;
> + break;
> +
> + case MODULE_OTHER:
> + break;
> +
> + default:
> + default_type = MODULE_IMAGE; /* error, reset the type */
> + return (grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid
> argument")));
> + }
> + }
> + else
> + {
> + /* the module type is set by "--type"(MODULE_CUSTOM) */
> + module->node_info.compat_string = temp =
> + (char *) grub_zalloc (total_size);
> + module->node_info.compat_string_size = total_size;
> + for (i = 0; num_types > 0; num_types--, i++, temp++)
> + {
> + grub_strcpy (temp, compat_string_temp_array[i]);
> + temp += grub_strlen (compat_string_temp_array[i]);
> + }
> + }
> +
Unnecessarry complexity just to have less commands. We've been down this
path with grub1, it's not good.
> + grub_free (compat_string_temp_array);
> + return (GRUB_ERR_NONE);
> +}
> +
> +static grub_err_t
> +prepare_kernel_params (void)
> +{
> + int retval;
> + int chosen_node = 0;
> +
> + multiboot_fdt = grub_linux_get_fdt ();
> + if (!multiboot_fdt)
> + return (grub_error (GRUB_ERR_BAD_OS, "failed to install/update 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 install/update FDT"));
> +
> + grub_dprintf ("multiboot_loader",
> + "Multiboot Kernel cmdline : %s @ %p size:%d\n",
> + multiboot_kernel->cmdline, multiboot_kernel->cmdline,
> + multiboot_kernel->cmdline_size);
> +
> + retval = grub_fdt_set_prop (multiboot_fdt, chosen_node, "bootargs",
> + multiboot_kernel->cmdline,
> + multiboot_kernel->cmdline_size);
> + if (retval)
> + return grub_error (GRUB_ERR_BAD_OS, "failed to install/update FDT");
> +
> + return (GRUB_ERR_NONE);
> +}
> +
> +static grub_err_t
> +prepare_module_params (struct multiboot_binary *module)
> +{
> + int retval, chosen_node = 0, module_node = 0;
> + char module_name[FDT_NODE_NAME_MAX_SIZE];
> +
> + 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 install/update FDT")));
> +
What kind of errors do you expect here? I don't see grub_snprintf
erroring here at all under any circumstances.
Please make errors more descriptive. You have same error message for at
least 5 different problems.
Is this error likely to happen to a normal user? If not, let's not load
translators with obscure technical strings. If it is likely it must way
more clearer and have TRANSLATORS comment on context.
> + 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 install/update FDT"));
> +
> + 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);
> +
Could you please encapsulate find/add/setprop to a separate function?
> + retval = grub_fdt_set_prop (multiboot_fdt, module_node, "compatible",
> + module->node_info.compat_string,
> + (grub_uint32_t)
> module->node_info.compat_string_size);
> + if (retval)
> + return (grub_error (GRUB_ERR_BAD_OS,
> + N_("failed to install/update FDT")));
> +
Style here and in many other places: no parens for return.
> +static grub_err_t
> +multiboot_binary_load (struct multiboot_binary *binary, grub_file_t file,
> + int argc, char *argv[])
> +{
> + binary->size = grub_file_size (file);
> + grub_dprintf ("multiboot_loader", "Multiboot %s file size: 0x%lx\n",
> + binary->name, binary->size);
> +
> + binary->start = (grub_addr_t) grub_efi_allocate_pages (0,
> + (BYTES_TO_PAGES
> + (binary->size +
> + binary->align)));
> + if (!binary->start)
> + return (grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory")));
> +
> + grub_dprintf ("multiboot_loader", "Multiboot %s numpages: 0x%lx\n",
> + binary->name, BYTES_TO_PAGES (binary->size + binary->align));
> +
> + if (grub_file_read (file,
> + (void *) multiboot_address_align (binary->start,
> + binary->align),
> + binary->size) < (grub_ssize_t) binary->size)
> + {
> + single_binary_unload (binary);
> + return (grub_error (GRUB_ERR_BAD_OS,
> + N_("premature end of file %s"), argv[0]));
Please don't override grub_errno if it's already set. See other loader
for example.
> --- /dev/null
> +++ b/include/grub/arm64/multiboot.h
Again, please don't call it multiboot
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
