[PATCH] Table code cleanup

[Myles Watson]

I didn't understand what was going on in tables.c, and it seemed a
little fragile.

These three patches try to help that out.  I would only recommend the
first two, but all three is closer to maintaining the functionality we
had.

When you're reviewing, it will be helpful to note that rom_tables_* ==
LOW_TABLES, high_tables_* == HIGH_TABLES, and low_tables==
coreboot-specific very low tables.

These patches are abuild tested and boot tested on Tyan s2892, qemu, and SimNOW.

tables.diff:
Add comments.
Remove ACPI-specific code.
Align low_table_end after gdt (so it matches high_table_end).
Correct "New low_table_end..." line in coreboot_table.c.

high_low.diff:
Factor out common code for writing tables.

both.diff:
If we need to have the tables written both times we can do something like this.

Signed-off-by: Myles Watson 
Thanks,
Myles

[Peter Stuge]

Myles Watson wrote:
> When you're reviewing, it will be helpful to note that rom_tables_* ==
> LOW_TABLES, high_tables_* == HIGH_TABLES, and low_tables==
> coreboot-specific very low tables.

Maybe they should be renamed to better show what they are.
> tables.diff:
> high_low.diff:
> both.diff:
>
> Signed-off-by: Myles Watson

Acked-by: Peter Stuge

[Myles Watson]

> Myles Watson wrote:
> > When you're reviewing, it will be helpful to note that rom_tables_* ==
> > LOW_TABLES, high_tables_* == HIGH_TABLES, and low_tables==
> > coreboot-specific very low tables.
>
> Maybe they should be renamed to better show what they are.

Yes.  I didn't rename them yet to make it clearer what I changed.  I think
the reason the names are confusing is historical.  I'm not sure why they
were called rom_tables, though.
> > tables.diff:
> > high_low.diff:
> > both.diff:
> >
> > Signed-off-by: Myles Watson

I'm hoping for a review from Stefan or Patrick because they've been using
the ACPI tables differently than anyone else.  I think I probably broke it
for them, but I'm hoping it will be an easy fix once I understand what they
wanted.
> Acked-by: Peter Stuge

Thanks.

Myles

[Bao, Zheng]

I tried tables.diff and high_low.diff. The patch was somehow rejected
and I had patched them manually. The kernel still reports ACPI table
error.

Please check it. And, it would be best for me if the patches based on
the latest version are provided.

Zheng

-----Original Message-----
From: [e-mail]
[mailto:[e-mail]] On Behalf Of Myles Watson
Sent: Thursday, May 14, 2009 6:02 AM
To: coreboot
Subject: [coreboot] [PATCH] Table code cleanup

I didn't understand what was going on in tables.c, and it seemed a
little fragile.

These three patches try to help that out.  I would only recommend the
first two, but all three is closer to maintaining the functionality we
had.

When you're reviewing, it will be helpful to note that rom_tables_* ==
LOW_TABLES, high_tables_* == HIGH_TABLES, and low_tables==
coreboot-specific very low tables.

These patches are abuild tested and boot tested on Tyan s2892, qemu, and
SimNOW.

tables.diff:
Add comments.
Remove ACPI-specific code.
Align low_table_end after gdt (so it matches high_table_end).
Correct "New low_table_end..." line in coreboot_table.c.

high_low.diff:
Factor out common code for writing tables.

both.diff:
If we need to have the tables written both times we can do something
like this.

Signed-off-by: Myles Watson 
Thanks,
Myles

[Myles Watson]

> -----Original Message-----
> From: coreboot-bounces+mylesgw=[e-mail] [mailto:coreboot-
> bounces+mylesgw=[e-mail]] On Behalf Of Bao, Zheng
> Sent: Thursday, May 14, 2009 12:14 AM
> To: coreboot
> Subject: Re: [coreboot] [PATCH] Table code cleanup
>
> I tried tables.diff and high_low.diff. The patch was somehow rejected
> and I had patched them manually. The kernel still reports ACPI table
> error.
>
> Please check it. And, it would be best for me if the patches based on
> the latest version are provided.

The patches apply to svn head for me.  Could you try "svn up" and "svn diff"
(should be no output) to make sure that you're up to date and try it again?

