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);                   \
-        }                                      \
+#define ACPI_READ_FIELD(field, addr)            \
+    do {                                        \
+        memread(addr, &field, sizeof(field));   \
         addr += sizeof(field);                  \
     } while (0);
 
@@ -74,16 +59,14 @@ typedef struct {
     } while (0);
 
 #define ACPI_ASSERT_CMP(actual, expected) do { \
-    uint32_t ACPI_ASSERT_CMP_le = cpu_to_le32(actual); \
     char ACPI_ASSERT_CMP_str[5] = {}; \
-    memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 4); \
+    memcpy(ACPI_ASSERT_CMP_str, &actual, 4); \
     g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
 } while (0)
 
 #define ACPI_ASSERT_CMP64(actual, expected) do { \
-    uint64_t ACPI_ASSERT_CMP_le = cpu_to_le64(actual); \
     char ACPI_ASSERT_CMP_str[9] = {}; \
-    memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 8); \
+    memcpy(ACPI_ASSERT_CMP_str, &actual, 8); \
     g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
 } while (0)
 
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 564da45..e28e0c9 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -96,17 +96,20 @@ static void test_acpi_rsdp_table(test_data *data)
 static void test_acpi_rsdt_table(test_data *data)
 {
     AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
-    uint32_t addr = data->rsdp_table.rsdt_physical_address;
+    uint32_t addr = le32_to_cpu(data->rsdp_table.rsdt_physical_address);
     uint32_t *tables;
     int tables_nr;
     uint8_t checksum;
+    uint32_t rsdt_table_length;
 
     /* read the header */
     ACPI_READ_TABLE_HEADER(rsdt_table, addr);
     ACPI_ASSERT_CMP(rsdt_table->signature, "RSDT");
 
+    rsdt_table_length = le32_to_cpu(rsdt_table->length);
+
     /* compute the table entries in rsdt */
-    tables_nr = (rsdt_table->length - sizeof(AcpiRsdtDescriptorRev1)) /
+    tables_nr = (rsdt_table_length - sizeof(AcpiRsdtDescriptorRev1)) /
                 sizeof(uint32_t);
     g_assert(tables_nr > 0);
 
@@ -114,7 +117,7 @@ static void test_acpi_rsdt_table(test_data *data)
     tables = g_new0(uint32_t, tables_nr);
     ACPI_READ_ARRAY_PTR(tables, tables_nr, addr);
 
-    checksum = acpi_calc_checksum((uint8_t *)rsdt_table, rsdt_table->length) +
+    checksum = acpi_calc_checksum((uint8_t *)rsdt_table, rsdt_table_length) +
                acpi_calc_checksum((uint8_t *)tables,
                                   tables_nr * sizeof(uint32_t));
     g_assert(!checksum);
@@ -130,7 +133,7 @@ static void test_acpi_fadt_table(test_data *data)
     uint32_t addr;
 
     /* FADT table comes first */
-    addr = data->rsdt_tables_addr[0];
+    addr = le32_to_cpu(data->rsdt_tables_addr[0]);
     ACPI_READ_TABLE_HEADER(fadt_table, addr);
 
     ACPI_READ_FIELD(fadt_table->firmware_ctrl, addr);
@@ -187,13 +190,14 @@ static void test_acpi_fadt_table(test_data *data)
     ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe1_block, addr);
 
     ACPI_ASSERT_CMP(fadt_table->signature, "FACP");
