[PATCH] Table cleanup continued

[Myles Watson]

I'm still worried about someone changing the alignment of the rdsp and
breaking the high tables code.

This patch moves rsdp table generation into acpi_tables and cleans up
a couple of unused variables.

Signed-off-by: Myles Watson

Thanks,
Myles

[Stefan Reinauer]

On 26.05.2009 20:14 Uhr, Myles Watson wrote:
> I'm still worried about someone changing the alignment of the rdsp and
> breaking the high tables code.

I don't understand?
> This patch moves rsdp table generation into acpi_tables and cleans up
> a couple of unused variables.
>
> Signed-off-by: Myles Watson
>
> Thanks,
> Myles

I don't think spreading out a generic piece of code to each and every
mainboard with ACPI is a terribly good idea. Can this be solved within
tables.c?

[Myles Watson]

On Tue, May 26, 2009 at 12:21 PM, Stefan Reinauer  wrote:
> On 26.05.2009 20:14 Uhr, Myles Watson wrote:
>
> I'm still worried about someone changing the alignment of the rdsp and
> breaking the high tables code.
>
> I don't understand?

Since the code in tables.c depends on the implementation of
acpi_write_tables, it will be easy to break.
> I don't think spreading out a generic piece of code to each and every
> mainboard with ACPI is a terribly good idea.

Agreed.
> Can this be solved within tables.c?

No. The best way to do it is to factor out acpi_write_tables, but I
don't have the time to do that right.  The most specific code is for
kontron and I don't have the hardware.

Thanks,
Myles

[Patrick Georgi]

Myles Watson schrieb:
> I'm still worried about someone changing the alignment of the rdsp and
> breaking the high tables code.
>
> This patch moves rsdp table generation into acpi_tables and cleans up
> a couple of unused variables.

I'd like to avoid pushing more stuff into the mainboard code. I'll start 
from your code, but try to keep the rsdp2 stuff contained, so please 
hold off with committing for a while (to avoid touching all those of 
mainboard files). Of course, if I don't follow up, go ahead - it's 
better than not doing it.

Patrick

[Myles Watson]

> I'd like to avoid pushing more stuff into the mainboard code. I'll start
> from your code, but try to keep the rsdp2 stuff contained, so please hold
> off with committing for a while (to avoid touching all those of mainboard
> files). Of course, if I don't follow up, go ahead - it's better than not
> doing it.

If you factor out the generic parts of the Kontron acpi_write_tables,
I'll factor out the other boards so we can have one function.  Then it
will make more sense to add rsdp2.

Thanks,
Myles

[Patrick Georgi]

Patrick Georgi schrieb:
> I'd like to avoid pushing more stuff into the mainboard code. I'll 
> start from your code, but try to keep the rsdp2 stuff contained, so 
> please hold off with committing for a while (to avoid touching all 
> those of mainboard files). Of course, if I don't follow up, go ahead - 
> it's better than not doing it.

Myles, how about this patch?
It looks for the RSDP, and takes the RSDT from there once it found it.

Signed-off-by: Patrick Georgi

Patrick

Index: src/arch/i386/boot/coreboot_table.c
===================================================================
--- src/arch/i386/boot/coreboot_table.c	(revision 4310)
+++ src/arch/i386/boot/coreboot_table.c	(working copy)
@@ -429,9 +429,8 @@
    		low_table_end);
    head = lb_table_init(low_table_end);
    lb_forward(head, (struct lb_header*)rom_table_end);
-	lb_table_fini(head, 0);

-	low_table_end = (unsigned long)head;
+	low_table_end = (unsigned long) lb_table_fini(head, 0);
    printk_debug("New low_table_end: 0x%08lx\n", low_table_end);
    printk_debug("Now going to write high coreboot table at 0x%08lx\n",
    		rom_table_end);
Index: src/arch/i386/boot/tables.c
===================================================================
--- src/arch/i386/boot/tables.c	(revision 4310)
+++ src/arch/i386/boot/tables.c	(working copy)
@@ -63,20 +63,15 @@
 {
    unsigned long low_table_start, low_table_end;
    unsigned long rom_table_start, rom_table_end;
-#if HAVE_MP_TABLE == 1
-	unsigned long new_low_table_end;
-#endif

/* Even if high tables are configured, some tables are copied both to
     * the low and the high area, so payloads and OSes don't need to know
     * about the high tables.
     */
-	unsigned long high_rsdp;
-	unsigned long high_table_start=0, high_table_end=0;
+	unsigned long high_table_end=0;

if (high_tables_base) {
    	printk_debug("High Tables Base is %llx.\n", high_tables_base);
-		high_table_start = high_tables_base;
    	high_table_end = high_tables_base;
    } else {
    	printk_err("ERROR: High Tables Base is not set.\n");
@@ -111,16 +106,24 @@
    /* Write ACPI tables to F segment and high tables area */
 #if HAVE_ACPI_TABLES == 1
    if (high_tables_base) {
-		unsigned long rsdt_location;
-		high_rsdp = ALIGN(high_table_end, 16);
+		unsigned long acpi_start = high_table_end;
+		rom_table_end = ALIGN(rom_table_end, 16);
    	high_table_end = write_acpi_tables(high_table_end);
+		while (acpi_start < high_table_end) {
+			if (memcmp(((acpi_rsdp_t *)acpi_start)->signature, RSDP_SIG, 8) == 0) {
+				break;
+			}
+			acpi_start++;
+		}
+		if (acpi_start != high_table_end) {
+			acpi_write_rsdp((acpi_rsdp_t *)rom_table_end, ((acpi_rsdp_t *)acpi_start)->rsdt_address);
+		} else {
+			printk_err("ERROR: Didn't find RSDP in high table.\n");
+		}
    	high_table_end = ALIGN(high_table_end, 1024);
-		rsdt_location = (unsigned long)(((acpi_rsdp_t*)high_rsdp)->rsdt_address);
-		printk_debug("high mem RSDP at %x, RSDT at %x\n", high_rsdp, rsdt_location);
-		acpi_write_rsdp((acpi_rsdp_t *)rom_table_end, (acpi_rsdt_t *)rsdt_location);
-		rom_table_end = ALIGN(ALIGN(rom_table_end, 16) + sizeof(acpi_rsdp_t), 16);
+		rom_table_end = ALIGN(rom_table_end + sizeof(acpi_rsdp_t), 16);
    } else {
    	rom_table_end = write_acpi_tables(rom_table_end);
    	rom_table_end = ALIGN(rom_table_end, 1024);
    }
 #endif
@@ -164,7 +167,11 @@
    if (high_tables_base) {
    	/* Also put a forwarder entry into 0-4K */
    	write_coreboot_table(low_table_start, low_table_end,
-				high_table_start, high_table_end);
+				high_tables_base, high_table_end);
+		if (high_table_end > high_tables_base + high_tables_size)
+			printk_err("%s: High tables didn't fit in %llx (%llx)\n",
+				   __func__, high_tables_size, high_table_end -
+				   high_tables_base);
    } else {
    	/* The coreboot table must be in 0-4K or 960K-1M */
    	write_coreboot_table(low_table_start, low_table_end,

[Myles Watson]

> Myles, how about this patch?
> It looks for the RSDP, and takes the RSDT from there once it found it.

That avoids the possible breakage.  The downside is how much casting we have
to do.

Can we agree that when we factor out acpi_write_tables this can go there
too?  Either way this is _much_ better.  Thanks.
> Signed-off-by: Patrick Georgi

Acked-by: Myles Watson

Thanks,
Myles