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

[Daniel Kiper]

On Thu, May 07, 2020 at 09:31:24PM +0000, Chen, Zide wrote:
> 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.

OK.

[...]

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

Then you can ignore my comment above.

Daniel