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

[Michael S. Tsirkin]

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.

Keeping it all LE seems cleaner.


-- 
MST