V2 Multiboot

[Jordan Crouse]

Having a copious amount of time on my hands right now, I had occasion to 
review the coreboot v2 multiboot patch from some time back.

http://www.coreboot.org/pipermail/coreboot/2008-September/039364.html

It looks technically correct, and it compiles and works.  I did see one 
issue:

Here is the memory map from the coreboot table by way of coreinfo:

-- Memory Map --

Table: 0000000000000000 - 0000000000000fff

RAM: 0000000000001000 - 00000000000effff

Table: 00000000000f0000 - 00000000000fffff

RAM: 0000000000100000 - 0000000007ffffff

And the multiboot map, also courtesy of coreinfo:

-- Memory Map --

Reserved: 0000000000000000 - 0000000000000527

Reserved: 00000000000f0000 - 00000000000f0457

RAM: 0000000000000528 - 00000000000effff

RAM: 00000000000f0458 - 0000000007ffffff

Multiboot is being rather generous in giving back memory to the system - 
I think that the two table should match - or failing that, at least 
multiboot should round up to the nearest 4k page.  V3 has the same issue.

Other then that, I'm cool with acking this.

Jordan

[Jordan Crouse]

Jordan Crouse wrote:
> Having a copious amount of time on my hands right now, I had occasion to 
> review the coreboot v2 multiboot patch from some time back.
>
> http://www.coreboot.org/pipermail/coreboot/2008-September/039364.html
>
> It looks technically correct, and it compiles and works.  I did see one 
> issue:
>
> Here is the memory map from the coreboot table by way of coreinfo:
>
> -- Memory Map --
>       Table: 0000000000000000 - 0000000000000fff
>         RAM: 0000000000001000 - 00000000000effff
>       Table: 00000000000f0000 - 00000000000fffff
>         RAM: 0000000000100000 - 0000000007ffffff
>
> And the multiboot map, also courtesy of coreinfo:
>
> -- Memory Map --
>    Reserved: 0000000000000000 - 0000000000000527
>    Reserved: 00000000000f0000 - 00000000000f0457
>         RAM: 0000000000000528 - 00000000000effff
>         RAM: 00000000000f0458 - 0000000007ffffff
>
> Multiboot is being rather generous in giving back memory to the system - 
> I think that the two table should match - or failing that, at least 
> multiboot should round up to the nearest 4k page.  V3 has the same issue.
>
> Other then that, I'm cool with acking this.

Unless I hear otherwise,  I _am_ going to ack it with the minor 
modification of rounding up the memory regions to the nearest 4k boundary.

Jordan

[Stefan Reinauer]

Jordan Crouse wrote:
> Jordan Crouse wrote:
> > Having a copious amount of time on my hands right now, I had occasion
> > to review the coreboot v2 multiboot patch from some time back.
> >
> > http://www.coreboot.org/pipermail/coreboot/2008-September/039364.html

The patch mentioned in this URL unconditionally adds a multiboot table
and the code to handle that. Please don't do that - not everyone wants
to use multiboot.

[Jordan Crouse]

Stefan Reinauer wrote:
> Jordan Crouse wrote:
> > Jordan Crouse wrote:
> > > Having a copious amount of time on my hands right now, I had occasion
> > > to review the coreboot v2 multiboot patch from some time back.
> > >
> > > http://www.coreboot.org/pipermail/coreboot/2008-September/039364.html
>
> The patch mentioned in this URL unconditionally adds a multiboot table
> and the code to handle that. Please don't do that - not everyone wants
> to use multiboot.

Why not?   It doesn't add any appreciable size, and we have plenty of 
room in that segment for all the tables we need.

Jordan

[Stefan Reinauer]

Jordan Crouse wrote:
> Stefan Reinauer wrote:
> > Jordan Crouse wrote:
> > > Jordan Crouse wrote:
> > > > Having a copious amount of time on my hands right now, I had occasion
> > > > to review the coreboot v2 multiboot patch from some time back.
> > > >
> > > > http://www.coreboot.org/pipermail/coreboot/2008-September/039364.html
> >
> > The patch mentioned in this URL unconditionally adds a multiboot table
> > and the code to handle that. Please don't do that - not everyone wants
> > to use multiboot.
>
> Why not?   It doesn't add any appreciable size, and we have plenty of
> room in that segment for all the tables we need.

Exactly. For those we need.

Why would we add tables we don't need, given the case we explicitly
disabled the functionality in the first place?

