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

[Chen, Zide]

Hi Daniel,

Thank you very much for your review! Comments inline:

Best Regards,
Zide

> -----Original Message-----
> From: Daniel Kiper <address@hidden>
> Sent: Thursday, May 7, 2020 5:54 AM
> To: Chen, Zide <address@hidden>
> Cc: address@hidden
> Subject: Re: [PATCH] multiboot2: Add module relocatable tag to support 
> modules relocation
> 
> 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.

Yes, will do.

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

Yes, will do.
 
> > 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".

Sure, I can change the name. I chose this name because the contain of this new 
tag is almost identical to the
relocatable header 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.

If this new tag is not named as "module relocatable tag", then I don't think 
it's needed to do this name changing any more,
since now these two names are not confusing any more.

> 
> >  @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.

Ditto. Seems no need to change name for relocatable header tag any more.

> > +@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./

Yes, will do.
 
> > +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

Yes, will do.

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

Yes, will do.

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

Yes, will do.

> > +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.

Not really. My purpose is to load modules not in the lower address and it 
doesn't
Matter whether it's loaded before or after kernel image.

I chose quirk-modules-after-kernel just because this quirk is supported in 
multiboot,
and it seems naturally to bring it to multiboot2 as well.

> Daniel