Hi Daniel,
Thank you very much for your review! Comments inline:
Best Regards,
Zide
> -----Original Message-----
> From: Daniel Kiper <dki...@net-space.pl>
> Sent: Thursday, May 7, 2020 5:54 AM
> To: Chen, Zide <zide.c...@intel.com>
> Cc: grub-devel@gnu.org
> 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 <zide.c...@intel.com>
> > ---
> > 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