Thanks,
Myles

[Patrick Georgi]

Am 14.05.2009 00:01, schrieb Myles Watson:
> tables.diff:
> Add comments.
> Remove ACPI-specific code.
> Align low_table_end after gdt (so it matches high_table_end).
> Correct "New low_table_end..." line in coreboot_table.c.
>
> high_low.diff:
> Factor out common code for writing tables.
>
> both.diff:
> If we need to have the tables written both times we can do something like this.
>
> Signed-off-by: Myles Watson
> Thanks,
> Myles

+	smp_write_floating_table_physaddr(low_table_end, mpc_start);
crashes some linux versions that expect the MP table to directly follow 
the floating table, I think.

Also your change seems to create _only_ a high ACPI table if 
HAVE_HIGH_TABLES is activated, which is less than optimal because ACPI 
tables there aren't found automatically but require the payload to 
create a low mem RSDP - which the code that you removed already does.

In my opinion we should keep the current behaviour:
- high tables: create ACPI in high tables area
- low tables: create ACPI in low tables area
- high and low tables: create ACPI in high tables area and a pointer to 
the structure in low tables.

The original behaviour was to write ACPI twice. Unfortunately, we need 
ACPI and SMM to interact, and this requires the address of an ACPI 
object in the SMM handler.
With two ACPI tables, that object exists twice, and we can't know for 
sure which one is used by the OS.

Patrick

[Myles Watson]

> -----Original Message-----
> From: Patrick Georgi [mailto:[e-mail]]
> Sent: Thursday, May 14, 2009 2:00 AM
> To: Myles Watson
> Cc: coreboot
> Subject: Re: [coreboot] [PATCH] Table code cleanup
>
> Am 14.05.2009 00:01, schrieb Myles Watson:
> > tables.diff:
> > Add comments.
> > Remove ACPI-specific code.
> > Align low_table_end after gdt (so it matches high_table_end).
> > Correct "New low_table_end..." line in coreboot_table.c.
> >
> > high_low.diff:
> > Factor out common code for writing tables.
> >
> > both.diff:
> > If we need to have the tables written both times we can do something
>
> like this.
> > Signed-off-by: Myles Watson
> > Thanks,
> > Myles
> smp_write_floating_table_physaddr(low_table_end, mpc_start);
>
> crashes some linux versions that expect the MP table to directly follow
> the floating table, I think.

?  I must have misunderstood.  How can we put MP table in the high tables
area if the floating table has to be in low memory to be found, if they have
to be adjacent?
> Also your change seems to create _only_ a high ACPI table if
> HAVE_HIGH_TABLES is activated, which is less than optimal because ACPI
> tables there aren't found automatically but require the payload to
> create a low mem RSDP - which the code that you removed already does.

Yes.  I think it should go into the ACPI code.  I'll add it if you'll test.
> In my opinion we should keep the current behaviour:
> high tables: create ACPI in high tables area
> low tables: create ACPI in low tables area
> high and low tables: create ACPI in high tables area and a pointer to
>
> the structure in low tables.

I wasn't unhappy with the behavior, just the maintainability.  I think the
code flow was too complex.
> The original behaviour was to write ACPI twice. Unfortunately, we need
> ACPI and SMM to interact, and this requires the address of an ACPI
> object in the SMM handler.
> With two ACPI tables, that object exists twice, and we can't know for
> sure which one is used by the OS.

Thanks for the comments,
Myles

[Patrick Georgi]

Am 14.05.2009 16:04, schrieb Myles Watson:
> > smp_write_floating_table_physaddr(low_table_end, mpc_start);
> >
> > crashes some linux versions that expect the MP table to directly follow
> > the floating table, I think.
>
> ?  I must have misunderstood.  How can we put MP table in the high tables
> area if the floating table has to be in low memory to be found, if they have
> to be adjacent?

The high table versions are (mostly) used to allow SeaBIOS to copy them 
down. SeaBIOS copies the entire MP table into low memory precisely 
because certain linux version can't cope with a different layout.

