[Qemu-devel] [PATCH 1/2] acpi-test: signature endian-ness fixes

[Qemu-devel] [PATCH 1/2] acpi-test: signature endian-ness fixes

[Michael S. Tsirkin]

acpi table signature is really an ASCII string.
Treat it as such in tests.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 tests/acpi-test.c | 48 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/tests/acpi-test.c b/tests/acpi-test.c
index 185309a..249fe03 100644
--- a/tests/acpi-test.c
+++ b/tests/acpi-test.c
@@ -23,7 +23,6 @@
 #define MACHINE_Q35 "q35"
 
 #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
-#define ACPI_SSDT_SIGNATURE 0x54445353 /* SSDT */
 
 /* DSDT and SSDTs format */
 typedef struct {
@@ -101,6 +100,20 @@ typedef struct {
         ACPI_READ_FIELD((table)->asl_compiler_revision, addr);   \
     } 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); \
+    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); \
+    g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
+} while (0)
+
 /* Boot sector code: write SIGNATURE into memory,
  * then halt.
  * Q35 machine requires a minimum 0x7e000 bytes disk.
@@ -213,7 +226,7 @@ static void test_acpi_rsdp_table(test_data *data)
     uint32_t addr = data->rsdp_addr;
 
     ACPI_READ_FIELD(rsdp_table->signature, addr);
-    g_assert_cmphex(rsdp_table->signature, ==, ACPI_RSDP_SIGNATURE);
+    ACPI_ASSERT_CMP64(rsdp_table->signature, "RSD PTR ");
 
     ACPI_READ_FIELD(rsdp_table->checksum, addr);
     ACPI_READ_ARRAY(rsdp_table->oem_id, addr);
@@ -235,7 +248,7 @@ static void test_acpi_rsdt_table(test_data *data)
 
     /* read the header */
     ACPI_READ_TABLE_HEADER(rsdt_table, addr);
-    g_assert_cmphex(rsdt_table->signature, ==, ACPI_RSDT_SIGNATURE);
+    ACPI_ASSERT_CMP(rsdt_table->signature, "RSDT");
 
     /* compute the table entries in rsdt */
     tables_nr = (rsdt_table->length - sizeof(AcpiRsdtDescriptorRev1)) /
@@ -304,7 +317,7 @@ static void test_acpi_fadt_table(test_data *data)
     ACPI_READ_FIELD(fadt_table->reserved4b, addr);
     ACPI_READ_FIELD(fadt_table->flags, addr);
 
-    g_assert_cmphex(fadt_table->signature, ==, ACPI_FACP_SIGNATURE);
+    ACPI_ASSERT_CMP(fadt_table->signature, "FACP");
     g_assert(!acpi_checksum((uint8_t *)fadt_table, fadt_table->length));
 }
 
@@ -321,7 +334,7 @@ static void test_acpi_facs_table(test_data *data)
     ACPI_READ_FIELD(facs_table->flags, addr);
     ACPI_READ_ARRAY(facs_table->resverved3, addr);
 
-    g_assert_cmphex(facs_table->signature, ==, ACPI_FACS_SIGNATURE);
+    ACPI_ASSERT_CMP(facs_table->signature, "FACS");
 }
 
 static void test_dst_table(AcpiSdtTable *sdt_table, uint32_t addr)
@@ -348,7 +361,7 @@ static void test_acpi_dsdt_table(test_data *data)
     data->tables = g_array_new(false, true, sizeof(AcpiSdtTable));
 
     test_dst_table(&dsdt_table, addr);
-    g_assert_cmphex(dsdt_table.header.signature, ==, ACPI_DSDT_SIGNATURE);
+    ACPI_ASSERT_CMP(dsdt_table.header.signature, "DSDT");
 
     /* Place DSDT first */
     g_array_append_val(data->tables, dsdt_table);
