[RFC] [PATCH] fw/coreboot: Don't scan all of memory

[Paul Menzel]

From: Raul E Rangel 
Date: Tue, 14 Jan 2020 14:19:00 -0700

There seems to be a mismatch between the ACPI table coreboot writes, and
what SeaBIOS is scanning:

SeaBIOS:
Scanning: 0x00000000 : 0x00001000
Scanning: 0xade50000 : 0xb0000000

Coreboot:
IMD ROOT    0. affff000 00001000
IMD SMALL   1. afffe000 00001000
FSP MEMORY  2. aeffe000 01000000
CONSOLE     3. aefde000 00020000
TIME STAMP  4. aefdd000 00000910
TSEG        5. adfdd000 01000000
RAMSTAGE    6. adede000 000ff000
REFCODE     7. ade8e000 00050000
ACPI GNVS   8. ade8d000 00001000
COREBOOT    9. ade85000 00008000
ACPI       10. ade61000 00024000
TPM2 TCGLOG11. ade51000 00010000
SMBIOS     12. ade50000 00000800

BUG=none
TEST=Dali is able to boot

Change-Id: I1db339f08d0754b836f84fe87cbf1139f27270e1
Signed-off-by: Raul E Rangel 
Reviewed-on: 
https://chromium-review.googlesource.com/c/chromiumos/third_party/seabios/+/...
Reviewed-by: Martin Roth 
Commit-Queue: Martin Roth 
Tested-by: Martin Roth 
---
The real bug should be fixed here.

src/fw/coreboot.c | 14 +++++++++++++-
  1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/fw/coreboot.c b/src/fw/coreboot.c
index 7c0954b..b95c680 100644
--- a/src/fw/coreboot.c
+++ b/src/fw/coreboot.c
@@ -243,10 +243,19 @@ void coreboot_debug_putc(char c)
  static void
  scan_tables(u32 start, u32 size)
  {
+    if (size > 0x100000) {
+        dprintf(5, "Size is too large: %d\n", size);
+        size = 0x100000;
+    }
      void *p = (void*)ALIGN(start, 16);
      void *end = (void*)start + size;
-    for (; p<end; p += 16)
+    dprintf(5, "Scanning: %p : %p\n", p, end);
+    for (; p<end; p += 16) {
          copy_table(p);
+        // dprintf(5, ".");
+    }
+
+    dprintf(3, "\nDone Scanning\n");
  }

void
@@ -266,9 +275,12 @@ coreboot_platform_setup(void)
      int i, count = MEM_RANGE_COUNT(cbm);
      for (i=0; i<count; i++) {
          struct cb_memory_range *m = &cbm->map[i];
+
+        dprintf(3, "Looking at type: %x\n", m->type);
          if (m->type == CB_MEM_TABLE)
              scan_tables(m->start, m->size);
      }
+    dprintf(3, "Done relocating coreboot bios tables\n");

find_acpi_features();
  }

[Raul Rangel]

This was a hack for our own branch. I never did a proper fix because we
were only using seabios short term.

On Mon, Mar 16, 2020 at 10:44 AM Paul Menzel  wrote:
> From: Raul E Rangel 
> Date: Tue, 14 Jan 2020 14:19:00 -0700
>
> There seems to be a mismatch between the ACPI table coreboot writes, and
> what SeaBIOS is scanning:
>
> SeaBIOS:
> Scanning: 0x00000000 : 0x00001000
> Scanning: 0xade50000 : 0xb0000000
>
> Coreboot:
> IMD ROOT    0. affff000 00001000
> IMD SMALL   1. afffe000 00001000
> FSP MEMORY  2. aeffe000 01000000
> CONSOLE     3. aefde000 00020000
> TIME STAMP  4. aefdd000 00000910
> TSEG        5. adfdd000 01000000
> RAMSTAGE    6. adede000 000ff000
> REFCODE     7. ade8e000 00050000
> ACPI GNVS   8. ade8d000 00001000
> COREBOOT    9. ade85000 00008000
> ACPI       10. ade61000 00024000
> TPM2 TCGLOG11. ade51000 00010000
> SMBIOS     12. ade50000 00000800
>
> BUG=none
> TEST=Dali is able to boot
>
> Change-Id: I1db339f08d0754b836f84fe87cbf1139f27270e1
> Signed-off-by: Raul E Rangel 
> Reviewed-on:
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/seabios/+/...
> Reviewed-by
> https://chromium-review.googlesource.com/c/chromiumos/third_party/seabios/+/2001559Reviewed-by:
> Martin Roth 
> Commit-Queue: Martin Roth 
> Tested-by: Martin Roth
>
> The real bug should be fixed here.
>
> src/fw/coreboot.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/src/fw/coreboot.c b/src/fw/coreboot.c
> index 7c0954b..b95c680 100644
> --- a/src/fw/coreboot.c
> +++ b/src/fw/coreboot.c
> @@ -243,10 +243,19 @@ void coreboot_debug_putc(char c)
>   static void
>   scan_tables(u32 start, u32 size)
>   {
> if (size > 0x100000) {
>    dprintf(5, "Size is too large: %d\n", size);
>    size = 0x100000;
> }
>  void *p = (void*)ALIGN(start, 16);
>  void *end = (void*)start + size;
> for (; p<end; p += 16)
>
> dprintf(5, "Scanning: %p : %p\n", p, end);
>
> for (; p<end; p += 16) {
>      copy_table(p);
>    // dprintf(5, ".");
>
> }
>
> dprintf(3, "\nDone Scanning\n");
> }
>
> void
>
> @@ -266,9 +275,12 @@ coreboot_platform_setup(void)
>       int i, count = MEM_RANGE_COUNT(cbm);
>       for (i=0; i<count; i++) {
>           struct cb_memory_range *m = &cbm->map[i];
>    dprintf(3, "Looking at type: %x\n", m->type);
>     if (m->type == CB_MEM_TABLE)
>         scan_tables(m->start, m->size);
> }
>
> dprintf(3, "Done relocating coreboot bios tables\n");
>
> find_acpi_features();
> }