The best outcome in my opinion is to default to
HAVE_LOW_TABLES == HAVE_HIGH_TABLES == 1. Maybe even drop the config 
flags and #ifs, unless there's a good reason to keep them
However I think we're not there yet.
> > Also your change seems to create _only_ a high ACPI table if
> > HAVE_HIGH_TABLES is activated, which is less than optimal because ACPI
> > tables there aren't found automatically but require the payload to
> > create a low mem RSDP - which the code that you removed already does.
>
> Yes.  I think it should go into the ACPI code.  I'll add it if you'll test.

I don't have a clear idea on how to approach this right now. I'll 
happily test a patch.

If you boot Linux on a board with ACPI, it's easy to see for yourself.
Early in the boot process (line 12 or so of the boot log), Linux figures 
out the ACPI tables (some example I copied from the 'net):
<6>ACPI: RSDP (v000 GBT ) @0x000f6a00
<6>ACPI: RSDT (v001 GBT AWRDACPI 0x42302e31 AWRD 0x00000000) @0x13ff3000
<6>ACPI: FADT (v001 GBT AWRDACPI 0x42302e31 AWRD 0x00000000) @0x13ff3040
<6>ACPI: DSDT (v001 GBT AWRDACPI 0x00001000 MSFT 0x0100000c) @0x00000000

As you can see here, RSDP is in low mem (@0x000f6a00), while the other 
stuff is somewhere else (@0x13ff3000) (no idea what's up with the DST here)
> I wasn't unhappy with the behavior, just the maintainability.  I think the
> code flow was too complex.

I'm not too happy with it either, and improvements are welcome - as long 
as they're not regressions, which is why I chimed in.

Patrick

[Myles Watson]

> -----Original Message-----
> From: Patrick Georgi [mailto:[e-mail]]
> Sent: Thursday, May 14, 2009 9:15 AM
> To: Myles Watson
> Cc: 'coreboot'
> Subject: Re: [coreboot] [PATCH] Table code cleanup
>
> Am 14.05.2009 16:04, schrieb Myles Watson:
> > > smp_write_floating_table_physaddr(low_table_end, mpc_start);
> > >
> > > crashes some linux versions that expect the MP table to directly follow
> > > the floating table, I think.
> >
> > ?  I must have misunderstood.  How can we put MP table in the high
>
> tables
> > area if the floating table has to be in low memory to be found, if they
>
> have
> > to be adjacent?
>
> The high table versions are (mostly) used to allow SeaBIOS to copy them
> down. SeaBIOS copies the entire MP table into low memory precisely
> because certain linux version can't cope with a different layout.
>
> The best outcome in my opinion is to default to
> HAVE_LOW_TABLES == HAVE_HIGH_TABLES == 1. Maybe even drop the config
> flags and #ifs, unless there's a good reason to keep them
> However I think we're not there yet.

Sorry to be dense.  Could we do this in terms of which tables we want in
each place?

Page0 (low_table_start):
COREBOOT TABLE

0xf0000 (rom_table):
PIRQ
MPTABLE (with floating table)
ACPI bulk

0x7fff0000 (high_tables):
PIRQ
MPTABLE
ACPI bulk

We could also do it in terms of the patch I sent:

You could tell me what things I should[n't] have included in write_table,
and what I missed.
> > > Also your change seems to create _only_ a high ACPI table if
> > > HAVE_HIGH_TABLES is activated, which is less than optimal because ACPI
> > > tables there aren't found automatically but require the payload to
> > > create a low mem RSDP - which the code that you removed already does.
> >
> > Yes.  I think it should go into the ACPI code.  I'll add it if you'll
>
> test.
> I don't have a clear idea on how to approach this right now. I'll
> happily test a patch.
>
> If you boot Linux on a board with ACPI, it's easy to see for yourself.
> Early in the boot process (line 12 or so of the boot log), Linux figures
> out the ACPI tables (some example I copied from the 'net):
> <6>ACPI: RSDP (v000 GBT ) @0x000f6a00
> <6>ACPI: RSDT (v001 GBT AWRDACPI 0x42302e31 AWRD 0x00000000) @0x13ff3000
> <6>ACPI: FADT (v001 GBT AWRDACPI 0x42302e31 AWRD 0x00000000) @0x13ff3040
> <6>ACPI: DSDT (v001 GBT AWRDACPI 0x00001000 MSFT 0x0100000c) @0x00000000
>
> As you can see here, RSDP is in low mem (@0x000f6a00), while the other
> stuff is somewhere else (@0x13ff3000) (no idea what's up with the DST
> here)

I've only been booting into Linux with SeaBIOS.
> > I wasn't unhappy with the behavior, just the maintainability.  I think
>
> the
> > code flow was too complex.
>
> I'm not too happy with it either, and improvements are welcome - as long
> as they're not regressions, which is why I chimed in.

And why I specifically asked for your input :)  Thanks for the explanations.
I'll try again.

Thanks,
Myles

[Myles Watson]

On Thu, May 14, 2009 at 9:33 AM, Myles Watson  wrote:
> > -----Original Message-----
> > From: Patrick Georgi [mailto:[e-mail]]
> > Sent: Thursday, May 14, 2009 9:15 AM
> > To: Myles Watson
> > Cc: 'coreboot'
> > Subject: Re: [coreboot] [PATCH] Table code cleanup
> >
> > Am 14.05.2009 16:04, schrieb Myles Watson:
> > > > smp_write_floating_table_physaddr(low_table_end, mpc_start);
> > > >
> > > > crashes some linux versions that expect the MP table to directly follow
> > > > the floating table, I think.
> > >
> > > ?  I must have misunderstood.  How can we put MP table in the high
> >
> > tables
> > > area if the floating table has to be in low memory to be found, if they
> >
> > have
> > > to be adjacent?
> >
> > The high table versions are (mostly) used to allow SeaBIOS to copy them
> > down. SeaBIOS copies the entire MP table into low memory precisely
> > because certain linux version can't cope with a different layout.
> >
> > The best outcome in my opinion is to default to
> > HAVE_LOW_TABLES == HAVE_HIGH_TABLES == 1. Maybe even drop the config
> > flags and #ifs, unless there's a good reason to keep them
> > However I think we're not there yet.
>
> Sorry to be dense.  Could we do this in terms of which tables we want in
> each place?
>
> Page0 (low_table_start):
> COREBOOT TABLE
>
> 0xf0000 (rom_table):
> PIRQ
> MPTABLE (with floating table)
> ACPI bulk
>
> 0x7fff0000 (high_tables):
> PIRQ
> MPTABLE
> ACPI bulk

That was less than clear :)

A problem that I see with the code right now is that it depends on the
implementation of acpi_write_tables, which is per-mainboard right now.

If someone decides to align their rsdp at 64 bytes instead of 16, it
might break.

You also end up with two rsdp structures, one in 0xf0000 and one in
high_tables.  Is that really what we want?

Thanks,
Myles

[Patrick Georgi]

Am 14.05.2009 18:46, schrieb Myles Watson:
> On Thu, May 14, 2009 at 9:33 AM, Myles Watson  wrote:
> > Sorry to be dense.  Could we do this in terms of which tables we want in
> > each place?
> >
> > Page0 (low_table_start):
> > COREBOOT TABLE
> >
> > 0xf0000 (rom_table):
> > PIRQ
> > MPTABLE (with floating table)
> > ACPI bulk
> >
> > 0x7fff0000 (high_tables):
> > PIRQ
> > MPTABLE
> > ACPI bulk
>
> That was less than clear :)

In general, I agree with that table.
The constraints are:

- MPTABLE must be two complete copies (because of Linux)

- ACPI must be one copy (because there needs to be a single address for 
some objects) with two RSDPs (see below).
> A problem that I see with the code right now is that it depends on the
> implementation of acpi_write_tables, which is per-mainboard right now.
>
> If someone decides to align their rsdp at 64 bytes instead of 16, it
> might break.

Right. I thought it works out, until the generic parts of ACPI aren't 
copied into every mainboard tree anymore.
> You also end up with two rsdp structures, one in 0xf0000 and one in
> high_tables.  Is that really what we want?

SeaBIOS overwrites the fseg with itself, then copies the RSDP in 
high_tables down into fseg again.

So the low RSDP is used if the payload doesn't overwrite it, the high 
one is used as "backup" in case the payload claims the same memory location.

I'll test your patches tonight, and look how to get stable RSDP handling 
in there.

Patrick

[Myles Watson]

> -----Original Message-----
> From: Patrick Georgi [mailto:[e-mail]]
> Sent: Thursday, May 14, 2009 11:13 AM
> To: Myles Watson
> Cc: coreboot
> Subject: Re: [coreboot] [PATCH] Table code cleanup
>
> Am 14.05.2009 18:46, schrieb Myles Watson:
> > On Thu, May 14, 2009 at 9:33 AM, Myles Watson  wrote:
> > > Sorry to be dense.  Could we do this in terms of which tables we want
>
> in
> > > each place?
> > >
> > > Page0 (low_table_start):
> > > COREBOOT TABLE
> > >
> > > 0xf0000 (rom_table):
> > > PIRQ
> > > MPTABLE (with floating table)
> > > ACPI bulk
> > >
> > > 0x7fff0000 (high_tables):
> > > PIRQ
> > > MPTABLE
> > > ACPI bulk
> >
> > That was less than clear :)
>
> In general, I agree with that table.
> The constraints are:
>
> MPTABLE must be two complete copies (because of Linux)
>
> ACPI must be one copy (because there needs to be a single address for
>
> some objects) with two RSDPs (see below).