@@ -383,8 +396,9 @@ 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", data_dir, data->machine,
-                                       (gchar *)&sdt->header.signature);
+                                       (gchar *)&signature);
             fd = g_open(aml_file, O_WRONLY|O_TRUNC|O_CREAT,
                         S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH);
         } else {
@@ -406,9 +420,9 @@ static void dump_aml_files(test_data *data, bool rebuild)
     }
 }
 
-static bool compare_signature(AcpiSdtTable *sdt, uint32_t signature)
+static bool compare_signature(AcpiSdtTable *sdt, const char *signature)
 {
-   return sdt->header.signature == signature;
+   return !memcmp(&sdt->header.signature, signature, 4);
 }
 
 static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
@@ -427,12 +441,12 @@ static bool load_asl(GArray *sdts, AcpiSdtTable *sdt)
 
     /* build command line */
     g_string_append_printf(command_line, " -p %s ", sdt->asl_file);
-    if (compare_signature(sdt, ACPI_DSDT_SIGNATURE) ||
-        compare_signature(sdt, ACPI_SSDT_SIGNATURE)) {
+    if (compare_signature(sdt, "DSDT") ||
+        compare_signature(sdt, "SSDT")) {
         for (i = 0; i < sdts->len; ++i) {
             temp = &g_array_index(sdts, AcpiSdtTable, i);
-            if (compare_signature(temp, ACPI_DSDT_SIGNATURE) ||
-                compare_signature(temp, ACPI_SSDT_SIGNATURE)) {
+            if (compare_signature(temp, "DSDT") ||
+                compare_signature(temp, "SSDT")) {
                 g_string_append_printf(command_line, "-e %s ", temp->aml_file);
             }
         }
@@ -495,13 +509,16 @@ 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;
+
         sdt = &g_array_index(data->tables, AcpiSdtTable, i);
 
         memset(&exp_sdt, 0, sizeof(exp_sdt));
         exp_sdt.header.signature = sdt->header.signature;
 
+        signature = cpu_to_le32(sdt->header.signature);
         aml_file = g_strdup_printf("%s/%s/%.4s", data_dir, data->machine,
-                                   (gchar *)&exp_sdt.header.signature);
+                                   (gchar *)&signature);
         exp_sdt.aml_file = aml_file;
         g_assert(g_file_test(aml_file, G_FILE_TEST_EXISTS));
         ret = g_file_get_contents(aml_file, &exp_sdt.aml,
@@ -543,12 +560,13 @@ static void test_acpi_asl(test_data *data)
         g_assert(!err || exp_err);
 
         if (g_strcmp0(asl->str, exp_asl->str)) {
+            uint32_t signature = cpu_to_le32(exp_sdt->header.signature);
             sdt->tmp_files_retain = true;
             exp_sdt->tmp_files_retain = true;
             fprintf(stderr,
                     "acpi-test: Warning! %.4s mismatch. "
                     "Actual [asl:%s, aml:%s], Expected [asl:%s, aml:%s].\n",
-                    (gchar *)&exp_sdt->header.signature,
+                    (gchar *)&signature,
                     sdt->asl_file, sdt->aml_file,
                     exp_sdt->asl_file, exp_sdt->aml_file);
         }
-- 
MST

[Qemu-devel] [PATCH 2/2] acpi: fix endian-ness for table ids

[Michael S. Tsirkin]

when using signature for table ID, we forgot to byte-swap it.
signatures are really ASCII strings, let's treat them as such.
While at it, get rid of most of _SIGNATURE macros.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/acpi-defs.h  | 14 --------------
 hw/i386/acpi-build.c | 31 ++++++++++++++++---------------
 2 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/hw/i386/acpi-defs.h b/hw/i386/acpi-defs.h
index 78ca204..e93babb 100644
--- a/hw/i386/acpi-defs.h
+++ b/hw/i386/acpi-defs.h
@@ -52,8 +52,6 @@ struct Acpi20GenericAddress {
 } QEMU_PACKED;
 typedef struct Acpi20GenericAddress Acpi20GenericAddress;
 
-#define ACPI_RSDP_SIGNATURE 0x2052545020445352LL // "RSD PTR "
-
 struct AcpiRsdpDescriptor {        /* Root System Descriptor Pointer */
     uint64_t signature;              /* ACPI signature, contains "RSD PTR " */
     uint8_t  checksum;               /* To make sum of struct == 0 */
@@ -92,7 +90,6 @@ typedef struct AcpiTableHeader AcpiTableHeader;
 /*
  * ACPI 1.0 Fixed ACPI Description Table (FADT)
  */
-#define ACPI_FACP_SIGNATURE 0x50434146 // FACP
 struct AcpiFadtDescriptorRev1
 {
     ACPI_TABLE_HEADER_DEF     /* ACPI common table header */
@@ -141,7 +138,6 @@ typedef struct AcpiFadtDescriptorRev1 AcpiFadtDescriptorRev1;
 /*
  * ACPI 1.0 Root System Description Table (RSDT)
  */
-#define ACPI_RSDT_SIGNATURE 0x54445352 // RSDT
 struct AcpiRsdtDescriptorRev1
 {
     ACPI_TABLE_HEADER_DEF       /* ACPI common table header */
@@ -153,7 +149,6 @@ typedef struct AcpiRsdtDescriptorRev1 AcpiRsdtDescriptorRev1;
 /*
  * ACPI 1.0 Firmware ACPI Control Structure (FACS)
  */
-#define ACPI_FACS_SIGNATURE 0x53434146 // FACS
 struct AcpiFacsDescriptorRev1
 {
     uint32_t signature;           /* ACPI Signature */
@@ -169,7 +164,6 @@ typedef struct AcpiFacsDescriptorRev1 AcpiFacsDescriptorRev1;
 /*
  * Differentiated System Description Table (DSDT)
  */
-#define ACPI_DSDT_SIGNATURE 0x54445344 // DSDT
 
 /*
  * MADT values and structures
@@ -182,7 +176,6 @@ typedef struct AcpiFacsDescriptorRev1 AcpiFacsDescriptorRev1;
 
 /* Master MADT */
 
-#define ACPI_APIC_SIGNATURE 0x43495041 // APIC
 struct AcpiMultipleApicTable
 {
     ACPI_TABLE_HEADER_DEF     /* ACPI common table header */
@@ -253,7 +246,6 @@ typedef struct AcpiMadtLocalNmi AcpiMadtLocalNmi;
 /*
  * HPET Description Table
  */
-#define ACPI_HPET_SIGNATURE 0x54455048 // HPET
 struct Acpi20Hpet {
     ACPI_TABLE_HEADER_DEF                    /* ACPI common table header */
     uint32_t           timer_block_id;
@@ -268,7 +260,6 @@ typedef struct Acpi20Hpet Acpi20Hpet;
  * SRAT (NUMA topology description) table
  */
 
-#define ACPI_SRAT_SIGNATURE 0x54415253 // SRAT
 struct AcpiSystemResourceAffinityTable
 {
     ACPI_TABLE_HEADER_DEF
@@ -316,11 +307,6 @@ struct AcpiMcfgAllocation {
 } QEMU_PACKED;
 typedef struct AcpiMcfgAllocation AcpiMcfgAllocation;
 
-#define ACPI_MCFG_SIGNATURE 0x4746434d       // MCFG
-
-/* Reserved signature: ignored by OSPM */
-#define ACPI_RSRV_SIGNATURE 0x554d4551       // QEMU
-
 struct AcpiTableMcfg {
     ACPI_TABLE_HEADER_DEF;
     uint8_t reserved[8];
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1dcfb25..f1054dd 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -226,14 +226,14 @@ static void acpi_get_pci_info(PcPciInfo *info)
 
 static void
 build_header(GArray *linker, GArray *table_data,
-             AcpiTableHeader *h, uint32_t sig, int len, uint8_t rev)
+             AcpiTableHeader *h, const char *sig, int len, uint8_t rev)
 {
-    h->signature = cpu_to_le32(sig);
+    memcpy(&h->signature, sig, 4);
     h->length = cpu_to_le32(len);
     h->revision = rev;
     memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
     memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
-    memcpy(h->oem_table_id + 4, (void *)&sig, 4);
+    memcpy(h->oem_table_id + 4, sig, 4);
     h->oem_revision = cpu_to_le32(1);
     memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
     h->asl_compiler_revision = cpu_to_le32(1);
@@ -495,7 +495,7 @@ static void
 build_facs(GArray *table_data, GArray *linker, PcGuestInfo *guest_info)
 {
     AcpiFacsDescriptorRev1 *facs = acpi_data_push(table_data, sizeof *facs);
-    facs->signature = cpu_to_le32(ACPI_FACS_SIGNATURE);
+    memcpy(&facs->signature, "FACS", 4);
     facs->length = cpu_to_le32(sizeof(*facs));
 }
 
@@ -552,7 +552,7 @@ build_fadt(GArray *table_data, GArray *linker, AcpiPmInfo *pm,
     fadt_setup(fadt, pm);
 
     build_header(linker, table_data,
-                 (void *)fadt, ACPI_FACP_SIGNATURE, sizeof(*fadt), 1);
+                 (void *)fadt, "FACP", sizeof(*fadt), 1);
 }
 
 static void
@@ -621,7 +621,7 @@ build_madt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu,
     local_nmi->lint         = 1; /* ACPI_LINT1 */
 
     build_header(linker, table_data,
-                 (void *)(table_data->data + madt_start), ACPI_APIC_SIGNATURE,
+                 (void *)(table_data->data + madt_start), "APIC",
                  table_data->len - madt_start, 1);
 }
 
@@ -1098,7 +1098,7 @@ build_ssdt(GArray *table_data, GArray *linker,
 
     build_header(linker, table_data,
                  (void *)(table_data->data + ssdt_start),
-                 ACPI_SSDT_SIGNATURE, table_data->len - ssdt_start, 1);
+                 "SSDT", table_data->len - ssdt_start, 1);
 }
 
 static void
@@ -1113,7 +1113,7 @@ build_hpet(GArray *table_data, GArray *linker)
     hpet->timer_block_id = cpu_to_le32(0x8086a201);
     hpet->addr.address = cpu_to_le64(HPET_BASE);
     build_header(linker, table_data,
-                 (void *)hpet, ACPI_HPET_SIGNATURE, sizeof(*hpet), 1);
+                 (void *)hpet, "HPET", sizeof(*hpet), 1);
 }
 
 static void
@@ -1205,7 +1205,7 @@ build_srat(GArray *table_data, GArray *linker,
 
     build_header(linker, table_data,
                  (void *)(table_data->data + srat_start),
-                 ACPI_SRAT_SIGNATURE,
+                 "SRAT",
                  table_data->len - srat_start, 1);
 }
 
@@ -1213,7 +1213,7 @@ static void
 build_mcfg_q35(GArray *table_data, GArray *linker, AcpiMcfgInfo *info)
 {
     AcpiTableMcfg *mcfg;
-    uint32_t sig;
+    const char *sig;
     int len = sizeof(*mcfg) + 1 * sizeof(mcfg->allocation[0]);
 
     mcfg = acpi_data_push(table_data, len);
@@ -1230,9 +1230,10 @@ build_mcfg_q35(GArray *table_data, GArray *linker, AcpiMcfgInfo *info)
      * ACPI spec requires OSPMs to ignore such tables.
      */
     if (info->mcfg_base == PCIE_BASE_ADDR_UNMAPPED) {
-        sig = ACPI_RSRV_SIGNATURE;
+        /* Reserved signature: ignored by OSPM */
+        sig = "QEMU";
     } else {
-        sig = ACPI_MCFG_SIGNATURE;
+        sig = "MCFG";
     }
     build_header(linker, table_data, (void *)mcfg, sig, len, 1);
 }
@@ -1248,7 +1249,7 @@ build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc)
     memcpy(dsdt, misc->dsdt_code, misc->dsdt_size);
 
     memset(dsdt, 0, sizeof *dsdt);
-    build_header(linker, table_data, dsdt, ACPI_DSDT_SIGNATURE,
+    build_header(linker, table_data, dsdt, "DSDT",
                  misc->dsdt_size, 1);
 }
 
@@ -1273,7 +1274,7 @@ build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
                                        sizeof(uint32_t));
     }
     build_header(linker, table_data,
-                 (void *)rsdt, ACPI_RSDT_SIGNATURE, rsdt_len, 1);
+                 (void *)rsdt, "RSDT", rsdt_len, 1);
 }
 
 static GArray *
@@ -1284,7 +1285,7 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
     bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, 1,
                              true /* fseg memory */);
 
