Re: [Qemu-devel] [PATCH v2] tests/bios-tables-test: Fix endianess problems when passing data to iasl

[Igor Mammedov]

On Mon, 20 Nov 2017 22:32:29 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Mon, Nov 20, 2017 at 05:55:22PM +0100, Igor Mammedov wrote:
> > On Thu, 16 Nov 2017 13:17:02 +0100
> > Thomas Huth <address@hidden> wrote:
> >   
> > > The bios-tables-test was writing out files that we pass to iasl in
> > > with the wrong endianness in the header when running on a big endian
> > > host. So instead of storing mixed endian information in our structures,
> > > let's keep everything in little endian and byte-swap it only when we
> > > need a value in the code.
> > > 
> > > Reported-by: Daniel P. Berrange <address@hidden>
> > > Buglink: https://bugs.launchpad.net/qemu/+bug/1724570
> > > Suggested-by: Michael S. Tsirkin <address@hidden>
> > > Signed-off-by: Thomas Huth <address@hidden>
> > > ---
> > >  v2: Fixed vmgenid-test which was accidentially broken in v1
> > > 
> > >  tests/acpi-utils.h       | 27 +++++----------------------
> > >  tests/bios-tables-test.c | 42 ++++++++++++++++++++++--------------------
> > >  tests/vmgenid-test.c     | 22 ++++++++++++----------
> > >  3 files changed, 39 insertions(+), 52 deletions(-)
> > > 
> > > diff --git a/tests/acpi-utils.h b/tests/acpi-utils.h
> > > index f8d8723..d5ca5b6 100644
> > > --- a/tests/acpi-utils.h
> > > +++ b/tests/acpi-utils.h
> > > @@ -28,24 +28,9 @@ typedef struct {
> > >      bool tmp_files_retain;   /* do not delete the temp asl/aml */
> > >  } AcpiSdtTable;
> > >  
> > > -#define ACPI_READ_FIELD(field, addr)           \
> > > -    do {                                       \
> > > -        switch (sizeof(field)) {               \
> > > -        case 1:                                \
> > > -            field = readb(addr);               \
> > > -            break;                             \
> > > -        case 2:                                \
> > > -            field = readw(addr);               \
> > > -            break;                             \
> > > -        case 4:                                \
> > > -            field = readl(addr);               \
> > > -            break;                             \
> > > -        case 8:                                \
> > > -            field = readq(addr);               \
> > > -            break;                             \
> > > -        default:                               \
> > > -            g_assert(false);                   \
> > > -        }                                      \  
> > probably it's been discussed but, why not do
> >  leXX_to_cpu()
> > here, instead of making each place that access read field
> > to do leXX_to_cpu() manually.?
> > 
> > Beside of keeping access to structure in natural host order,
> > it should also be less error-prone as field users don't
> > have to worry about endianness.  
> 
> Yes, I suggested that.
> The issue is that we don't byte-swap all of the tables
> (we can't, that would require a full ACPI parser).
> So when byte-swapping we end up with a mixed host/LE
> structures.
Agreed that having mixed endianness structures is hard to deal with.
Unfortunately switching away from host endianness for some structures
doesn't make code less confused as we still have mixed
structures in use (internal vs acpi ones).

That's why I'd rather make ACPI_FOO accessors take in/return data
in host byte order so that test case won't have to worry about it
as far as LE blobs are accessed via ACPI_FOO(). It'd be a lot less
fragile.

What tables/use-cases do you have in mind where we'd need full
ACPI parser /whatever it is/?


> Keeping it all LE seems cleaner.
> 
>