Patchwork [commit] r5286 - ...

On Thu, Mar 25, 2010 at 10:39 AM, Stefan Reinauer <stepan@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@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
     /* recalculate checksum */

Thanks,
Myles

On 3/25/10 5:45 PM, Myles Watson wrote:
> 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.
I don't think it's masking the problem. It should be sufficient to let
gcc know that it can't assume anything from the pointer since it comes
from a void *. Also, it's not casting it back to a struct, because it
never was a struct. So, while it might not be beautiful, I think it's at
least not wrong.

> > 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.
It would not if we included the file

----- 8< ------ dsdt_wrapper.c -------- 8< ----------

#include "dsdt.c"

int AmlCodeSize = sizeof(AmlCode);

----- 8< ------ dsdt_wrapper.c -------- 8< ----------

and use that.

A (maybe too obvious) variant would be to create a binary dsdt instead
of a C file and pack that into CBFS. It would reduce coreboot size and
allow to copy/decompress it right to cbmem

Stefan

On Thu, Mar 25, 2010 at 11:53 AM, Stefan Reinauer <stepan@coresystems.de>wrote:

>  On 3/25/10 5:45 PM, Myles Watson wrote:
>
> 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.
>
>
> I don't think it's masking the problem. It should be sufficient to let gcc
> know that it can't assume anything from the pointer since it comes from a
> void *. Also, it's not casting it back to a struct, because it never was a
> struct. So, while it might not be beautiful, I think it's at least not
> wrong.
>
The interesting thing is that gcc won't allow the cast if you don't have a
variable for it.  That's why I thought it was masking the problem.

In other words, when I tried:
((acpi_header_t*)((void*)&AmlCode)) ->length

It still has the type-punned-pointer warning.

> > 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.
>
> It would not if we included the file
>
> ----- 8< ------ dsdt_wrapper.c -------- 8< ----------
>
> #include "dsdt.c"
>
> int AmlCodeSize = sizeof(AmlCode);
>
The boards with multiple ssdts would have to be refactored.


> A (maybe too obvious) variant would be to create a binary dsdt instead of a
> C file and pack that into CBFS. It would reduce coreboot size and allow to
> copy/decompress it right to cbmem
>
This would be fine with me.  It's quite a bit more overhead than the double
copy, though.

Thanks,
Myles

On 3/25/10 7:54 PM, Myles Watson wrote:
>
>     A (maybe too obvious) variant would be to create a binary dsdt
>     instead of a C file and pack that into CBFS. It would reduce
>     coreboot size and allow to copy/decompress it right to cbmem
>
> This would be fine with me.  It's quite a bit more overhead than the
> double copy, though.
how so?

On Thu, Mar 25, 2010 at 1:00 PM, Stefan Reinauer <stepan@coresystems.de>wrote:

>  On 3/25/10 7:54 PM, Myles Watson wrote:
>
>  A (maybe too obvious) variant would be to create a binary dsdt instead of
>> a C file and pack that into CBFS. It would reduce coreboot size and allow to
>> copy/decompress it right to cbmem
>>
> This would be fine with me.  It's quite a bit more overhead than the double
> copy, though.
>
> how so?
>
The overhead of more and longer searches through CBFS.

Thanks,
Myles