Re: Alignment of LinuxBIOS table structures

Stefan Reinauer  writes:
> Hi,
>
> I got an interesting report today from a customer having problems 
> with building LinuxBIOS and the payload with different compilers
>
> The problem is that different compilers handle structure alignment
> differently, ie 2.95.x and 3.x have fundamental differences here:

I agree that there is a problem with the definition of this structure.  
Everything in the table remains 32bit aligned so we are clear there.
For 64bit data types the alignment is much less clear, and struct
lb_memory_range is a bit of a problem in that it does not have a 32bit
type as padding.

To see what the problem actually amounts to I walked through my
compiler collection with a simple test to see what sizeof reported
for lb_memory_range, (without adding the packed attribute).
> #include <stdio.h>
> #include "linuxbios_tables.h"
>
> int main(int argc, char **argv)
> {
>    printf("sizeof(lb_memory_range): %d\n", sizeof(struct lb_memory_range));
>    return 0;
> }

On 32bit x86 I tested with:
gcc-2.7.2
gcc-2.95
gcc-3.0
gcc-3.2
gcc-3.3
gcc-3.4

And in each instance the result was 20.

On 64bit x86 I with I tested with
gcc-3.2
gcc-3.3
gcc-3.4

And the result was 24.

I also managed to test with gcc-3.3 with -m32 and the size was still
20.

So from what I can see with 32bit x86 code we are consistent, and we
do not have compiler version dependencies.   So the bad definition
is consistent.

Moving forward we need to remove this table entry and 
replace it with a table entry that is properly defined.
Utilities like mkelfImage can continue to support the old definition,
but it should be deprecated there.
> Since this is a table that is passed in memory, we do want it to be
> exactly as it is defined, with no extra padding of any kind to make it
> reliable information. So I consider adding __attribute__ ((packed))
> a good solution for the problem.

It is a good pragmatic solution, but actually needing __attribute__ ((packed))
is an issue.  As Ron has pointed out, not all compilers support it.
And having a definition that varies between 32bit and 64bit is a
problem anyway.
> If I get no good reasons against adding this, I will check it in
> later

What I want to do is add a replacement for struct lb_memory_range that
looks like:

struct lb_memory_range2 {
    uint64_t start;
    uint64_t size;
    uint32_t type;
#define LB_MEM_RAM       1	/* Memory anyone can use */
#define LB_MEM_RESERVED  2	/* Don't use this memory region */
#define LB_MEM_TABLE     16	/* Ram configuration tables are kept in */
    uint32_t reserved;	
};

At the same time I need to define an LB_TAB_ALIGN that I can insert
when data is only 32bit aligned and I need 64bit or better alignment
in the table.  64bit alignment is unlikely to cost much at this
point but better safe than sorry :)  And it preserves the property
that the data in the table can just be used.

Eric