C'mon, all I'm asking for is to put multiboot code into #if
CONFIG_MULTIBOOT guards. If the plan is, however, to force everyone into
multiboot, I suggest to drop the CONFIG_MULTIBOOT option completely.

Stefan

[Jordan Crouse]

Stefan Reinauer wrote:
> Jordan Crouse wrote:
> > Stefan Reinauer wrote:
> > > Jordan Crouse wrote:
> > > > Jordan Crouse wrote:
> > > > > Having a copious amount of time on my hands right now, I had occasion
> > > > > to review the coreboot v2 multiboot patch from some time back.
> > > > >
> > > > > http://www.coreboot.org/pipermail/coreboot/2008-September/039364.html
> > >
> > > The patch mentioned in this URL unconditionally adds a multiboot table
> > > and the code to handle that. Please don't do that - not everyone wants
> > > to use multiboot.
> >
> > Why not?   It doesn't add any appreciable size, and we have plenty of
> > room in that segment for all the tables we need.
>
> Exactly. For those we need.
>
> Why would we add tables we don't need, given the case we explicitly
> disabled the functionality in the first place?
>
> C'mon, all I'm asking for is to put multiboot code into #if
> CONFIG_MULTIBOOT guards. If the plan is, however, to force everyone into
> multiboot, I suggest to drop the CONFIG_MULTIBOOT option completely.

Okay - as long as we agree it should be enabled by default.

Jordan

[Jordan Crouse]

Stefan Reinauer wrote:
> Jordan Crouse wrote:
> > Stefan Reinauer wrote:
> > > Jordan Crouse wrote:
> > > > Jordan Crouse wrote:
> > > > > Having a copious amount of time on my hands right now, I had occasion
> > > > > to review the coreboot v2 multiboot patch from some time back.
> > > > >
> > > > > http://www.coreboot.org/pipermail/coreboot/2008-September/039364.html
> > >
> > > The patch mentioned in this URL unconditionally adds a multiboot table
> > > and the code to handle that. Please don't do that - not everyone wants
> > > to use multiboot.
> >
> > Why not?   It doesn't add any appreciable size, and we have plenty of
> > room in that segment for all the tables we need.
>
> Exactly. For those we need.
>
> Why would we add tables we don't need, given the case we explicitly
> disabled the functionality in the first place?
>
> C'mon, all I'm asking for is to put multiboot code into #if
> CONFIG_MULTIBOOT guards. If the plan is, however, to force everyone into
> multiboot, I suggest to drop the CONFIG_MULTIBOOT option completely.

Changes made.  Can I get a quick sanity check before committing?

Jordan

[Peter Stuge]

Jordan Crouse wrote:
> > C'mon, all I'm asking for is to put multiboot code into #if
> > CONFIG_MULTIBOOT guards. If the plan is, however, to force everyone
> > into multiboot, I suggest to drop the CONFIG_MULTIBOOT option
> > completely.
>
> Changes made.  Can I get a quick sanity check before committing?
>
> Signed-off-by: Robert Millan

With the small change below it's

Acked-by: Peter Stuge
> +++ src/arch/i386/boot/multiboot.c	2008-11-11 09:12:01.000000000 -0700

..
> +#if CONFIG_MULTIBOOT

.. all of file ..
> +#endif

Please move this condition to Config.lb:
> Index: src/arch/i386/boot/Config.lb
>
> --- src/arch/i386/boot/Config.lb.orig	2008-11-06 11:47:48.000000000 -0700
> +++ src/arch/i386/boot/Config.lb	2008-11-07 13:45:38.000000000 -0700
> @@ -3,6 +3,7 @@
>
> object boot.o
>  object coreboot_table.o
> +object multiboot.o
>  object tables.o
>  if HAVE_PIRQ_TABLE
>  object pirq_routing.o

So that this becomes:

object coreboot_table.o
+if CONFIG_MULTIBOOT
+object multiboot.o
+end
 object tables.o

//Peter

[Jordan Crouse]

Peter Stuge wrote:
> Jordan Crouse wrote:
> > > C'mon, all I'm asking for is to put multiboot code into #if
> > > CONFIG_MULTIBOOT guards. If the plan is, however, to force everyone
> > > into multiboot, I suggest to drop the CONFIG_MULTIBOOT option
> > > completely.
> >
> > Changes made.  Can I get a quick sanity check before committing?
> >
> > Signed-off-by: Robert Millan
>
> With the small change below it's
>
> Acked-by: Peter Stuge

r3745.

Jordan