[PATCH V4] multiboot2: Implement quirk-modules-after-kernel

[PATCH V4] multiboot2: Implement quirk-modules-after-kernel

[Zide Chen]

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 patch implements the quirk quirk-modules-after-kernel to load modules
to address after the kernel for Multiboot2 by reusing the implementation
for Multiboot:

- remove "ifndef GRUB_USE_MULTIBOOT2" that is handling this quirk.
- define separate variables for both Multiboot and Multiboot2, e.g.,
  grub_multiboot_quirks, highest_load, etc.
- in grub_multiboot2_load(): calculate the highest load address of raw
  image and assign it to grub_multiboot2_highest_load.
- the existing code in multiboot_elfxx.c is able to calculate the
  highest_load for Mutiboot2 already.

Currently, lowest address for Multiboot2 modules allocation was 0x0.
With this patch, the lowest module load address is set to 1MB even if this
quirk is not enabled.

Tested on KBL NUC loading ACRN Hypervisor.

v3: update commit messages
v4: don't round up lowest_addr for Multiboot2

Signed-off-by: Zide Chen <zide.chen@intel.com>
---
 grub-core/loader/i386/multiboot_mbi.c |  1 -
 grub-core/loader/multiboot.c          | 31 ++++++++++++++++++---------
 grub-core/loader/multiboot_elfxx.c    |  7 +++---
 grub-core/loader/multiboot_mbi2.c     |  2 ++
 include/grub/multiboot.h              |  1 +
 include/grub/multiboot2.h             |  8 +++++++
 6 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/grub-core/loader/i386/multiboot_mbi.c b/grub-core/loader/i386/multiboot_mbi.c
index ad3cc292fd18..18aaac847011 100644
--- a/grub-core/loader/i386/multiboot_mbi.c
+++ b/grub-core/loader/i386/multiboot_mbi.c
@@ -63,7 +63,6 @@ static int bootdev_set;
 static grub_size_t elf_sec_num, elf_sec_entsize;
 static unsigned elf_sec_shstrndx;
 static void *elf_sections;
-grub_multiboot_quirks_t grub_multiboot_quirks;
 
 static grub_err_t
 load_kernel (grub_file_t file, const char *filename,
diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
index 4a98d7082598..1227960f7bf3 100644
--- a/grub-core/loader/multiboot.c
+++ b/grub-core/loader/multiboot.c
@@ -32,6 +32,8 @@
 #include <grub/multiboot2.h>
 #define GRUB_MULTIBOOT_CONSOLE_FRAMEBUFFER GRUB_MULTIBOOT2_CONSOLE_FRAMEBUFFER
 #define GRUB_MULTIBOOT_CONSOLE_EGA_TEXT GRUB_MULTIBOOT2_CONSOLE_EGA_TEXT
+#define GRUB_MULTIBOOT_QUIRKS_NONE GRUB_MULTIBOOT2_QUIRKS_NONE
+#define GRUB_MULTIBOOT_QUIRK_MODULES_AFTER_KERNEL GRUB_MULTIBOOT2_QUIRK_MODULES_AFTER_KERNEL
 #define GRUB_MULTIBOOT(x) grub_multiboot2_ ## x
 #else
 #include <grub/multiboot.h>
@@ -210,7 +212,8 @@ grub_multiboot_unload (void)
   return GRUB_ERR_NONE;
 }
 
-static grub_uint64_t highest_load;
+GRUB_MULTIBOOT (quirks_t) GRUB_MULTIBOOT (quirks);
+grub_uint64_t GRUB_MULTIBOOT (highest_load);
 
 #define MULTIBOOT_LOAD_ELF64
 #include "multiboot_elfxx.c"