[Paul Menzel]

Dear Raul,

Am 16.03.20 um 17:48 schrieb Raul Rangel:
> This was a hack for our own branch. I never did a proper fix because we
> were only using seabios short term.

No problem. From your commit message it’s not clear, what problem this 
caused.
> TEST=Dali is able to boot

Before it didn’t boot? Or did it just take a very long time?

I am currently looking into this. Testing with coreboot for QEMU Q35 
with SeaBIOS as payload, corebooot writes the coreboot table below.

```
Writing coreboot table at 0x7ff99000
  0. 0000000000000000-0000000000000fff: CONFIGURATION TABLES
  1. 0000000000001000-000000000009ffff: RAM
  2. 00000000000c0000-000000007ff73fff: RAM
  3. 000000007ff74000-000000007ffa1fff: CONFIGURATION TABLES
  4. 000000007ffa2000-000000007ffd1fff: RAMSTAGE
  5. 000000007ffd2000-000000007fffffff: CONFIGURATION TABLES
  6. 00000000b0000000-00000000bfffffff: RESERVED
  7. 0000000100000000-000000017fffffff: RAM
```

Dumping the coreboot table from SeaBIOS, it merges regions four to six.

```
Relocating coreboot bios tables
CBMEM table has 6 items:
   0: 0000000000000000 - 0000000000000fff = 0x10 (16)
   1: 0000000000001000 - 000000000009ffff = 0x1 (1)
   2: 00000000000c0000 - 000000007ff73fff = 0x1 (1)
   3: 000000007ff74000 - 000000007fffffff = 0x10 (16)
   4: 00000000b0000000 - 00000000bfffffff = 0x2 (2)
   5: 0000000100000000 - 000000017fffffff = 0x1 (1)
Looking at type 0x10 (start: 0x0, size 4096/0x1000)
Looking at type 0x1 (start: 0x1000, size 651264/0x9f000)
Looking at type 0x1 (start: 0xc0000, size 2146123776/0x7feb4000)
Looking at type 0x10 (start: 0x7ff74000, size 573440/0x8c000)
Copying SMBIOS entry point from 0x7ff74000 to 0x000f5700
Copying ACPI RSDP from 0x7ff75000 to 0x000f56e0
Looking at type 0x2 (start: 0xb0000000, size 268435456/0x10000000)
Looking at type 0x1 (start: 0x100000000, size -2147483648/0x80000000)
```

Kind regards,

Paul

[Raul Rangel]

> Before it didn’t boot? Or did it just take a very long time?

It just took a very long time.
> I am currently looking into this. Testing with coreboot for QEMU Q35
> with SeaBIOS as payload, corebooot writes the coreboot table below.
> Writing coreboot table at 0x7ff99000
>   0. 0000000000000000-0000000000000fff: CONFIGURATION TABLES
>   1. 0000000000001000-000000000009ffff: RAM
>   2. 00000000000c0000-000000007ff73fff: RAM
>   3. 000000007ff74000-000000007ffa1fff: CONFIGURATION TABLES
>   4. 000000007ffa2000-000000007ffd1fff: RAMSTAGE
>   5. 000000007ffd2000-000000007fffffff: CONFIGURATION TABLES
>   6. 00000000b0000000-00000000bfffffff: RESERVED
>   7. 0000000100000000-000000017fffffff: RAM

Ideally we could avoid scanning and just lookup:
ACPI       10. ade61000 00024000