Re: [PATCH 4/5] arm64: Add multiboot support file

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9555 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]