-    g_assert(!acpi_calc_checksum((uint8_t *)fadt_table, fadt_table->length));
+    g_assert(!acpi_calc_checksum((uint8_t *)fadt_table,
+                                 le32_to_cpu(fadt_table->length)));
 }
 
 static void test_acpi_facs_table(test_data *data)
 {
     AcpiFacsDescriptorRev1 *facs_table = &data->facs_table;
-    uint32_t addr = data->fadt_table.firmware_ctrl;
+    uint32_t addr = le32_to_cpu(data->fadt_table.firmware_ctrl);
 
     ACPI_READ_FIELD(facs_table->signature, addr);
     ACPI_READ_FIELD(facs_table->length, addr);
@@ -212,7 +216,8 @@ static void test_dst_table(AcpiSdtTable *sdt_table, uint32_t addr)
 
     ACPI_READ_TABLE_HEADER(&sdt_table->header, addr);
 
-    sdt_table->aml_len = sdt_table->header.length - sizeof(AcpiTableHeader);
+    sdt_table->aml_len = le32_to_cpu(sdt_table->header.length)
+                         - sizeof(AcpiTableHeader);
     sdt_table->aml = g_malloc0(sdt_table->aml_len);
     ACPI_READ_ARRAY_PTR(sdt_table->aml, sdt_table->aml_len, addr);
 
@@ -226,7 +231,7 @@ static void test_dst_table(AcpiSdtTable *sdt_table, uint32_t addr)
 static void test_acpi_dsdt_table(test_data *data)
 {
     AcpiSdtTable dsdt_table;
-    uint32_t addr = data->fadt_table.dsdt;
+    uint32_t addr = le32_to_cpu(data->fadt_table.dsdt);
 
     memset(&dsdt_table, 0, sizeof(dsdt_table));
     data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
@@ -245,9 +250,10 @@ static void test_acpi_tables(test_data *data)
 
     for (i = 0; i < tables_nr; i++) {
         AcpiSdtTable ssdt_table;
+        uint32_t addr;
 
         memset(&ssdt_table, 0, sizeof(ssdt_table));
-        uint32_t addr = data->rsdt_tables_addr[i + 1]; /* fadt is first */
+        addr = le32_to_cpu(data->rsdt_tables_addr[i + 1]); /* fadt is first */
         test_dst_table(&ssdt_table, addr);
         g_array_append_val(data->tables, ssdt_table);
     }
@@ -268,9 +274,8 @@ static void dump_aml_files(test_data *data, bool rebuild)
         g_assert(sdt->aml);
 
         if (rebuild) {
-            uint32_t signature = cpu_to_le32(sdt->header.signature);
             aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
-                                       (gchar *)&signature, ext);
+                                       (gchar *)&sdt->header.signature, ext);
             fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
                         S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
         } else {
@@ -381,7 +386,6 @@ static GArray *load_expected_aml(test_data *data)
     GArray *exp_tables = g_array_new(false, true, sizeof(AcpiSdtTable));
     for (i = 0; i < data->tables->len; ++i) {
         AcpiSdtTable exp_sdt;
-        uint32_t signature;
         gchar *aml_file = NULL;
         const char *ext = data->variant ? data->variant : "";
 
@@ -390,11 +394,9 @@ static GArray *load_expected_aml(test_data *data)
         memset(&exp_sdt, 0, sizeof(exp_sdt));
         exp_sdt.header.signature = sdt->header.signature;
 
-        signature = cpu_to_le32(sdt->header.signature);
-
 try_again:
         aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
-                                   (gchar *)&signature, ext);
+                                   (gchar *)&sdt->header.signature, ext);
         if (getenv("V")) {
             fprintf(stderr, "\nLooking for expected file '%s'\n", aml_file);
         }