OK.
> > A problem that I see with the code right now is that it depends on the
> > implementation of acpi_write_tables, which is per-mainboard right now.
> >
> > If someone decides to align their rsdp at 64 bytes instead of 16, it
> > might break.
>
> Right. I thought it works out, until the generic parts of ACPI aren't
> copied into every mainboard tree anymore.
> > You also end up with two rsdp structures, one in 0xf0000 and one in
> > high_tables.  Is that really what we want?
>
> SeaBIOS overwrites the fseg with itself, then copies the RSDP in
> high_tables down into fseg again.
>
> So the low RSDP is used if the payload doesn't overwrite it, the high
> one is used as "backup" in case the payload claims the same memory
> location.

In that case I think we could add an extra parameter to write_acpi_tables
and write the "backup" rsdp at that location.  I'll put that together for
Kontron and qemu.  I'm assuming that's where you'd like to test.

This is probably the time to unify write_acpi_tables, but I don't want to
muddy the waters too much.
> I'll test your patches tonight, and look how to get stable RSDP handling
> in there.

Thanks,
Myles

[Myles Watson]

> -----Original Message-----
> From: Patrick Georgi [mailto:[e-mail]]
> Sent: Thursday, May 14, 2009 11:13 AM
> To: Myles Watson
> Cc: coreboot
> Subject: Re: [coreboot] [PATCH] Table code cleanup
>
> Am 14.05.2009 18:46, schrieb Myles Watson:
> > On Thu, May 14, 2009 at 9:33 AM, Myles Watson  wrote:
> > > Sorry to be dense.  Could we do this in terms of which tables we want
>
> in
> > > each place?
> > >
> > > Page0 (low_table_start):
> > > COREBOOT TABLE
> > >
> > > 0xf0000 (rom_table):
> > > PIRQ
> > > MPTABLE (with floating table)
> > > ACPI bulk
> > >
> > > 0x7fff0000 (high_tables):
> > > PIRQ
> > > MPTABLE
> > > ACPI bulk
> >
> > That was less than clear :)
>
> In general, I agree with that table.
> The constraints are:
>
> MPTABLE must be two complete copies (because of Linux)
>
> ACPI must be one copy (because there needs to be a single address for
>
> some objects) with two RSDPs (see below).

