[coreboot] [commit] r5286 - ...

[Myles Watson]

On Thu, Mar 25, 2010 at 10:39 AM, Stefan Reinauer <stepan at coresystems.de>wrote:

> On 3/25/10 5:18 PM, Myles Watson wrote:
> >
> >> On Thu, Mar 25, 2010 at 7:56 AM, Myles Watson <mylesgw at gmail.com>
> wrote:
> >>
> >>
> >>> It seems like others must have this problem of needing to force a
> binary
> >>> blob into a struct.
> >>>
> >> Hi, I've only been following this at a distance. I assume this all
> >> happens after memory is turned on. Why not just have a function to
> >> fill the struct given the binary blob? I'm starting to worry about all
> >> the tricks people want to play with gcc.
> >>
> > Memory is on.  The blob is in flash, and the first bit of the blob is the
> > header with the length field.
> >
> > We do a memcpy from flash to the struct,
> Actually this is a memcpy from memory to the cbmem area,

You're right, since it gets copied to memory with the rest of coreboot_ram.


> but after it's
> copied it's not treated as a struct anymore.
>
Except when you patch it?


> > but first we have to cast it to a
> > header so we can read out the length to copy.
> Which makes me think if there are not other ways to determine the size
> of an array. Maybe sizeof(AmlCode) ?
>
I tried that.  It doesn't work because it's an incomplete type.


>
> > Gcc doesn't like the cast, so
> > we just declare that we have the header stored in flash.
> >
> > One way to solve it would be to copy the header first, then read the
> length
> > and copy the rest...
> >
> >
> That's more copying than my variant with the extra pointer. Since we
> copied (or uncompressed) the dsdt from flash to ram already before, and
> are going to make another copy, I suggest rather keeping an extra 4
> bytes, than copying the complete header for no apparent reason...
>
The reason I object to the void* method was that it just masked the problem
so that gcc couldn't spot it.  Casting to void* and back to a struct seems
equivalent to just having it declared two different ways.

Copying the header first works (boot tested), and if you want to save the
redundant copy, we could start after the header for the second copy.

 Index: svn/src/mainboard/amd/serengeti_cheetah_fam10/acpi_tables.c
===================================================================
--- svn.orig/src/mainboard/amd/serengeti_cheetah_fam10/acpi_tables.c
+++ svn/src/mainboard/amd/serengeti_cheetah_fam10/acpi_tables.c
@@ -47,7 +47,7 @@ static void dump_mem(u32 start, u32 end)
 #endif

 extern const acpi_header_t AmlCode;
-extern const acpi_header_t AmlCode_ssdt;
+extern const unsigned char AmlCode_ssdt[];

 #if CONFIG_ACPI_SSDTX_NUM >= 1
 extern const acpi_header_t AmlCode_ssdt2;
@@ -264,8 +264,10 @@ unsigned long write_acpi_tables(unsigned
     current      = ( current + 0x0f) & -0x10;
     printk(BIOS_DEBUG, "ACPI:    * SSDT at %lx\n", current);
     ssdt = (acpi_header_t *)current;
-    current += AmlCode_ssdt.length;
-    memcpy((void *)ssdt, &AmlCode_ssdt, AmlCode_ssdt.length);
+    memcpy(ssdt, &AmlCode_ssdt[0], sizeof(acpi_header_t));
+    current += ssdt->length;
+    printk(BIOS_DEBUG, "ssdt->length %x\n", ssdt->length);
+    memcpy(ssdt, &AmlCode_ssdt[0], ssdt->length);
     //Here you need to set value in pci1234, sblk and sbdn in
get_bus_conf.c
     update_ssdt((void*)ssdt);
     /* recalculate checksum */

Thanks,
Myles
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20100325/995ca442/attachment.html>