[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] multiboot2: Add module relocatable tag to support modules re
Re: [PATCH] multiboot2: Add module relocatable tag to support modules relocation
Mon, 11 May 2020 16:23:56 +0200
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,
> > -----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.
> > > +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
> Matter whether it's loaded before or after kernel image.
Then you can ignore my comment above.