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

[Thomas Huth <thuth@redhat.com>]

On 20.11.2017 17:55, Igor Mammedov wrote:
> On Thu, 16 Nov 2017 13:17:02 +0100
> Thomas Huth <thuth@redhat.com> 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 <berrange@redhat.com>
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1724570
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  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.

Actually, the readw/l/q functions already do byte-swapping, so the data
was stored in host endian order. But Michael said that he'd prefer to
store all data from the ACPI tables in little endian mode instead -
that's why I've done the patch that way.

 Thomas