Re: [PATCH] multiboot2: Add module relocatable tag to support modules relocation

[Daniel Kiper]

On Thu, Apr 16, 2020 at 03:56:08PM -0700, Zide Chen wrote:
> Also change the name from "relocatable header tag" to "kernel relocatable
> tag" to make it less confusing. These two tags are independent from each
> other.

First of all, the commit message should say what the patch does and why.
Just in a few words. You do not need to repeat everything from below.
Though it have to be clear what happens without looking at the patch content.

Additionally, may I ask you to send new patch for Multiboo2 spec with
"[MULTIBOOT2 SPEC PATCH]" subject prefix instead of normal "[PATCH]" one?

> Signed-off-by: Zide Chen <address@hidden>
> ---
>  doc/multiboot.texi | 51 +++++++++++++++++++++++++++++++++++++++++-----
>  doc/multiboot2.h   | 11 ++++++++++
>  2 files changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/doc/multiboot.texi b/doc/multiboot.texi
> index df8a0d056e76..bf0150dc86a0 100644
> --- a/doc/multiboot.texi
> +++ b/doc/multiboot.texi
> @@ -355,7 +355,8 @@ executable header.
>  * Console header tags::
>  * Module alignment tag::
>  * EFI boot services tag::
> -* Relocatable header tag::
> +* Kernel relocatable tag::
> +* Module relocatable tag::

IMO this is not "Module relocatable tag". It is rather "Module load
preferences tag".

>  @end menu
>
> @@ -691,8 +692,8 @@ u32     | size = 8          |
>  This tag indicates that payload supports starting without
>  terminating boot services.
>
> -@node Relocatable header tag
> -@subsection Relocatable header tag
> +@node Kernel relocatable tag
> +@subsection Kernel relocatable tag

I am OK with this change. However, it should be done in separate patch.

>  @example
>  @group
> @@ -708,7 +709,7 @@ u32     | preference        |
>  @end group
>  @end example
>
> -This tag indicates that image is relocatable.
> +This tag indicates that kernel image is relocatable.

Ditto. However, if you want to change this then whole description of
relocatable header tag requires more editing too.

>  The meaning of each field is as follows:
>
> @@ -730,6 +731,46 @@ Boot loader should follow it. @samp{0} means none, 
> @samp{1} means
>  load image at lowest possible address but not lower than min_addr
>  and @samp{2} means load image at highest possible address but not
>  higher than max_addr.
> +
> +@node Module relocatable tag
> +@subsection Module relocatable tag

Please take a look above...

> +@example
> +@group
> +        +-------------------+
> +u16     | type = 11         |
> +u16     | flags             |
> +u32     | size = 20         |
> +u32     | min_addr          |
> +u32     | max_addr          |
> +u32     | preference        |
> +        +-------------------+
> +@end group
> +@end example
> +
> +This tag is independent to the kernel relocatable tag. All of the
> +address fields in this tag are physical addresses.
> +
> +The meaning of each field is as follows:
> +
> +@table @code
> +@item 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.
> +
> +@item max_addr
> +Highest possible physical address at which any loaded modules should
> +end. The bootloader cannot load any part of any modules above this
> +address.
> +
> +@item preference
> +It contains load address placement suggestion for Multiboot2 modules

s/Multiboot2 modules/the boot loader./

> +Boot loader should follow it. @samp{0} means load modules not lower
> +than min_addr, and not higher than max_addr, but no preference on either

s/,//g

> +lower or higher address; @samp{1} means load modules at lowest possible

s/;/./

> +address but not lower than min_addr; @samp{2} means load modules at

s/;/./

> +highest possible address but not higher than max_addr.

After reading all threads related to this change somehow I got an
impression that you want to use this tag to ask the booloader to load
the modules above the kernel. Am I right? If this is true then we should
have another value, 4, which says: please load modules above the kernel.
...and maybe 8 for below the kernel... Or vice versa would be better.
Otherwise IMO you will be playing games with relocatable tag and this
new tag to gain what you want. This will be not nice and maybe not very
reliable.

Daniel