-    rsdp->signature = cpu_to_le64(ACPI_RSDP_SIGNATURE);
+    memcpy(&rsdp->signature, "RSD PTR ", 8);
     memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
     rsdp->rsdt_physical_address = cpu_to_le32(rsdt);
     /* Address to be filled by Guest linker */
-- 
MST

Re: [Qemu-devel] [PATCH 1/2] acpi-test: signature endian-ness fixes

[Laszlo Ersek]

On 03/18/14 15:48, Michael S. Tsirkin wrote:
> acpi table signature is really an ASCII string.
> Treat it as such in tests.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  tests/acpi-test.c | 48 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/acpi-test.c b/tests/acpi-test.c
> index 185309a..249fe03 100644
> --- a/tests/acpi-test.c
> +++ b/tests/acpi-test.c
> @@ -23,7 +23,6 @@
>  #define MACHINE_Q35 "q35"
>  
>  #define ACPI_REBUILD_EXPECTED_AML "TEST_ACPI_REBUILD_AML"
> -#define ACPI_SSDT_SIGNATURE 0x54445353 /* SSDT */
>  
>  /* DSDT and SSDTs format */
>  typedef struct {
> @@ -101,6 +100,20 @@ typedef struct {
>          ACPI_READ_FIELD((table)->asl_compiler_revision, addr);   \
>      } 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); \
> +    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); \
> +    g_assert_cmpstr(ACPI_ASSERT_CMP_str, ==, expected); \
> +} while (0)
> +