What about gdt?  It ends up in two places now, but never in fseg.  Why do we
copy it to high_tables?

Thanks,
Myles

[Myles Watson]

On Thu, May 14, 2009 at 12:48 PM, Myles Watson  wrote:
> > -----Original Message-----
> > From: Patrick Georgi [mailto:[e-mail]]
> > Sent: Thursday, May 14, 2009 11:13 AM
> > To: Myles Watson
> > Cc: coreboot
> > Subject: Re: [coreboot] [PATCH] Table code cleanup
> >
> > Am 14.05.2009 18:46, schrieb Myles Watson:
> > > On Thu, May 14, 2009 at 9:33 AM, Myles Watson  wrote:
> > > > Sorry to be dense.  Could we do this in terms of which tables we want
> >
> > in
> > > > each place?
> > > >
> > > > Page0 (low_table_start):
> > > > COREBOOT TABLE
> > > >
> > > > 0xf0000 (rom_table):
> > > > PIRQ
> > > > MPTABLE (with floating table)
> > > > ACPI bulk
> > > >
> > > > 0x7fff0000 (high_tables):
> > > > PIRQ
> > > > MPTABLE
> > > > ACPI bulk
> > >
> > > That was less than clear :)
> >
> > In general, I agree with that table.
> > The constraints are:
> >
> > MPTABLE must be two complete copies (because of Linux)
> >
> > ACPI must be one copy (because there needs to be a single address for
> >
> > some objects) with two RSDPs (see below).
>
> What about gdt?  It ends up in two places now, but never in fseg.  Why do we
> copy it to high_tables?

