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 <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.
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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
