[LinuxBIOS] drop struct lb_uint64?

[Stefan Reinauer]

Hi,

what exactly is this structure good for?

declared in src/include/boot/linuxbios_tables.h:

struct lb_uint64 {
        uint32_t lo;
        uint32_t hi;
};

It is used in the following files:

src/arch/i386/boot/linuxbios_table.c
  src/arch/ppc/boot/linuxbios_table.c
  src/boot/elfboot.c

since each machine that can compile LinuxBIOS, ie that has a
(cross-)gcc, does have a uint64_t we should use that one.

Stefan

Stefan Reinauer  writes:
> Hi,
>
> what exactly is this structure good for?
>
> declared in src/include/boot/linuxbios_tables.h:
>
> struct lb_uint64 {
>         uint32_t lo;
>         uint32_t hi;
> };
>
> It is used in the following files:
>
> src/arch/i386/boot/linuxbios_table.c
>   src/arch/ppc/boot/linuxbios_table.c
>   src/boot/elfboot.c
>
> since each machine that can compile LinuxBIOS, ie that has a
> (cross-)gcc, does have a uint64_t we should use that one.

No, it used to be a uint64_t and that caused problems.  For reasons of
backwards compatibility we want 32bit alignment not 64bit alignment.
x86 only requires 32bit alignment for a unit64_t but x86_64 requires
64bit alignment.

For all new instances I agree.

Eric

[Stefan Reinauer]

* Eric W. Biederman  [051112 22:13]:
> > struct lb_uint64 {
> >         uint32_t lo;
> >         uint32_t hi;
> > };
>
> No, it used to be a uint64_t and that caused problems.  For reasons of
> backwards compatibility we want 32bit alignment not 64bit alignment.
> x86 only requires 32bit alignment for a unit64_t but x86_64 requires
> 64bit alignment.

Ok alignment might be an issue. Though that is a tough assumption made
that a compiler won't happen to align the above struct to 64bit under
some circumstances.

Do you remember the problems that this caused? If so, we should probably
add a comment to the source, drop one version of the [un]pack_lb64
functions and put the other one in a common place.

The reason for my bitching is that there are so many of these things
that some of us know to be necessary whereas from an outsiders view it
might look like the left over of a long-fixed compiler bug or something.

Stefan

Stefan Reinauer  writes:
> Eric W. Biederman  [051112 22:13]:
> > > struct lb_uint64 {
> > >         uint32_t lo;
> > >         uint32_t hi;
> > > };
> >
> > No, it used to be a uint64_t and that caused problems.  For reasons of
> > backwards compatibility we want 32bit alignment not 64bit alignment.
> > x86 only requires 32bit alignment for a unit64_t but x86_64 requires
> > 64bit alignment.
>
> Ok alignment might be an issue. Though that is a tough assumption made
> that a compiler won't happen to align the above struct to 64bit under
> some circumstances.

Compilers can do a lot but it would have to be a non-ABI conformant
C compiler to request 64bit alignment for that structure.
> Do you remember the problems that this caused? 
> If so, we should probably add a comment to the source, drop one
> version of the [un]pack_lb64 functions and put the other one in a
> common place.
>
> The reason for my bitching is that there are so many of these things
> that some of us know to be necessary whereas from an outsiders view it
> might look like the left over of a long-fixed compiler bug or
> something.

I actually a little surprised you don't remember as I think you were
the one who brought it to the list.  Someone was using the old
definition which was uint64_t in a 64bit environment and they couldn't
read the table.

I don't have a problem with adding a comment and cleaning up the
implementation.  At the bug fix out was the important piece.  Looking
at this started all kinds of discussion on how we needed to structure
the linuxbios table.  With this fix we did not have to redo everything
so life continued forward.

Eric

[Stefan Reinauer]

* Eric W. Biederman  [051113 09:43]:
> I actually a little surprised you don't remember as I think you were
> the one who brought it to the list.  Someone was using the old
> definition which was uint64_t in a 64bit environment and they couldn't
> read the table.

Seems I was oblivious yesterday. You are right. Therefore I have
attached a patch to clean up lb_uint64 handling and comment it.

Looks Ok?

Stefan

[yhlu]

for alignment, We can use more generic function such as

static void int_to_stream(uint32_t val, uint8_t *dest)
{
int i;
for(i=0;i<4;i++) {
*(dest+i) = (val >> (8*i)) & 0xff;
}
}

static void int64_to_stream(uint64_t val, uint8_t *dest)
{
int i;
for(i=0;i<8;i++) {
*(dest+i) = (val >> (8*i)) & 0xff;
}
}

YH

[yhlu]

interesting, So even new gcc on x86 still default uint64_t to 32bit
alignment?

YH

On 11/12/05, Eric W. Biederman  wrote:
> Stefan Reinauer  writes:
> > Hi,
> >
> > what exactly is this structure good for?
> >
> > declared in src/include/boot/linuxbios_tables.h:
> >
> > struct lb_uint64 {
> > uint32_t lo;
> > uint32_t hi;
> > };
> >
> > It is used in the following files:
> >
> > src/arch/i386/boot/linuxbios_table.c
> > src/arch/ppc/boot/linuxbios_table.c
> > src/boot/elfboot.c
> >
> > since each machine that can compile LinuxBIOS, ie that has a
> > (cross-)gcc, does have a uint64_t we should use that one.
>
> No, it used to be a uint64_t and that caused problems. For reasons of
> backwards compatibility we want 32bit alignment not 64bit alignment.
> x86 only requires 32bit alignment for a unit64_t but x86_64 requires
> 64bit alignment.
>
> For all new instances I agree.
>
> Eric

yhlu  writes:
> interesting, So even new gcc on x86 still default uint64_t to 32bit alignment?

It's a murky area of the C ABI but I don't think the implementation has
changed.  I know gcc and Intel's C compiler were at disagreement over
that one at one point.  But I would have to read through the release
notes to see.

If gcc has changed on x86 it is even more important that the field is 
not implemented as a uint64_t.  Because unfortunately it is preceded
by a 32bit number which means that field should only be 32bit aligned.

Eric