The {} initializers are GNUisms, { 0 } is the standard way to write them.

(For strings where you specify the size explicitly, = "" would work too
(in C99 explicitly, in C89 a bit more navel gazing would be required to
derive that).)

The patch seems reasonable to me.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] acpi: fix endian-ness for table ids

[Laszlo Ersek]

On 03/18/14 15:48, Michael S. Tsirkin wrote:
> when using signature for table ID, we forgot to byte-swap it.
> signatures are really ASCII strings, let's treat them as such.
> While at it, get rid of most of _SIGNATURE macros.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/i386/acpi-defs.h  | 14 --------------
>  hw/i386/acpi-build.c | 31 ++++++++++++++++---------------
>  2 files changed, 16 insertions(+), 29 deletions(-)

Looks reasonable. Of course you'll probably want Marcel's R-b.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] acpi: fix endian-ness for table ids

[Marcel Apfelbaum]

On Tue, 2014-03-18 at 19:07 +0100, Laszlo Ersek wrote:
> On 03/18/14 15:48, Michael S. Tsirkin wrote:
> > when using signature for table ID, we forgot to byte-swap it.
> > signatures are really ASCII strings, let's treat them as such.
> > While at it, get rid of most of _SIGNATURE macros.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/i386/acpi-defs.h  | 14 --------------
> >  hw/i386/acpi-build.c | 31 ++++++++++++++++---------------
> >  2 files changed, 16 insertions(+), 29 deletions(-)
> 
> Looks reasonable. Of course you'll probably want Marcel's R-b.
I agree with the changes, but sadly it does not resolve the problem for ppc64 hosts.

Reviewed-by: Marcel Apfelbaum <marcel.a@redhat.com>

> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>