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