@@ -291,32 +294,32 @@ grub_cmd_multiboot (grub_command_t cmd __attribute__ ((unused)),
 
   grub_loader_unset ();
 
-  highest_load = 0;
+  GRUB_MULTIBOOT (highest_load) = 0;
 
-#ifndef GRUB_USE_MULTIBOOT2
-  grub_multiboot_quirks = GRUB_MULTIBOOT_QUIRKS_NONE;
+  GRUB_MULTIBOOT (quirks) = GRUB_MULTIBOOT_QUIRKS_NONE;
   int option_found = 0;
 
   do
     {
       option_found = 0;
+#ifndef GRUB_USE_MULTIBOOT2
       if (argc != 0 && grub_strcmp (argv[0], "--quirk-bad-kludge") == 0)
 	{
 	  argc--;
 	  argv++;
 	  option_found = 1;
-	  grub_multiboot_quirks |= GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE;
+	  GRUB_MULTIBOOT (quirks) |= GRUB_MULTIBOOT_QUIRK_BAD_KLUDGE;
 	}
+#endif
 
       if (argc != 0 && grub_strcmp (argv[0], "--quirk-modules-after-kernel") == 0)
 	{
 	  argc--;
 	  argv++;
 	  option_found = 1;
-	  grub_multiboot_quirks |= GRUB_MULTIBOOT_QUIRK_MODULES_AFTER_KERNEL;
+	  GRUB_MULTIBOOT (quirks) |= GRUB_MULTIBOOT_QUIRK_MODULES_AFTER_KERNEL;
 	}
     } while (option_found);
-#endif
 
   if (argc == 0)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
@@ -392,11 +395,19 @@ grub_cmd_module (grub_command_t cmd __attribute__ ((unused)),
   if (! file)
     return grub_errno;
 
-#ifndef GRUB_USE_MULTIBOOT2
   lowest_addr = 0x100000;
-  if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_MODULES_AFTER_KERNEL)
-    lowest_addr = ALIGN_UP (highest_load + 1048576, 4096);
+  if (GRUB_MULTIBOOT (quirks) & GRUB_MULTIBOOT_QUIRK_MODULES_AFTER_KERNEL) {
+#ifdef GRUB_USE_MULTIBOOT2
+    lowest_addr = GRUB_MULTIBOOT (highest_load);
+#else
+    /*
+     * Either Multiboot or Multiboot2, supposedly there is no requirement for this
+     * address round up. But not sure if there was any historical reason why it
+     * was here.
+     */
+    lowest_addr = ALIGN_UP (GRUB_MULTIBOOT (highest_load) + 1048576, 4096);
 #endif
+  }
 
   size = grub_file_size (file);
   if (size)
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 70cd1db513e6..8adce7b3489b 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -89,17 +89,18 @@ CONCAT(grub_multiboot_load_elf, XX) (mbi_load_data_t *mld)
     if (phdr(i)->p_type == PT_LOAD)
       {
 	mld->link_base_addr = grub_min (mld->link_base_addr, phdr(i)->p_paddr);
-	highest_load = grub_max (highest_load, phdr(i)->p_paddr + phdr(i)->p_memsz);
+	GRUB_MULTIBOOT (highest_load) = grub_max (GRUB_MULTIBOOT (highest_load),
+			phdr(i)->p_paddr + phdr(i)->p_memsz);
       }
 
 #ifdef MULTIBOOT_LOAD_ELF64
-  if (highest_load >= 0x100000000)
+  if (GRUB_MULTIBOOT(highest_load) >= 0x100000000)
     return grub_error (GRUB_ERR_BAD_OS, "segment crosses 4 GiB border");
 #endif
 
   if (mld->relocatable)
     {
-      load_size = highest_load - mld->link_base_addr;
+      load_size = GRUB_MULTIBOOT(highest_load) - mld->link_base_addr;
 
       grub_dprintf ("multiboot_loader", "align=0x%lx, preference=0x%x, "
 		    "load_size=0x%x, avoid_efi_boot_services=%d\n",
diff --git a/grub-core/loader/multiboot_mbi2.c b/grub-core/loader/multiboot_mbi2.c
index 18e766c7b31c..3883732df531 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -315,8 +315,10 @@ grub_multiboot2_load (grub_file_t file, const char *filename)
 	  grub_free (mld.buffer);
 	  return err;
 	}
+
       mld.link_base_addr = load_addr;
       mld.load_base_addr = get_physical_target_address (ch);
+      grub_multiboot2_highest_load = mld.load_base_addr + code_size;
       source = get_virtual_current_address (ch);
 
       grub_dprintf ("multiboot_loader", "link_base_addr=0x%x, load_base_addr=0x%x, "
diff --git a/include/grub/multiboot.h b/include/grub/multiboot.h
index bd0a9873e6c1..3d7c5ad29318 100644
--- a/include/grub/multiboot.h
+++ b/include/grub/multiboot.h
@@ -107,6 +107,7 @@ grub_multiboot_load_elf (mbi_load_data_t *mld);
 extern grub_size_t grub_multiboot_pure_size;
 extern grub_size_t grub_multiboot_alloc_mbi;
 extern grub_uint32_t grub_multiboot_payload_eip;
+extern grub_uint64_t grub_multiboot_highest_load;
 
 
 #endif /* ! GRUB_MULTIBOOT_HEADER */
diff --git a/include/grub/multiboot2.h b/include/grub/multiboot2.h
index 502d34ef1804..fa74508afcda 100644
--- a/include/grub/multiboot2.h
+++ b/include/grub/multiboot2.h
@@ -27,6 +27,13 @@
 #include <grub/types.h>
 #include <grub/err.h>
 
+typedef enum
+  {
+    GRUB_MULTIBOOT2_QUIRKS_NONE = 0,
+    GRUB_MULTIBOOT2_QUIRK_MODULES_AFTER_KERNEL = 1
+  } grub_multiboot2_quirks_t;
+extern grub_multiboot2_quirks_t grub_multiboot2_quirks;
+
 extern struct grub_relocator *grub_multiboot2_relocator;
 
 void grub_multiboot2 (int argc, char *argv[]);
@@ -99,6 +106,7 @@ grub_multiboot2_load_elf (mbi_load_data_t *mld);
 extern grub_size_t grub_multiboot2_pure_size;
 extern grub_size_t grub_multiboot2_alloc_mbi;
 extern grub_uint32_t grub_multiboot2_payload_eip;
+extern grub_uint64_t grub_multiboot2_highest_load;
 
 
 #endif /* ! GRUB_MULTIBOOT_HEADER */
-- 
2.17.1

Re: [PATCH V4] multiboot2: Implement quirk-modules-after-kernel

[Daniel Kiper]

Hi Zide,

On Tue, Apr 07, 2020 at 02:08:59PM -0700, Zide Chen 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 patch implements the quirk quirk-modules-after-kernel to load modules
> to address after the kernel for Multiboot2 by reusing the implementation
> for Multiboot:
>
> - remove "ifndef GRUB_USE_MULTIBOOT2" that is handling this quirk.
> - define separate variables for both Multiboot and Multiboot2, e.g.,
>   grub_multiboot_quirks, highest_load, etc.
> - in grub_multiboot2_load(): calculate the highest load address of raw
>   image and assign it to grub_multiboot2_highest_load.
> - the existing code in multiboot_elfxx.c is able to calculate the
>   highest_load for Mutiboot2 already.
>
> Currently, lowest address for Multiboot2 modules allocation was 0x0.
> With this patch, the lowest module load address is set to 1MB even if this
> quirk is not enabled.
>
> Tested on KBL NUC loading ACRN Hypervisor.

OK, I understand the need but I do not like the solution. I would prefer
if you extend Multiboot2 protocol specification [1] with an additional
tag. I think that it should allow a user to specify above/below the
kernel (just a 32-bit bitfield) and specific address in the memory
(64-bit). Specific address should have a preference. So, the new tag
should have at least two members. If you decide to go that way I would
like to see updates to the Multiboot2 specification first. If we are OK
with the proposed changes then you can go and write the code for GRUB.
Is it OK for you?

Last but not least, this change is not GRUB 2.06 release material.
However, I am happy to get it after the release.

If you have any questions just drop me a line.

Daniel

RE: [PATCH V4] multiboot2: Implement quirk-modules-after-kernel

[Chen, Zide]

Hi Daniel,


> Subject: Re: [PATCH V4] multiboot2: Implement quirk-modules-after-kernel
> On Tue, Apr 07, 2020 at 02:08:59PM -0700, Zide Chen 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 patch implements the quirk quirk-modules-after-kernel to load modules
> > to address after the kernel for Multiboot2 by reusing the implementation
> > for Multiboot:
> >
> > - remove "ifndef GRUB_USE_MULTIBOOT2" that is handling this quirk.
> > - define separate variables for both Multiboot and Multiboot2, e.g.,
> >   grub_multiboot_quirks, highest_load, etc.
> > - in grub_multiboot2_load(): calculate the highest load address of raw
> >   image and assign it to grub_multiboot2_highest_load.
> > - the existing code in multiboot_elfxx.c is able to calculate the
> >   highest_load for Mutiboot2 already.
> >
> > Currently, lowest address for Multiboot2 modules allocation was 0x0.
> > With this patch, the lowest module load address is set to 1MB even if this
> > quirk is not enabled.
> >
> > Tested on KBL NUC loading ACRN Hypervisor.
> 
> OK, I understand the need but I do not like the solution. I would prefer
> if you extend Multiboot2 protocol specification [1] with an additional
> tag. I think that it should allow a user to specify above/below the
> kernel (just a 32-bit bitfield) and specific address in the memory
> (64-bit). Specific address should have a preference. So, the new tag
> should have at least two members. If you decide to go that way I would
> like to see updates to the Multiboot2 specification first. If we are OK
> with the proposed changes then you can go and write the code for GRUB.
> Is it OK for you?

Thank you very much for the comments!

Yes, a new tag would give it more flexibility for loading modules. But my main
concern is don't know how to push the new tag to Multiboot2 spec, and not sure
will it take very long time, for example, months?

w.r.t. to the tag, how about the following draft which was almost borrowed from
the relocatable header tag? It seems this better covers the case for loading multiple
modules.

Modules relocatable tag

            +------------------+
u16     | type = 22       |
u16     | flags               |
u32     | size = 20        |
u32     | min_addr      |
u32     | max_addr     |
u32     | preference   |
            +------------------+

This tag is independent to the relocatable header tag. All of the address fields in this tag are physical addresses.

min_addr
Lowest possible physical address at which any modules should be loaded. The bootloader cannot load any part of any modules below this address.

max_addr
Highest possible physical address at which loaded any modules should end. The bootloader cannot load any part of any modules above this address.

preference
It contains load address placement suggestion for modules. Boot loader should follow it. '0' means none, '1' means load image at lowest possible address but not lower than min_addr and '2' means load image at highest possible address but not higher than max_addr.

 
> Last but not least, this change is not GRUB 2.06 release material.
> However, I am happy to get it after the release.
> 
> If you have any questions just drop me a line.
> 
> Daniel

Best Regards,
Zide

Re: [PATCH V4] multiboot2: Implement quirk-modules-after-kernel

[Daniel Kiper]

On Tue, Apr 14, 2020 at 09:39:17PM +0000, Chen, Zide wrote:
> Hi Daniel,
>
> > Subject: Re: [PATCH V4] multiboot2: Implement quirk-modules-after-kernel
> > On Tue, Apr 07, 2020 at 02:08:59PM -0700, Zide Chen 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 patch implements the quirk quirk-modules-after-kernel to load modules
> > > to address after the kernel for Multiboot2 by reusing the implementation
> > > for Multiboot:
> > >
> > > - remove "ifndef GRUB_USE_MULTIBOOT2" that is handling this quirk.
> > > - define separate variables for both Multiboot and Multiboot2, e.g.,
> > >   grub_multiboot_quirks, highest_load, etc.
> > > - in grub_multiboot2_load(): calculate the highest load address of raw
> > >   image and assign it to grub_multiboot2_highest_load.
> > > - the existing code in multiboot_elfxx.c is able to calculate the
> > >   highest_load for Mutiboot2 already.
> > >
> > > Currently, lowest address for Multiboot2 modules allocation was 0x0.
> > > With this patch, the lowest module load address is set to 1MB even if this
> > > quirk is not enabled.
> > >
> > > Tested on KBL NUC loading ACRN Hypervisor.
> >
> > OK, I understand the need but I do not like the solution. I would prefer
> > if you extend Multiboot2 protocol specification [1] with an additional
> > tag. I think that it should allow a user to specify above/below the
> > kernel (just a 32-bit bitfield) and specific address in the memory
> > (64-bit). Specific address should have a preference. So, the new tag
> > should have at least two members. If you decide to go that way I would
> > like to see updates to the Multiboot2 specification first. If we are OK
> > with the proposed changes then you can go and write the code for GRUB.
> > Is it OK for you?
>
> Thank you very much for the comments!

You are welcome!

> Yes, a new tag would give it more flexibility for loading modules. But my main
> concern is don't know how to push the new tag to Multiboot2 spec, and not sure

The spec is in the multiboot2 branch in the GRUB repository.

> will it take very long time, for example, months?

It depends mostly on you and a bit on workload of folks here...

> w.r.t. to the tag, how about the following draft which was almost borrowed from
> the relocatable header tag? It seems this better covers the case for loading multiple
> modules.
>
> Modules relocatable tag
>
>             +------------------+
> u16     | type = 22       |
> u16     | flags               |
> u32     | size = 20        |
> u32     | min_addr      |

s/u32/u64/

> u32     | max_addr     |

s/u32/u64/

> u32     | preference   |

Please put preference immediately behind the size.

>             +------------------+
>
> This tag is independent to the relocatable header tag. All of the
> address fields in this tag are physical addresses.
>
> min_addr
> Lowest possible physical address at which any modules should be
> loaded. The bootloader cannot load any part of any modules below this
> address.

OK.

> max_addr
> Highest possible physical address at which loaded any modules should
> end. The bootloader cannot load any part of any modules above this
> address.

OK.

> preference
> It contains load address placement suggestion for modules. Boot loader
> should follow it. '0' means none, '1' means load image at lowest
> possible address but not lower than min_addr and '2' means load image
> at highest possible address but not higher than max_addr.

I am OK with that. However, I would add something saying that the
min_addr and max_addr usage do not depend so strongly on preference
setting. I mean if preference is 0 then the bootloader still looks
at the min_addr and max_addr.

Hmmm... How would you want to use that tag to force the bootloader to
load the modules above/below the kernel?

Daniel

RE: [PATCH V4] multiboot2: Implement quirk-modules-after-kernel

[Chen, Zide]

> -----Original Message-----
> From: Daniel Kiper <dkiper@net-space.pl>
> Sent: Thursday, April 16, 2020 6:29 AM
> To: Chen, Zide <zide.chen@intel.com>
> Cc: grub-devel@gnu.org
> Subject: Re: [PATCH V4] multiboot2: Implement quirk-modules-after-kernel
> 
> > Yes, a new tag would give it more flexibility for loading modules. But my main
> > concern is don't know how to push the new tag to Multiboot2 spec, and not sure
> 
> The spec is in the multiboot2 branch in the GRUB repository.
> 
> > will it take very long time, for example, months?
> 
> It depends mostly on you and a bit on workload of folks here...

I see, sorry for my naïve question:  I need to send out a new patch dedicated for the new tag change and send to
grub-devel@gnu.orgfor review, at the time it's accepted, you will merge it to multiboot2 branch? Is it correct?

> > w.r.t. to the tag, how about the following draft which was almost borrowed from
> > the relocatable header tag? It seems this better covers the case for loading multiple
> > modules.
> >
> > Modules relocatable tag
> >
> >             +------------------+
> > u16     | type = 22       |
> > u16     | flags               |
> > u32     | size = 20        |
> > u32     | min_addr      |
> 
> s/u32/u64/

Currently modules are loaded at 32bit address only. But yes, I agree that 'u64' would
make it more flexible.

> > u32     | max_addr     |
> 
> s/u32/u64/
> 
> > u32     | preference   |
> 
> Please put preference immediately behind the size.

Sure, will do.

> 
> >             +------------------+
> >
> > This tag is independent to the relocatable header tag. All of the
> > address fields in this tag are physical addresses.
> >
> > min_addr
> > Lowest possible physical address at which any modules should be
> > loaded. The bootloader cannot load any part of any modules below this
> > address.
> 
> OK.
> 
> > max_addr
> > Highest possible physical address at which loaded any modules should
> > end. The bootloader cannot load any part of any modules above this
> > address.
> 
> OK.
> 
> > preference
> > It contains load address placement suggestion for modules. Boot loader
> > should follow it. '0' means none, '1' means load image at lowest
> > possible address but not lower than min_addr and '2' means load image
> > at highest possible address but not higher than max_addr.
> 
> I am OK with that. However, I would add something saying that the
> min_addr and max_addr usage do not depend so strongly on preference
> setting. I mean if preference is 0 then the bootloader still looks
> at the min_addr and max_addr.
> 

Sure, will do. 

> Hmmm... How would you want to use that tag to force the bootloader to
> load the modules above/below the kernel?
> 
> Daniel

Either in my case, or the motivation to quirk-modules-after-kernel in Multiboot, the
problem was that it's preferred that the modules not be loaded at lower address, and
whether it's above/below the kernel is not important.

However, if the user does want to the modules to be above/below kernel, it still has
options:

Case 1: kernel relocatable tag is not present. 
Kernel will be loaded at known address, either through address tag, or ELF. 
Then user can manipulate this modules relocatable tag to put modules above or below kernel.

Case 2: kernel relocatable tag is present.
Yes, depending on the memory fragmentation situation, the kernel can't guaranteed  to 
get address that is lower or higher addresses for modules. But the user may put different
min/max_addr in this tag and the kernel relocatable tag. In this case, the only issue is
the gap between kernel load address and modules load addresses could be relative large.

-Zide

RE: [PATCH V4] multiboot2: Implement quirk-modules-after-kernel

[Chen, Zide]

Hi Daniel,

> -----Original Message-----
> From: Chen, Zide
> Sent: Thursday, April 16, 2020 10:41 AM
> To: 'Daniel Kiper' <dkiper@net-space.pl>
> Cc: grub-devel@gnu.org
> Subject: RE: [PATCH V4] multiboot2: Implement quirk-modules-after-kernel
> 
> > >             +------------------+
> > > u16     | type = 22       |
> > > u16     | flags               |
> > > u32     | size = 20        |
> > > u32     | min_addr      |
> >
> > s/u32/u64/
> 
> Currently modules are loaded at 32bit address only. But yes, I agree that 'u64' would
> make it more flexible.
> 
> > > u32     | max_addr     |
> >
> > s/u32/u64/
> >
> > > u32     | preference   |
> >
> > Please put preference immediately behind the size.
> Sure, will do.

Just noticed that if we change min/max_addr to u64, and move preference right after size,
it could have struct packing issue. For example:

struct multiboot_header_tag_module_relocatable
{
  multiboot_uint16_t type;
  multiboot_uint16_t flags;
  multiboot_uint32_t size;
  multiboot_uint32_t preference; 
  multiboot_uint64_t min_addr;  // address is compiler dependent
  multiboot_uint64_t max_addr;
};

So I still propose to put preference after max_addr. I wrote the code based on this proposed new tag,
and will send out the doc change and implementation for review shortly.

-Zide