@@ -571,12 +573,12 @@ static void test_smbios_structs(test_data *data)
 {
     DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
     struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
-    uint32_t addr = ep_table->structure_table_address;
+    uint32_t addr = le32_to_cpu(ep_table->structure_table_address);
     int i, len, max_len = 0;
     uint8_t type, prv, crt;
 
     /* walk the smbios tables */
-    for (i = 0; i < ep_table->number_of_structures; i++) {
+    for (i = 0; i < le16_to_cpu(ep_table->number_of_structures); i++) {
 
         /* grab type and formatted area length from struct header */
         type = readb(addr);
@@ -608,9 +610,9 @@ static void test_smbios_structs(test_data *data)
     }
 
     /* total table length and max struct size must match entry point values */
-    g_assert_cmpuint(ep_table->structure_table_length, ==,
-                     addr - ep_table->structure_table_address);
-    g_assert_cmpuint(ep_table->max_structure_size, ==, max_len);
+    g_assert_cmpuint(le16_to_cpu(ep_table->structure_table_length), ==,
+                     addr - le32_to_cpu(ep_table->structure_table_address));
+    g_assert_cmpuint(le16_to_cpu(ep_table->max_structure_size), ==, max_len);
 
     /* required struct types must all be present */
     for (i = 0; i < data->required_struct_types_len; i++) {
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index b6e7b3b..5a86b40 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -38,7 +38,7 @@ static uint32_t acpi_find_vgia(void)
     uint32_t rsdp_offset;
     uint32_t guid_offset = 0;
     AcpiRsdpDescriptor rsdp_table;
-    uint32_t rsdt;
+    uint32_t rsdt, rsdt_table_length;
     AcpiRsdtDescriptorRev1 rsdt_table;
     size_t tables_nr;
     uint32_t *tables;
@@ -56,14 +56,15 @@ static uint32_t acpi_find_vgia(void)
 
     acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
 
-    rsdt = rsdp_table.rsdt_physical_address;
+    rsdt = le32_to_cpu(rsdp_table.rsdt_physical_address);
     /* read the header */
     ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
     ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
+    rsdt_table_length = le32_to_cpu(rsdt_table.length);
 
     /* compute the table entries in rsdt */
-    g_assert_cmpint(rsdt_table.length, >, sizeof(AcpiRsdtDescriptorRev1));
-    tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
+    g_assert_cmpint(rsdt_table_length, >, sizeof(AcpiRsdtDescriptorRev1));
+    tables_nr = (rsdt_table_length - sizeof(AcpiRsdtDescriptorRev1)) /
                 sizeof(uint32_t);
 
     /* get the addresses of the tables pointed by rsdt */
@@ -71,23 +72,24 @@ static uint32_t acpi_find_vgia(void)
     ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
 
     for (i = 0; i < tables_nr; i++) {
-        ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
+        uint32_t addr = le32_to_cpu(tables[i]);
+        ACPI_READ_TABLE_HEADER(&ssdt_table, addr);
         if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
             /* the first entry in the table should be VGIA
              * That's all we need
              */
-            ACPI_READ_FIELD(vgid_table.name_op, tables[i]);
+            ACPI_READ_FIELD(vgid_table.name_op, addr);
             g_assert(vgid_table.name_op == 0x08);  /* name */
-            ACPI_READ_ARRAY(vgid_table.vgia, tables[i]);
+            ACPI_READ_ARRAY(vgid_table.vgia, addr);
             g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
-            ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
+            ACPI_READ_FIELD(vgid_table.val_op, addr);
             g_assert(vgid_table.val_op == 0x0C);  /* dword */
-            ACPI_READ_FIELD(vgid_table.vgia_val, tables[i]);
+            ACPI_READ_FIELD(vgid_table.vgia_val, addr);
             /* The GUID is written at a fixed offset into the fw_cfg file
              * in order to implement the "OVMF SDT Header probe suppressor"
              * see docs/specs/vmgenid.txt for more details
              */
-            guid_offset = vgid_table.vgia_val + VMGENID_GUID_OFFSET;
+            guid_offset = le32_to_cpu(vgid_table.vgia_val) + VMGENID_GUID_OFFSET;
             break;
         }
     }
-- 
1.8.3.1

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.


> +#define ACPI_READ_FIELD(field, addr)            \
> +    do {                                        \
> +        memread(addr, &field, sizeof(field));   \
>          addr += sizeof(field);                  \
>      } while (0);
>  
> @@ -74,16 +59,14 @@ typedef struct {
>      } while (0);
>  
>  #define ACPI_ASSERT_CMP(actual, expected) do { \
> -    uint32_t ACPI_ASSERT_CMP_le = cpu_to_le32(actual); \
>      char ACPI_ASSERT_CMP_str[5] = {}; \
> -    memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 4); \
> +    memcpy(ACPI_ASSERT_CMP_str, &actual, 4); \
>      g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
>  } while (0)
>  
>  #define ACPI_ASSERT_CMP64(actual, expected) do { \
> -    uint64_t ACPI_ASSERT_CMP_le = cpu_to_le64(actual); \
>      char ACPI_ASSERT_CMP_str[9] = {}; \
> -    memcpy(ACPI_ASSERT_CMP_str, &ACPI_ASSERT_CMP_le, 8); \
> +    memcpy(ACPI_ASSERT_CMP_str, &actual, 8); \
>      g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
>  } while (0)
>  
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 564da45..e28e0c9 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -96,17 +96,20 @@ static void test_acpi_rsdp_table(test_data *data)
>  static void test_acpi_rsdt_table(test_data *data)
>  {
>      AcpiRsdtDescriptorRev1 *rsdt_table = &data->rsdt_table;
> -    uint32_t addr = data->rsdp_table.rsdt_physical_address;
> +    uint32_t addr = le32_to_cpu(data->rsdp_table.rsdt_physical_address);
>      uint32_t *tables;
>      int tables_nr;
>      uint8_t checksum;
> +    uint32_t rsdt_table_length;
>  
>      /* read the header */
>      ACPI_READ_TABLE_HEADER(rsdt_table, addr);
>      ACPI_ASSERT_CMP(rsdt_table->signature, "RSDT");
>  
> +    rsdt_table_length = le32_to_cpu(rsdt_table->length);
> +
>      /* compute the table entries in rsdt */
> -    tables_nr = (rsdt_table->length - sizeof(AcpiRsdtDescriptorRev1)) /
> +    tables_nr = (rsdt_table_length - sizeof(AcpiRsdtDescriptorRev1)) /
>                  sizeof(uint32_t);
>      g_assert(tables_nr > 0);
>  
> @@ -114,7 +117,7 @@ static void test_acpi_rsdt_table(test_data *data)
>      tables = g_new0(uint32_t, tables_nr);
>      ACPI_READ_ARRAY_PTR(tables, tables_nr, addr);
>  
> -    checksum = acpi_calc_checksum((uint8_t *)rsdt_table, rsdt_table->length) +
> +    checksum = acpi_calc_checksum((uint8_t *)rsdt_table, rsdt_table_length) +
>                 acpi_calc_checksum((uint8_t *)tables,
>                                    tables_nr * sizeof(uint32_t));
>      g_assert(!checksum);
> @@ -130,7 +133,7 @@ static void test_acpi_fadt_table(test_data *data)
>      uint32_t addr;
>  
>      /* FADT table comes first */
> -    addr = data->rsdt_tables_addr[0];
> +    addr = le32_to_cpu(data->rsdt_tables_addr[0]);
>      ACPI_READ_TABLE_HEADER(fadt_table, addr);
>  
>      ACPI_READ_FIELD(fadt_table->firmware_ctrl, addr);
> @@ -187,13 +190,14 @@ static void test_acpi_fadt_table(test_data *data)
>      ACPI_READ_GENERIC_ADDRESS(fadt_table->xgpe1_block, addr);
>  
>      ACPI_ASSERT_CMP(fadt_table->signature, "FACP");
> -    g_assert(!acpi_calc_checksum((uint8_t *)fadt_table, fadt_table->length));
> +    g_assert(!acpi_calc_checksum((uint8_t *)fadt_table,
> +                                 le32_to_cpu(fadt_table->length)));
>  }
>  
>  static void test_acpi_facs_table(test_data *data)
>  {
>      AcpiFacsDescriptorRev1 *facs_table = &data->facs_table;
> -    uint32_t addr = data->fadt_table.firmware_ctrl;
> +    uint32_t addr = le32_to_cpu(data->fadt_table.firmware_ctrl);
>  
>      ACPI_READ_FIELD(facs_table->signature, addr);
>      ACPI_READ_FIELD(facs_table->length, addr);
> @@ -212,7 +216,8 @@ static void test_dst_table(AcpiSdtTable *sdt_table, uint32_t addr)
>  
>      ACPI_READ_TABLE_HEADER(&sdt_table->header, addr);
>  
> -    sdt_table->aml_len = sdt_table->header.length - sizeof(AcpiTableHeader);
> +    sdt_table->aml_len = le32_to_cpu(sdt_table->header.length)
> +                         - sizeof(AcpiTableHeader);
>      sdt_table->aml = g_malloc0(sdt_table->aml_len);
>      ACPI_READ_ARRAY_PTR(sdt_table->aml, sdt_table->aml_len, addr);
>  
> @@ -226,7 +231,7 @@ static void test_dst_table(AcpiSdtTable *sdt_table, uint32_t addr)
>  static void test_acpi_dsdt_table(test_data *data)
>  {
>      AcpiSdtTable dsdt_table;
> -    uint32_t addr = data->fadt_table.dsdt;
> +    uint32_t addr = le32_to_cpu(data->fadt_table.dsdt);
>  
>      memset(&dsdt_table, 0, sizeof(dsdt_table));
>      data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
> @@ -245,9 +250,10 @@ static void test_acpi_tables(test_data *data)
>  
>      for (i = 0; i < tables_nr; i++) {
>          AcpiSdtTable ssdt_table;
> +        uint32_t addr;
>  
>          memset(&ssdt_table, 0, sizeof(ssdt_table));
> -        uint32_t addr = data->rsdt_tables_addr[i + 1]; /* fadt is first */
> +        addr = le32_to_cpu(data->rsdt_tables_addr[i + 1]); /* fadt is first */
>          test_dst_table(&ssdt_table, addr);
>          g_array_append_val(data->tables, ssdt_table);
>      }
> @@ -268,9 +274,8 @@ static void dump_aml_files(test_data *data, bool rebuild)
>          g_assert(sdt->aml);
>  
>          if (rebuild) {
> -            uint32_t signature = cpu_to_le32(sdt->header.signature);
>              aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
> -                                       (gchar *)&signature, ext);
> +                                       (gchar *)&sdt->header.signature, ext);
>              fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
>                          S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
>          } else {
> @@ -381,7 +386,6 @@ static GArray *load_expected_aml(test_data *data)
>      GArray *exp_tables = g_array_new(false, true, sizeof(AcpiSdtTable));
>      for (i = 0; i < data->tables->len; ++i) {
>          AcpiSdtTable exp_sdt;
> -        uint32_t signature;
>          gchar *aml_file = NULL;
>          const char *ext = data->variant ? data->variant : "";
>  
> @@ -390,11 +394,9 @@ static GArray *load_expected_aml(test_data *data)
>          memset(&exp_sdt, 0, sizeof(exp_sdt));
>          exp_sdt.header.signature = sdt->header.signature;
>  
> -        signature = cpu_to_le32(sdt->header.signature);
> -
>  try_again:
>          aml_file = g_strdup_printf("%s/%s/%.4s%s", data_dir, data->machine,
> -                                   (gchar *)&signature, ext);
> +                                   (gchar *)&sdt->header.signature, ext);
>          if (getenv("V")) {
>              fprintf(stderr, "\nLooking for expected file '%s'\n", aml_file);
>          }
> @@ -571,12 +573,12 @@ static void test_smbios_structs(test_data *data)
>  {
>      DECLARE_BITMAP(struct_bitmap, SMBIOS_MAX_TYPE+1) = { 0 };
>      struct smbios_21_entry_point *ep_table = &data->smbios_ep_table;
> -    uint32_t addr = ep_table->structure_table_address;
> +    uint32_t addr = le32_to_cpu(ep_table->structure_table_address);
>      int i, len, max_len = 0;
>      uint8_t type, prv, crt;
>  
>      /* walk the smbios tables */
> -    for (i = 0; i < ep_table->number_of_structures; i++) {
> +    for (i = 0; i < le16_to_cpu(ep_table->number_of_structures); i++) {
>  
>          /* grab type and formatted area length from struct header */
>          type = readb(addr);
> @@ -608,9 +610,9 @@ static void test_smbios_structs(test_data *data)
>      }
>  
>      /* total table length and max struct size must match entry point values */
> -    g_assert_cmpuint(ep_table->structure_table_length, ==,
> -                     addr - ep_table->structure_table_address);
> -    g_assert_cmpuint(ep_table->max_structure_size, ==, max_len);
> +    g_assert_cmpuint(le16_to_cpu(ep_table->structure_table_length), ==,
> +                     addr - le32_to_cpu(ep_table->structure_table_address));
> +    g_assert_cmpuint(le16_to_cpu(ep_table->max_structure_size), ==, max_len);
>  
>      /* required struct types must all be present */
>      for (i = 0; i < data->required_struct_types_len; i++) {
> diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
> index b6e7b3b..5a86b40 100644
> --- a/tests/vmgenid-test.c
> +++ b/tests/vmgenid-test.c
> @@ -38,7 +38,7 @@ static uint32_t acpi_find_vgia(void)
>      uint32_t rsdp_offset;
>      uint32_t guid_offset = 0;
>      AcpiRsdpDescriptor rsdp_table;
> -    uint32_t rsdt;
> +    uint32_t rsdt, rsdt_table_length;
>      AcpiRsdtDescriptorRev1 rsdt_table;
>      size_t tables_nr;
>      uint32_t *tables;
> @@ -56,14 +56,15 @@ static uint32_t acpi_find_vgia(void)
>  
>      acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
>  
> -    rsdt = rsdp_table.rsdt_physical_address;
> +    rsdt = le32_to_cpu(rsdp_table.rsdt_physical_address);
>      /* read the header */
>      ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
>      ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
> +    rsdt_table_length = le32_to_cpu(rsdt_table.length);
>  
>      /* compute the table entries in rsdt */
> -    g_assert_cmpint(rsdt_table.length, >, sizeof(AcpiRsdtDescriptorRev1));
> -    tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
> +    g_assert_cmpint(rsdt_table_length, >, sizeof(AcpiRsdtDescriptorRev1));
> +    tables_nr = (rsdt_table_length - sizeof(AcpiRsdtDescriptorRev1)) /
>                  sizeof(uint32_t);
>  
>      /* get the addresses of the tables pointed by rsdt */
> @@ -71,23 +72,24 @@ static uint32_t acpi_find_vgia(void)
>      ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
>  
>      for (i = 0; i < tables_nr; i++) {
> -        ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
> +        uint32_t addr = le32_to_cpu(tables[i]);
> +        ACPI_READ_TABLE_HEADER(&ssdt_table, addr);
>          if (!strncmp((char *)ssdt_table.oem_table_id, "VMGENID", 7)) {
>              /* the first entry in the table should be VGIA
>               * That's all we need
>               */
> -            ACPI_READ_FIELD(vgid_table.name_op, tables[i]);
> +            ACPI_READ_FIELD(vgid_table.name_op, addr);
>              g_assert(vgid_table.name_op == 0x08);  /* name */
> -            ACPI_READ_ARRAY(vgid_table.vgia, tables[i]);
> +            ACPI_READ_ARRAY(vgid_table.vgia, addr);
>              g_assert(memcmp(vgid_table.vgia, "VGIA", 4) == 0);
> -            ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
> +            ACPI_READ_FIELD(vgid_table.val_op, addr);
>              g_assert(vgid_table.val_op == 0x0C);  /* dword */
> -            ACPI_READ_FIELD(vgid_table.vgia_val, tables[i]);
> +            ACPI_READ_FIELD(vgid_table.vgia_val, addr);
>              /* The GUID is written at a fixed offset into the fw_cfg file
>               * in order to implement the "OVMF SDT Header probe suppressor"
>               * see docs/specs/vmgenid.txt for more details
>               */
> -            guid_offset = vgid_table.vgia_val + VMGENID_GUID_OFFSET;
> +            guid_offset = le32_to_cpu(vgid_table.vgia_val) + VMGENID_GUID_OFFSET;
>              break;
>          }
>      }

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

On Mon, Nov 20, 2017 at 05:55:22PM +0100, 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.

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

On Mon, 20 Nov 2017 22:32:29 +0200
"Michael S. Tsirkin" <mst@redhat.com> 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 <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.  
> 
> 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.
> 
> 

On Tue, Nov 21, 2017 at 02:26:39PM +0100, Igor Mammedov wrote:
> On Mon, 20 Nov 2017 22:32:29 +0200
> "Michael S. Tsirkin" <mst@redhat.com> 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 <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.  
> > 
> > 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).

We can't change ACPI so internal ones should switch to LE
for consistency IMHO. If a specific field is accessed a lot,
we can add an API that does a byte swap internally.

> 
> 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 we really need to do is support __attribute__((bitwise))
from sparse.  Then we can have endian-ness statically checked.


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

We dump ACPI tables for parsing by iasl.


> 
> > Keeping it all LE seems cleaner.
> > 
> > 

On Tue, 21 Nov 2017 16:47:11 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 21, 2017 at 02:26:39PM +0100, Igor Mammedov wrote:
> > On Mon, 20 Nov 2017 22:32:29 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> 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 <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.    
> > > 
> > > 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.  
...
> > What tables/use-cases do you have in mind where we'd need full
> > ACPI parser /whatever it is/?  
> 
> We dump ACPI tables for parsing by iasl.
I don't really see a problem here (i.e. how dumping blobs for
iasl would require byte-swap all of the tables).

Where bios-tables-test.c is concerned we should just read
a blob from qemu (get its location/len/name) and dump it to
disk without changing anything in it.

> >   
> > > Keeping it all LE seems cleaner.
> > > 
> > >   

On Tue, Nov 21, 2017 at 04:58:09PM +0100, Igor Mammedov wrote:
> On Tue, 21 Nov 2017 16:47:11 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Nov 21, 2017 at 02:26:39PM +0100, Igor Mammedov wrote:
> > > On Mon, 20 Nov 2017 22:32:29 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> 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 <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.    
> > > > 
> > > > 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.  
> ...
> > > What tables/use-cases do you have in mind where we'd need full
> > > ACPI parser /whatever it is/?  
> > 
> > We dump ACPI tables for parsing by iasl.
> I don't really see a problem here (i.e. how dumping blobs for
> iasl would require byte-swap all of the tables).

It's not all the tables. We would need to track what is and
what isn't swapped.

> Where bios-tables-test.c is concerned we should just read
> a blob from qemu (get its location/len/name) and dump it to
> disk without changing anything in it.

Exactly.

> > >   
> > > > Keeping it all LE seems cleaner.
> > > > 
> > > >   

On Tue, 21 Nov 2017 18:02:48 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 21, 2017 at 04:58:09PM +0100, Igor Mammedov wrote:
> > On Tue, 21 Nov 2017 16:47:11 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Tue, Nov 21, 2017 at 02:26:39PM +0100, Igor Mammedov wrote:  
> > > > On Mon, 20 Nov 2017 22:32:29 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> 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 <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.      
> > > > > 
> > > > > 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.    
> > ...  
> > > > What tables/use-cases do you have in mind where we'd need full
> > > > ACPI parser /whatever it is/?    
> > > 
> > > We dump ACPI tables for parsing by iasl.  
> > I don't really see a problem here (i.e. how dumping blobs for
> > iasl would require byte-swap all of the tables).  
> 
> It's not all the tables. We would need to track what is and
> what isn't swapped.
Why we need track swapping at all?
Why not just convert everything to host order and work with that
in C code, so we won't have to deal with mixed endianness anywhere
in code beside of accessors wrappers? 

 
> > Where bios-tables-test.c is concerned we should just read
> > a blob from qemu (get its location/len/name) and dump it to
> > disk without changing anything in it.  
> 
> Exactly.
> 
> > > >     
> > > > > Keeping it all LE seems cleaner.
> > > > > 
> > > > >     