OK.  Try this one :)

I added a parameter to write_acpi_tables called rsdp2.  If it is
non-zero write an rsdp there.

I also assumed that one copy of gdt is enough and put it in low_tables.

abuild-tested and boot tested on Tyan s2892 and SimNOW.

Signed-off-by: Myles Watson

Thanks,
Myles

[Bao, Zheng]

It still failed on dbm690t + filo.

Do I have to patch the high_low.diff?

Zheng

-----Original Message-----
From: [e-mail] [mailto:[e-mail]] On Behalf Of Myles Watson
Sent: Friday, May 15, 2009 4:55 AM
To: Patrick Georgi
Cc: coreboot
Subject: Re: [coreboot] [PATCH] Table code cleanup

On Thu, May 14, 2009 at 12:48 PM, Myles Watson  wrote:
> > -----Original Message-----
> > From: Patrick Georgi [mailto:[e-mail]]
> > Sent: Thursday, May 14, 2009 11:13 AM
> > To: Myles Watson
> > Cc: coreboot
> > Subject: Re: [coreboot] [PATCH] Table code cleanup
> >
> > Am 14.05.2009 18:46, schrieb Myles Watson:
> > > On Thu, May 14, 2009 at 9:33 AM, Myles Watson  wrote:
> > > > Sorry to be dense.  Could we do this in terms of which tables we want
> >
> > in
> > > > each place?
> > > >
> > > > Page0 (low_table_start):
> > > > COREBOOT TABLE
> > > >
> > > > 0xf0000 (rom_table):
> > > > PIRQ
> > > > MPTABLE (with floating table)
> > > > ACPI bulk
> > > >
> > > > 0x7fff0000 (high_tables):
> > > > PIRQ
> > > > MPTABLE
> > > > ACPI bulk
> > >
> > > That was less than clear :)
> >
> > In general, I agree with that table.
> > The constraints are:
> >
> > MPTABLE must be two complete copies (because of Linux)
> >
> > ACPI must be one copy (because there needs to be a single address for
> >
> > some objects) with two RSDPs (see below).
>
> What about gdt?  It ends up in two places now, but never in fseg.  Why do we
> copy it to high_tables?

OK.  Try this one :)

I added a parameter to write_acpi_tables called rsdp2.  If it is
non-zero write an rsdp there.

I also assumed that one copy of gdt is enough and put it in low_tables.

abuild-tested and boot tested on Tyan s2892 and SimNOW.

Signed-off-by: Myles Watson

Thanks,
Myles

[Myles Watson]

> It still failed on dbm690t + filo.
>
> Do I have to patch the high_low.diff?

No.  Just that patch.

Could you send the output?  With and without HAVE_HIGH_TABLES would be nice.

Thanks,
Myles

[Bao, Zheng]

Please see the output below.

Zheng

