Re: [PATCH v3] multiboot2: Implement the new module load and preferences tag

[Daniel Kiper]

On Mon, Aug 16, 2021 at 04:34:31PM +0200, Lukasz Hawrylko wrote:
> In contrast to Mulitboot, in Mulitboot2, there is currently no way to
> control where to load the modules to. This could be a problem, e.g., the
> OS loaded by Multiboot2 needs to reserve low address for some particular
> purposes and not for loading modules.
>
> This new tag gives it flexibility to control the modules load addresses,
> which is independent to whether the kernel relocatable tag is present or
> not.
>
> Signed-off-by: Zide Chen <zide.chen@intel.com>
> Signed-off-by: Lukasz Hawrylko <lukasz.hawrylko@intel.com>
>
> diff -r 7fb546da47c0 -r 94ff59f2d22c grub-core/loader/multiboot.c
> --- a/grub-core/loader/multiboot.c    Thu Jul 15 17:35:28 2021 +0200
> +++ b/grub-core/loader/multiboot.c    Mon Aug 16 12:02:52 2021 +0200
> @@ -363,12 +363,15 @@
>                int argc, char *argv[])
>  {
>    grub_file_t file = 0;
> -  grub_ssize_t size;
> +  grub_size_t size;

Why do you change this? If you need that change it requires an
explanation and probably begs for separate patch.

>    void *module = NULL;
>    grub_addr_t target;
>    grub_err_t err;
>    int nounzip = 0;
>    grub_uint64_t lowest_addr = 0;
> +  grub_uint64_t max_addr;

grub_uint64_t lowest_addr = 0, max_addr;

> +  grub_size_t align = MULTIBOOT_MOD_ALIGN;

s/grub_size_t/grub_uint32_t/?

> +  int preference = GRUB_RELOCATOR_PREFERENCE_NONE;
>
>    if (argc == 0)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
> @@ -401,11 +404,33 @@
>    size = grub_file_size (file);
>    if (size)
>    {
> +    max_addr = UP_TO_TOP32 (size);
> +
> +#ifdef GRUB_USE_MULTIBOOT2
> +    if (grub_multiboot2_mlp.relocatable)
> +      {
> +        lowest_addr = grub_multiboot2_mlp.min_addr;
> +        max_addr = grub_multiboot2_mlp.max_addr;
> +        preference = grub_multiboot2_mlp.preference;
> +
> +        if (grub_multiboot2_mlp.align)
> +          {
> +            align = grub_max(grub_multiboot2_mlp.align, align);

Missing space between grub_max and "(".

> +          }

I would drop these curly brackets.

> +        if (size > max_addr || lowest_addr > max_addr - size)
> +          {
> +            grub_file_close (file);
> +            return grub_error (GRUB_ERR_BAD_ARGUMENT, "invalid min/max 
> address and/or load size for modules");
> +          }
> +      }
> +#endif
> +
>      grub_relocator_chunk_t ch;
>      err = grub_relocator_alloc_chunk_align (GRUB_MULTIBOOT (relocator), &ch,
> -                                         lowest_addr, UP_TO_TOP32 (size),
> -                                         size, MULTIBOOT_MOD_ALIGN,
> -                                         GRUB_RELOCATOR_PREFERENCE_NONE, 1);
> +                                         lowest_addr, max_addr,
> +                                         size, align,
> +                                         preference, 1);
>      if (err)
>        {
>       grub_file_close (file);
> @@ -427,7 +452,7 @@
>        return err;
>      }
>
> -  if (size && grub_file_read (file, module, size) != size)
> +  if (size && grub_file_read (file, module, size) != (grub_ssize_t)size)

???

>      {
>        grub_file_close (file);
>        if (!grub_errno)
> diff -r 7fb546da47c0 -r 94ff59f2d22c grub-core/loader/multiboot_mbi2.c
> --- a/grub-core/loader/multiboot_mbi2.c       Thu Jul 15 17:35:28 2021 +0200
> +++ b/grub-core/loader/multiboot_mbi2.c       Mon Aug 16 12:02:52 2021 +0200
> @@ -75,6 +75,7 @@
>  static void *elf_sections;
>  static int keep_bs = 0;
>  static grub_uint32_t load_base_addr;
> +struct module_load_preferences grub_multiboot2_mlp;
>
>  void
>  grub_multiboot2_add_elfsyms (grub_size_t num, grub_size_t entsize,
> @@ -114,6 +115,7 @@
>    struct multiboot_header_tag *tag;
>    struct multiboot_header_tag_address *addr_tag = NULL;
>    struct multiboot_header_tag_relocatable *rel_tag;
> +  struct multiboot_header_tag_module_load_preferences *mod_load_tag;
>    int entry_specified = 0, efi_entry_specified = 0;
>    grub_addr_t entry = 0, efi_entry = 0;
>    grub_uint32_t console_required = 0;
> @@ -123,6 +125,7 @@
>
>    mld.mbi_ver = 2;
>    mld.relocatable = 0;
> +  grub_multiboot2_mlp.relocatable = 0;

I would prefer if you use mld for this. Of course you have to rename
members then, e.g. mld.mod_relocatable.

Daniel