On Tue, Nov 21, 2017 at 05:22:36PM +0100, Igor Mammedov wrote:
> > > > > > > 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.    
> > > ...  
> > > > > What tables/use-cases do you have in mind where we'd need full
> > > > > ACPI parser /whatever it is/?    
> > > > 
> > > > We dump ACPI tables for parsing by iasl.  
> > > I don't really see a problem here (i.e. how dumping blobs for
> > > iasl would require byte-swap all of the tables).  
> > 
> > It's not all the tables. We would need to track what is and
> > what isn't swapped.
> Why we need track swapping at all?
> Why not just convert everything to host order and work with that
> in C code, so we won't have to deal with mixed endianness anywhere
> in code beside of accessors wrappers? 

It's a tooling issue really.

Consider any structure, e.g. AcpiRsdtDescriptorRev1.  We want its fields
to have specific endian-ness so we can use static checking for that down
the line.

So it must be LE since we use it to format guest tables.

Therefore this change:
-    uint32_t addr = data->rsdp_table.rsdt_physical_address;
+    uint32_t addr = le32_to_cpu(data->rsdp_table.rsdt_physical_address);

is really a requirement, otherwise we end up with converting
field format in-place and static checkers can't support that
right now.


>  
> > > Where bios-tables-test.c is concerned we should just read
> > > a blob from qemu (get its location/len/name) and dump it to
> > > disk without changing anything in it.  
> > 
> > Exactly.
> > 
> > > > >     
> > > > > > Keeping it all LE seems cleaner.
> > > > > > 
> > > > > >     