-----------output with HAVE_HIGH_TABLES ------------------
Writing IRQ routing tables to 0xf0000...write_pirq_routing_table done.
Wrote the mp table end at: 000f0410 - 000f0524
High Tables Base is 7fff0000.
Writing IRQ routing tables to 0x7fff0000...write_pirq_routing_table
done.
Wrote the mp table end at: 7fff0410 - 7fff0524
ACPI: Writing ACPI tables at 7fff0800...
ACPI:    * HPET
ACPI: added table 1/9 Length now 40
ACPI:    * MADT
ACPI: added table 2/9 Length now 44
ACPI:    * SSDT
processor_brand=AMD Athlon(tm) 64 X2 Dual Core Processor 3400+
Pstates Algorithm ...
Pstate_freq[0] = 1800MHz        Pstate_vid[0] = 18      Pstate_volt[0] =
1100mv Pstate_power[0]
 = 35000mw
Pstate_freq[1] = 1000MHz        Pstate_vid[1] = 22      Pstate_volt[1] =
1000mv Pstate_power[1]
 = 16069mw
ACPI: added table 3/9 Length now 48
ACPI:    * FACS
ACPI:    * DSDT
ACPI:    * DSDT @ 7fff0bee Length 27c8
ACPI:    * FADT
pm_base: 0x0800
ACPI: added table 4/9 Length now 52
ACPI: done.
Moving GDT to 0x500...ok
Multiboot Information structure has been written.
Writing high table forward entry at 0x00000800
Wrote coreboot table at: 00000800 - 00000818  checksum 470f
New low_table_end: 0x00000818
Now going to write high coreboot table at 0x7fff38d0
rom_table_end = 0x7fff38d0
Adjust low_table_end from 0x00000818 to 0x00001000
Adjust rom_table_end from 0x7fff38d0 to 0x80000000
Adding high table area
uma_memory_start=0x78000000, uma_memory_size=0x0
Wrote coreboot table at: 7fff38d0 - 7fff3ac8  checksum 4b5b

elfboot: Attempting to load payload.
rom_stream: 0xfffc0000 - 0xfffdffff
Found ELF candidate at offset 0
header_offset is 0
Try to load at offset 0x0
--------------end of output with HAVE_HIGH_TABLES-----------------

-------------- output without HAVE_HIGH_TABLES ---------------------
Writing IRQ routing tables to 0xf0000...write_pirq_routing_table done.
Wrote the mp table end at: 000f0410 - 000f0524
ACPI: Writing ACPI tables at f0800...
ACPI:    * HPET
ACPI: added table 1/9 Length now 40
ACPI:    * MADT
ACPI: added table 2/9 Length now 44
ACPI:    * SSDT
processor_brand=AMD Athlon(tm) 64 X2 Dual Core Processor 3400+
Pstates Algorithm ...
Pstate_freq[0] = 1800MHz        Pstate_vid[0] = 18      Pstate_volt[0] =
1100mv Pstate_power[0]
 = 35000mw
Pstate_freq[1] = 1000MHz        Pstate_vid[1] = 22      Pstate_volt[1] =
1000mv Pstate_power[1]
 = 16069mw
ACPI: added table 3/9 Length now 48
ACPI:    * FACS
ACPI:    * DSDT
ACPI:    * DSDT @ 000f0bee Length 27c8
ACPI:    * FADT
pm_base: 0x0800
ACPI: added table 4/9 Length now 52
ACPI: done.
Moving GDT to 0x500...ok
Multiboot Information structure has been written.
Adjust low_table_end from 0x00000800 to 0x00001000
Adjust rom_table_end from 0x000f38d0 to 0x00100000
uma_memory_start=0x78000000, uma_memory_size=0x0
Wrote coreboot table at: 00000800 - 00000a20  checksum 4307
--------------end of output without HAVE_HIGH_TABLES ------------

-----Original Message-----
From: Myles Watson [mailto:[e-mail]] 
Sent: Friday, May 15, 2009 11:08 AM
To: Bao, Zheng; 'Patrick Georgi'
Cc: 'coreboot'
Subject: RE: [coreboot] [PATCH] Table code cleanup
> It still failed on dbm690t + filo.
>
> Do I have to patch the high_low.diff?

No.  Just that patch.

Could you send the output?  With and without HAVE_HIGH_TABLES would be
nice.

Thanks,
Myles

[Myles Watson]

> -----------output with HAVE_HIGH_TABLES ------------------
> High Tables Base is 7fff0000.
> Writing IRQ routing tables to 0x7fff0000...write_pirq_routing_table

