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

[Chen, Zide]

Hi Lukasz,

> From: Lukasz Hawrylko <address@hidden>
> Sent: Tuesday, April 7, 2020 2:20 AM
> To: The development of GNU GRUB <address@hidden>
> Cc: Chen, Zide <address@hidden>
> Subject: Re: [PATCH V3] multiboot2: Implement quirk-modules-after-kernel
> 
> On Mon, 2020-04-06 at 23:30 -0700, Zide Chen wrote:
> > @@ -392,11 +395,9 @@ 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);
> > -#endif
> > +  if (GRUB_MULTIBOOT (quirks) & GRUB_MULTIBOOT_QUIRK_MODULES_AFTER_KERNEL)
> > +    lowest_addr = ALIGN_UP (GRUB_MULTIBOOT (highest_load) + 1048576, 4096);
> >
> >    size = grub_file_size (file);
> >    if (size)
> >
> 
> Hi Zide
> 
> Above modification changes another behavior. Before your patch, lowest
> address for multiboot2 modules allocation was 0x0, now, when you removed
> #ifndef, it is set to 1MB even if quirk is not enabled. I think that it
> is worth to mention than in commit message.

Sure, will update the commit message.

> To be honest, I was going to submit patch that sets lowest_addr to
> 0x1000 by default, so from my perspective your 'undocumented' change is
> good, 1MB works for my project too.

This reminds me that I may set the lowest_addr in the following way. It seems 
to me that the ALIGN_UP to 1MB + 4KB
is unnecessary since grub_relocator_alloc_chunk_align() is called with PAGE 
alignment, and the additional 1MB distance
is not needed. But I keep the Multiboot behavior unchanged because I don’t know 
the background of adding this statement
in the first place.

diff --git a/grub-core/loader/multiboot.c b/grub-core/loader/multiboot.c
index 4a98d7082598..23fd46008aca 100644
--- a/grub-core/loader/multiboot.c
+++ b/grub-core/loader/multiboot.c
@@ -392,11 +392,14 @@ 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)
+  if (grub_multiboot_quirks & GRUB_MULTIBOOT_QUIRK_MODULES_AFTER_KERNEL) {
+#ifdef GRUB_USE_MULTIBOOT2
+    lowest_addr = highest_load;
+#else
     lowest_addr = ALIGN_UP (highest_load + 1048576, 4096);
 #endif
+  }

Thanks,
Zide