On Tue, 21 Nov 2017 18:31:19 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 21, 2017 at 05:22:36PM +0100, Igor Mammedov wrote:
> > > > > > > > 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.      
> > > > ...    
> > > > > > What tables/use-cases do you have in mind where we'd need full
> > > > > > ACPI parser /whatever it is/?      
> > > > > 
> > > > > We dump ACPI tables for parsing by iasl.    
> > > > I don't really see a problem here (i.e. how dumping blobs for
> > > > iasl would require byte-swap all of the tables).    
> > > 
> > > It's not all the tables. We would need to track what is and
> > > what isn't swapped.  
> > Why we need track swapping at all?
> > Why not just convert everything to host order and work with that
> > in C code, so we won't have to deal with mixed endianness anywhere
> > in code beside of accessors wrappers?   
> 
> It's a tooling issue really.
> 
> Consider any structure, e.g. AcpiRsdtDescriptorRev1.  We want its fields
> to have specific endian-ness so we can use static checking for that down
> the line.
> 
> So it must be LE since we use it to format guest tables.
> 
> Therefore this change:
> -    uint32_t addr = data->rsdp_table.rsdt_physical_address;
> +    uint32_t addr = le32_to_cpu(data->rsdp_table.rsdt_physical_address);
> 
> is really a requirement, otherwise we end up with converting
> field format in-place and static checkers can't support that
> right now.