...
> rom_table_end = 0x7fff38d0
> Adjust low_table_end from 0x00000818 to 0x00001000
> Adjust rom_table_end from 0x7fff38d0 to 0x80000000
> Adding high table area
> uma_memory_start=0x78000000, uma_memory_size=0x0

Maybe the problem is the UMA memory area?  Is the size really zero?  We need
to fix this message and/or the UMA code.  It looks like ACPI and UMA
overlap.
> Wrote coreboot table at: 7fff38d0 - 7fff3ac8  checksum 4b5b
> From your earlier email:
> [    0.000000] BIOS-provided physical RAM map:
> [    0.000000]  BIOS-e820: 0000000000000000 - 00000000000a0000 (usable)
> [    0.000000]  BIOS-e820: 0000000000100000 - 0000000004000000 (usable)
> [    0.000000] end_pfn_map = 16384

I don't see any reserved regions.  Do reserved regions show up when it
works?

Thanks,
Myles

[Bao, Zheng]

1. We have disabled the display controller.
2. How is the region reserved? What can we do?

Sorry, I didn't trace the roadmap of HIGH_TABLE and have no idea about
what happened. I really need more help to solve this issue.

Joe

-----Original Message-----
From: Myles Watson [mailto:[e-mail]] 
Sent: Friday, May 15, 2009 8:40 PM
To: Bao, Zheng; 'Patrick Georgi'
Cc: 'coreboot'
Subject: RE: [coreboot] [PATCH] Table code cleanup
> -----------output with HAVE_HIGH_TABLES ------------------
> High Tables Base is 7fff0000.
> Writing IRQ routing tables to 0x7fff0000...write_pirq_routing_table

...
> rom_table_end = 0x7fff38d0
> Adjust low_table_end from 0x00000818 to 0x00001000
> Adjust rom_table_end from 0x7fff38d0 to 0x80000000
> Adding high table area
> uma_memory_start=0x78000000, uma_memory_size=0x0

Maybe the problem is the UMA memory area?  Is the size really zero?  We
need
to fix this message and/or the UMA code.  It looks like ACPI and UMA
overlap.
> Wrote coreboot table at: 7fff38d0 - 7fff3ac8  checksum 4b5b
> From your earlier email:
> [    0.000000] BIOS-provided physical RAM map:
> [    0.000000]  BIOS-e820: 0000000000000000 - 00000000000a0000

(usable)
> [    0.000000]  BIOS-e820: 0000000000100000 - 0000000004000000

(usable)
> [    0.000000] end_pfn_map = 16384

I don't see any reserved regions.  Do reserved regions show up when it
works?

Thanks,
Myles

[Myles Watson]

> -----Original Message-----
> From: Bao, Zheng [mailto:[e-mail]]
> Sent: Sunday, May 17, 2009 8:12 PM
> To: Myles Watson; Patrick Georgi
> Cc: coreboot
> Subject: RE: [coreboot] [PATCH] Table code cleanup
> We have disabled the display controller.

How did you disable it?  The message "uma_memory_start=...." shouldn't
appear if CONFIG_GFXUMA == 0.
> How is the region reserved? What can we do?

Help us understand what the desired behavior is.  I'm looking at
dbm690t/mainboard.c wondering why uma_memory_size = 0x8000000; when
CONFIG_GFXUMA == 0.  It doesn't make too much sense to hard code the address
there.
> Sorry, I didn't trace the roadmap of HIGH_TABLE and have no idea about
> what happened. I really need more help to solve this issue.

HIGH TABLES just puts the ACPI tables in higher memory.  It's not that bad,
but there are a few kinks to be worked out.

Something else I don't understand is why UMA allocation is saved until
enable.  Shouldn't it be done with resource allocation?  It doesn't make
sense to do it after the tables are written.

Thanks,
Myles

[Myles Watson]

> Something else I don't understand is why UMA allocation is saved until
> enable.  Shouldn't it be done with resource allocation?  It doesn't make
> sense to do it after the tables are written.

I was wrong here.  Have you edited some of the log messages?  I can't
find "uma_memory_start" in my tree.

Thanks,
Myles