I didn't really used static checker for this purpose but in my humble
experience of writing portable cross platform code, mixed endianness
always was a source of bugs and reviewing corresponding code is much
harder and a way to reduce/simplify code was isolate byte-swapping to
accessors while the the rest of the code works with native encoding.

it seems a bit wrong to make code twisted (when we don't have) for
the sake of some tooling to work.

anyways, if you prefer having mixed endianness here, I won't mind much,
as impact of test case is much smaller so we'll see down the road
if it became any better. (as far as we continue going in direction
of using host byteorder within main qemu acpi code, which we were
doing by gradually converting acpi structures to build_append_foo API)

> 
> >    
> > > > Where bios-tables-test.c is concerned we should just read
> > > > a blob from qemu (get its location/len/name) and dump it to
> > > > disk without changing anything in it.    
> > > 
> > > Exactly.
> > >   
> > > > > >       
> > > > > > > Keeping it all LE seems cleaner.
> > > > > > > 
> > > > > > >       

On Tue, Nov 21, 2017 at 05:58:12PM +0100, Igor Mammedov wrote:
> anyways, if you prefer having mixed endianness here, I won't mind much,
> as impact of test case is much smaller so we'll see down the road
> if it became any better.

I agree here.

> (as far as we continue going in direction
> of using host byteorder within main qemu acpi code, which we were
> doing by gradually converting acpi structures to build_append_foo API)

So APIs are absolutely a good alternative. If we get rid of
packed structures in main qemu code, we could convert tests
to whatever makes sense for tests.

-- 
MST