[Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

XSDT support allows using ACPI 2 features while
avoiding breaking legacy windows XP guests:
ACPI 2 tables are linked from XSDT only,
ACPI 1 tables from both RSDT and XSDT, this way
XP does not see ACPI 2 tables.

As a first step, this patchset generates v2 RSDP
and fills in XSDT matching RSDT exactly.

ARM can switch to XSDT as well, I'm not bothering
until there's an easy way to test that.

Note: unit test files need to be updated with this,
I'm not bothering with posting them.

Changes from v1:
    xsdt address is 64 bit
    arm patch is now tested

Michael S. Tsirkin (4):
  acpi: add API for 64 bit offsets
  i386/acpi: collect 64 bit offsets for xsdt
  i386/acpi: add XSDT
  acpi: unify rsdp generation

 include/hw/acpi/acpi-defs.h | 15 +++++--
 include/hw/acpi/aml-build.h |  7 +++-
 hw/acpi/aml-build.c         | 99 +++++++++++++++++++++++++++++++++++++--------
 hw/arm/virt-acpi-build.c    | 39 +++---------------
 hw/i386/acpi-build.c        | 64 +++++++++++------------------
 5 files changed, 129 insertions(+), 95 deletions(-)

-- 
MST

[Qemu-devel] [PATCH v2 1/4] acpi: add API for 64 bit offsets

[Michael S. Tsirkin]

Collecting these is useful for implementing the XSDT.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/aml-build.h |  3 ++-
 hw/acpi/aml-build.c         | 13 ++++++++++---
 hw/arm/virt-acpi-build.c    |  8 ++++----
 hw/i386/acpi-build.c        | 20 ++++++++++----------
 4 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index e3afa13..edc1dfa 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -281,7 +281,8 @@ build_header(GArray *linker, GArray *table_data,
              AcpiTableHeader *h, const char *sig, int len, uint8_t rev);
 void *acpi_data_push(GArray *table_data, unsigned size);
 unsigned acpi_data_len(GArray *table);
-void acpi_add_table(GArray *table_offsets, GArray *table_data);
+void acpi_add_table(GArray *table_offsets32, GArray *table_offsets64,
+                    GArray *table_data);
 void acpi_build_tables_init(AcpiBuildTables *tables);
 void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
 void
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 0d4b324..4c930b6 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1169,10 +1169,17 @@ unsigned acpi_data_len(GArray *table)
     return table->len;
 }
 
-void acpi_add_table(GArray *table_offsets, GArray *table_data)
+void acpi_add_table(GArray *table_offsets32, GArray *table_offsets64,
+                    GArray *table_data)
 {
-    uint32_t offset = cpu_to_le32(table_data->len);
-    g_array_append_val(table_offsets, offset);
+    uint32_t offset32 = cpu_to_le32(table_data->len);
+    uint64_t offset64 = cpu_to_le64(table_data->len);
+    if (table_offsets32) {
+        g_array_append_val(table_offsets32, offset32);
+    }
+    if (table_offsets64) {
+        g_array_append_val(table_offsets64, offset64);
+    }
 }
 
 void acpi_build_tables_init(AcpiBuildTables *tables)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a9373cc..42c8dd9 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -515,16 +515,16 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
     build_dsdt(tables_blob, tables->linker, guest_info);
 
     /* FADT MADT GTDT pointed to by RSDT */
-    acpi_add_table(table_offsets, tables_blob);
+    acpi_add_table(table_offsets, NULL, tables_blob);
     build_fadt(tables_blob, tables->linker, dsdt);
 
-    acpi_add_table(table_offsets, tables_blob);
+    acpi_add_table(table_offsets, NULL, tables_blob);
     build_madt(tables_blob, tables->linker, guest_info, &cpuinfo);
 
-    acpi_add_table(table_offsets, tables_blob);
+    acpi_add_table(table_offsets, NULL, tables_blob);
     build_gtdt(tables_blob, tables->linker);
 
-    acpi_add_table(table_offsets, tables_blob);
+    acpi_add_table(table_offsets, NULL, tables_blob);
     build_mcfg(tables_blob, tables->linker, guest_info);
 
     /* RSDT is pointed to by RSDP */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index db32fd1..e5b0b7a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1706,27 +1706,27 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     aml_len += tables_blob->len - dsdt;
 
     /* ACPI tables pointed to by RSDT */
-    acpi_add_table(table_offsets, tables_blob);
+    acpi_add_table(table_offsets, NULL, tables_blob);
     build_fadt(tables_blob, tables->linker, &pm, facs, dsdt);
 
     ssdt = tables_blob->len;
-    acpi_add_table(table_offsets, tables_blob);
+    acpi_add_table(table_offsets, NULL, tables_blob);
     build_ssdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci,
                guest_info);
     aml_len += tables_blob->len - ssdt;
 
-    acpi_add_table(table_offsets, tables_blob);
+    acpi_add_table(table_offsets, NULL, tables_blob);
     build_madt(tables_blob, tables->linker, &cpu, guest_info);
 
     if (misc.has_hpet) {
-        acpi_add_table(table_offsets, tables_blob);
+        acpi_add_table(table_offsets, NULL, tables_blob);
         build_hpet(tables_blob, tables->linker);
     }
     if (misc.tpm_version != TPM_VERSION_UNSPEC) {
-        acpi_add_table(table_offsets, tables_blob);
+        acpi_add_table(table_offsets, NULL, tables_blob);
         build_tpm_tcpa(tables_blob, tables->linker, tables->tcpalog);
 
-        acpi_add_table(table_offsets, tables_blob);
+        acpi_add_table(table_offsets, NULL, tables_blob);
         switch (misc.tpm_version) {
         case TPM_VERSION_1_2:
             build_tpm_ssdt(tables_blob, tables->linker);
@@ -1739,15 +1739,15 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
         }
     }
     if (guest_info->numa_nodes) {
-        acpi_add_table(table_offsets, tables_blob);
+        acpi_add_table(table_offsets, NULL, tables_blob);
         build_srat(tables_blob, tables->linker, guest_info);
     }
     if (acpi_get_mcfg(&mcfg)) {
-        acpi_add_table(table_offsets, tables_blob);
+        acpi_add_table(table_offsets, NULL, tables_blob);
         build_mcfg_q35(tables_blob, tables->linker, &mcfg);
     }
     if (acpi_has_iommu()) {
-        acpi_add_table(table_offsets, tables_blob);
+        acpi_add_table(table_offsets, NULL, tables_blob);
         build_dmar_q35(tables_blob, tables->linker);
     }
 
@@ -1755,7 +1755,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
         unsigned len = acpi_table_len(u);
 
-        acpi_add_table(table_offsets, tables_blob);
+        acpi_add_table(table_offsets, NULL, tables_blob);
         g_array_append_vals(tables_blob, u, len);
     }
 
-- 
MST

[Qemu-devel] [PATCH v2 2/4] i386/acpi: collect 64 bit offsets for xsdt

[Michael S. Tsirkin]

collect then discard, they are unused for now.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/acpi-build.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e5b0b7a..aaa149b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1663,7 +1663,8 @@ static bool acpi_has_iommu(void)
 static
 void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
 {
-    GArray *table_offsets;
+    GArray *rsdt_table_offsets;
+    GArray *xsdt_table_offsets;
     unsigned facs, ssdt, dsdt, rsdt;
     AcpiCpuInfo cpu;
     AcpiPmInfo pm;
@@ -1680,8 +1681,10 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     acpi_get_misc_info(&misc);
     acpi_get_pci_info(&pci);
 
-    table_offsets = g_array_new(false, true /* clear */,
+    rsdt_table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));
+    xsdt_table_offsets = g_array_new(false, true /* clear */,
+                                        sizeof(uint64_t));
     ACPI_BUILD_DPRINTF("init ACPI tables\n");
 
     bios_linker_loader_alloc(tables->linker, ACPI_BUILD_TABLE_FILE,
@@ -1706,27 +1709,27 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     aml_len += tables_blob->len - dsdt;
 
     /* ACPI tables pointed to by RSDT */
-    acpi_add_table(table_offsets, NULL, tables_blob);
+    acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
     build_fadt(tables_blob, tables->linker, &pm, facs, dsdt);
 
     ssdt = tables_blob->len;
-    acpi_add_table(table_offsets, NULL, tables_blob);
+    acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
     build_ssdt(tables_blob, tables->linker, &cpu, &pm, &misc, &pci,
                guest_info);
     aml_len += tables_blob->len - ssdt;
 
-    acpi_add_table(table_offsets, NULL, tables_blob);
+    acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
     build_madt(tables_blob, tables->linker, &cpu, guest_info);
 
     if (misc.has_hpet) {
-        acpi_add_table(table_offsets, NULL, tables_blob);
+        acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
         build_hpet(tables_blob, tables->linker);
     }
     if (misc.tpm_version != TPM_VERSION_UNSPEC) {
-        acpi_add_table(table_offsets, NULL, tables_blob);
+        acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
         build_tpm_tcpa(tables_blob, tables->linker, tables->tcpalog);
 
-        acpi_add_table(table_offsets, NULL, tables_blob);
+        acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
         switch (misc.tpm_version) {
         case TPM_VERSION_1_2:
             build_tpm_ssdt(tables_blob, tables->linker);
@@ -1739,15 +1742,15 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
         }
     }
     if (guest_info->numa_nodes) {
-        acpi_add_table(table_offsets, NULL, tables_blob);
+        acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
         build_srat(tables_blob, tables->linker, guest_info);
     }
     if (acpi_get_mcfg(&mcfg)) {
-        acpi_add_table(table_offsets, NULL, tables_blob);
+        acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
         build_mcfg_q35(tables_blob, tables->linker, &mcfg);
     }
     if (acpi_has_iommu()) {
-        acpi_add_table(table_offsets, NULL, tables_blob);
+        acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
         build_dmar_q35(tables_blob, tables->linker);
     }
 
@@ -1755,13 +1758,13 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     for (u = acpi_table_first(); u; u = acpi_table_next(u)) {
         unsigned len = acpi_table_len(u);
 
-        acpi_add_table(table_offsets, NULL, tables_blob);
+        acpi_add_table(rsdt_table_offsets, xsdt_table_offsets, tables_blob);
         g_array_append_vals(tables_blob, u, len);
     }
 
     /* RSDT is pointed to by RSDP */
     rsdt = tables_blob->len;
-    build_rsdt(tables_blob, tables->linker, table_offsets);
+    build_rsdt(tables_blob, tables->linker, rsdt_table_offsets);
 
     /* RSDP is in FSEG memory, so allocate it separately */
     build_rsdp(tables->rsdp, tables->linker, rsdt);
@@ -1813,7 +1816,8 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
     acpi_align_size(tables->linker, ACPI_BUILD_ALIGN_SIZE);
 
     /* Cleanup memory that's no longer used. */
-    g_array_free(table_offsets, true);
+    g_array_free(xsdt_table_offsets, true);
+    g_array_free(rsdt_table_offsets, true);
 }
 
 static void acpi_ram_update(MemoryRegion *mr, GArray *data)
-- 
MST

[Qemu-devel] [PATCH v2 3/4] i386/acpi: add XSDT

[Michael S. Tsirkin]

At the moment it mirrors RSDT exactly.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/acpi-defs.h | 15 ++++++++++++---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 41 ++++++++++++++++++++++++++++-------------
 hw/i386/acpi-build.c        | 29 ++++++++++++++++++++++++-----
 4 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 59cf277..fe1dc7d 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -201,13 +201,22 @@ enum {
  */
 struct AcpiRsdtDescriptorRev1
 {
-    ACPI_TABLE_HEADER_DEF       /* ACPI common table header */
-    uint32_t table_offset_entry[0];  /* Array of pointers to other */
-    /* ACPI tables */
+    ACPI_TABLE_HEADER_DEF           /* ACPI common table header */
+    uint32_t table_offset_entry[0]; /* Array of pointers to other ACPI tables */
 } QEMU_PACKED;
 typedef struct AcpiRsdtDescriptorRev1 AcpiRsdtDescriptorRev1;
 
 /*
+ * ACPI 2.0 Extended System Description Table (XSDT)
+ */
+struct AcpiXsdtDescriptor
+{
+    ACPI_TABLE_HEADER_DEF           /* ACPI common table header */
+    uint64_t table_offset_entry[0]; /* Array of pointers to other ACPI tables */
+} QEMU_PACKED;
+typedef struct AcpiXsdtDescriptor AcpiXsdtDescriptor;
+
+/*
  * ACPI 1.0 Firmware ACPI Control Structure (FACS)
  */
 struct AcpiFacsDescriptorRev1
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index edc1dfa..0a00402 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -287,5 +287,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables);
 void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre);
 void
 build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets);
+void
+build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets);
 
 #endif
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 4c930b6..d0e52c4 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1200,25 +1200,40 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
 }
 
 /* Build rsdt table */
-void
-build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
+static void
+build_sdt(GArray *table_data, GArray *linker, GArray *table_offsets,
+          const char *sig, int entry_size)
 {
-    AcpiRsdtDescriptorRev1 *rsdt;
-    size_t rsdt_len;
+    AcpiTableHeader *sdt;
+    size_t sdt_len;
     int i;
-    const int table_data_len = (sizeof(uint32_t) * table_offsets->len);
+    const int table_data_len = (entry_size * table_offsets->len);
+    char *entries;
 
-    rsdt_len = sizeof(*rsdt) + table_data_len;
-    rsdt = acpi_data_push(table_data, rsdt_len);
-    memcpy(rsdt->table_offset_entry, table_offsets->data, table_data_len);
+    sdt_len = sizeof(*sdt) + table_data_len;
+    sdt = acpi_data_push(table_data, sdt_len);
+    entries = ((char *)sdt) + sizeof(*sdt);
+    memcpy(entries, table_offsets->data, table_data_len);
     for (i = 0; i < table_offsets->len; ++i) {
-        /* rsdt->table_offset_entry to be filled by Guest linker */
+        /* entry to be filled by Guest linker */
         bios_linker_loader_add_pointer(linker,
                                        ACPI_BUILD_TABLE_FILE,
                                        ACPI_BUILD_TABLE_FILE,
-                                       table_data, &rsdt->table_offset_entry[i],
-                                       sizeof(uint32_t));
+                                       table_data, entries + i * entry_size,
+                                       entry_size);
     }
-    build_header(linker, table_data,
-                 (void *)rsdt, "RSDT", rsdt_len, 1);
+    build_header(linker, table_data, sdt, sig, sdt_len, 1);
+}
+
+void
+build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
+{
+    build_sdt(table_data, linker, table_offsets, "RSDT", sizeof(uint32_t));
+}
+
+/* Build xsdt table */
+void
+build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
+{
+    build_sdt(table_data, linker, table_offsets, "XSDT", sizeof(uint64_t));
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index aaa149b..3579d18 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1593,7 +1593,7 @@ build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc)
 }
 
 static GArray *
-build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
+build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt, unsigned xsdt)
 {
     AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
 
@@ -1602,16 +1602,33 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
 
     memcpy(&rsdp->signature, "RSD PTR ", 8);
     memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
+
+    rsdp->revision = 0x02;
+    rsdp->length = cpu_to_le32(sizeof *rsdp);
+
     rsdp->rsdt_physical_address = cpu_to_le32(rsdt);
     /* Address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
                                    ACPI_BUILD_TABLE_FILE,
                                    rsdp_table, &rsdp->rsdt_physical_address,
                                    sizeof rsdp->rsdt_physical_address);
+
+    rsdp->xsdt_physical_address = cpu_to_le64(xsdt);
+    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
+                                   ACPI_BUILD_TABLE_FILE,
+                                   rsdp_table, &rsdp->xsdt_physical_address,
+                                   sizeof rsdp->xsdt_physical_address);
     rsdp->checksum = 0;
     /* Checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-                                    rsdp, rsdp, sizeof *rsdp, &rsdp->checksum);
+                                    rsdp, rsdp, offsetof(AcpiRsdpDescriptor, length),
+                                    &rsdp->checksum);
+
+    rsdp->extended_checksum = 0;
+    /* Checksum to be filled by Guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
+                                    rsdp, rsdp, sizeof *rsdp,
+                                    &rsdp->extended_checksum);
 
     return rsdp_table;
 }
@@ -1665,7 +1682,7 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
 {
     GArray *rsdt_table_offsets;
     GArray *xsdt_table_offsets;
-    unsigned facs, ssdt, dsdt, rsdt;
+    unsigned facs, ssdt, dsdt, rsdt, xsdt;
     AcpiCpuInfo cpu;
     AcpiPmInfo pm;
     AcpiMiscInfo misc;
@@ -1762,12 +1779,14 @@ void acpi_build(PcGuestInfo *guest_info, AcpiBuildTables *tables)
         g_array_append_vals(tables_blob, u, len);
     }
 
-    /* RSDT is pointed to by RSDP */
+    /* RSDT and XSDT pointed to by RSDP */
     rsdt = tables_blob->len;
     build_rsdt(tables_blob, tables->linker, rsdt_table_offsets);
+    xsdt = tables_blob->len;
+    build_xsdt(tables_blob, tables->linker, xsdt_table_offsets);
 
     /* RSDP is in FSEG memory, so allocate it separately */
-    build_rsdp(tables->rsdp, tables->linker, rsdt);
+    build_rsdp(tables->rsdp, tables->linker, rsdt, xsdt);
 
     /* We'll expose it all to Guest so we want to reduce
      * chance of size changes.
-- 
MST

[Qemu-devel] [PATCH v2 4/4] acpi: unify rsdp generation

[Michael S. Tsirkin]

Now that both i386 and arm use v2 tables,
use common code for both.

Tested-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/acpi/aml-build.h |  2 ++
 hw/acpi/aml-build.c         | 45 +++++++++++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 31 +------------------------------
 hw/i386/acpi-build.c        | 41 -----------------------------------------
 4 files changed, 48 insertions(+), 71 deletions(-)

diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 0a00402..ae169fa 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -289,5 +289,7 @@ void
 build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets);
 void
 build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets);
+GArray *
+build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt, unsigned xsdt);
 
 #endif
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index d0e52c4..9449875 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1237,3 +1237,48 @@ build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
 {
     build_sdt(table_data, linker, table_offsets, "XSDT", sizeof(uint64_t));
 }
+
+GArray *
+build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt, unsigned xsdt)
+{
+    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
+
+    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, 16,
+                             true /* fseg memory */);
+
+    memcpy(&rsdp->signature, "RSD PTR ", 8);
+    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
+
+    rsdp->revision = 0x02;
+    rsdp->length = cpu_to_le32(sizeof *rsdp);
+
+    if (rsdt) {
+        rsdp->rsdt_physical_address = cpu_to_le32(rsdt);
+        /* Address to be filled by Guest linker */
+        bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
+                                       ACPI_BUILD_TABLE_FILE,
+                                       rsdp_table, &rsdp->rsdt_physical_address,
+                                       sizeof rsdp->rsdt_physical_address);
+    }
+
+    if (xsdt) {
+        rsdp->xsdt_physical_address = cpu_to_le64(xsdt);
+        bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
+                                       ACPI_BUILD_TABLE_FILE,
+                                       rsdp_table, &rsdp->xsdt_physical_address,
+                                       sizeof rsdp->xsdt_physical_address);
+    }
+    rsdp->checksum = 0;
+    /* Checksum to be filled by Guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
+                                    rsdp, rsdp, offsetof(AcpiRsdpDescriptor, length),
+                                    &rsdp->checksum);
+
+    rsdp->extended_checksum = 0;
+    /* Checksum to be filled by Guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
+                                    rsdp, rsdp, sizeof *rsdp,
+                                    &rsdp->extended_checksum);
+
+    return rsdp_table;
+}
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 42c8dd9..c164f65 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -304,35 +304,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq)
     aml_append(scope, dev);
 }
 
-/* RSDP */
-static GArray *
-build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
-{
-    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
-
-    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, 16,
-                             true /* fseg memory */);
-
-    memcpy(&rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
-    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
-    rsdp->length = cpu_to_le32(sizeof(*rsdp));
-    rsdp->revision = 0x02;
-
-    /* Point to RSDT */
-    rsdp->rsdt_physical_address = cpu_to_le32(rsdt);
-    /* Address to be filled by Guest linker */
-    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
-                                   ACPI_BUILD_TABLE_FILE,
-                                   rsdp_table, &rsdp->rsdt_physical_address,
-                                   sizeof rsdp->rsdt_physical_address);
-    rsdp->checksum = 0;
-    /* Checksum to be filled by Guest linker */
-    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-                                    rsdp, rsdp, sizeof *rsdp, &rsdp->checksum);
-
-    return rsdp_table;
-}
-
 static void
 build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
 {
@@ -532,7 +503,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
     build_rsdt(tables_blob, tables->linker, table_offsets);
 
     /* RSDP is in FSEG memory, so allocate it separately */
-    build_rsdp(tables->rsdp, tables->linker, rsdt);
+    build_rsdp(tables->rsdp, tables->linker, rsdt, 0);
 
     /* Cleanup memory that's no longer used. */
     g_array_free(table_offsets, true);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 3579d18..8c6d7f5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1592,47 +1592,6 @@ build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc)
                  misc->dsdt_size, 1);
 }
 
-static GArray *
-build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt, unsigned xsdt)
-{
-    AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
-
-    bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, 16,
-                             true /* fseg memory */);
-
-    memcpy(&rsdp->signature, "RSD PTR ", 8);
-    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
-
-    rsdp->revision = 0x02;
-    rsdp->length = cpu_to_le32(sizeof *rsdp);
-
-    rsdp->rsdt_physical_address = cpu_to_le32(rsdt);
-    /* Address to be filled by Guest linker */
-    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
-                                   ACPI_BUILD_TABLE_FILE,
-                                   rsdp_table, &rsdp->rsdt_physical_address,
-                                   sizeof rsdp->rsdt_physical_address);
-
-    rsdp->xsdt_physical_address = cpu_to_le64(xsdt);
-    bios_linker_loader_add_pointer(linker, ACPI_BUILD_RSDP_FILE,
-                                   ACPI_BUILD_TABLE_FILE,
-                                   rsdp_table, &rsdp->xsdt_physical_address,
-                                   sizeof rsdp->xsdt_physical_address);
-    rsdp->checksum = 0;
-    /* Checksum to be filled by Guest linker */
-    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-                                    rsdp, rsdp, offsetof(AcpiRsdpDescriptor, length),
-                                    &rsdp->checksum);
-
-    rsdp->extended_checksum = 0;
-    /* Checksum to be filled by Guest linker */
-    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-                                    rsdp, rsdp, sizeof *rsdp,
-                                    &rsdp->extended_checksum);
-
-    return rsdp_table;
-}
-
 typedef
 struct AcpiBuildState {
     /* Copy of table in RAM (for patching). */
-- 
MST

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Laszlo Ersek]

On 06/08/15 20:14, Michael S. Tsirkin wrote:
> XSDT support allows using ACPI 2 features while
> avoiding breaking legacy windows XP guests:
> ACPI 2 tables are linked from XSDT only,
> ACPI 1 tables from both RSDT and XSDT, this way
> XP does not see ACPI 2 tables.
> 
> As a first step, this patchset generates v2 RSDP
> and fills in XSDT matching RSDT exactly.
> 
> ARM can switch to XSDT as well, I'm not bothering
> until there's an easy way to test that.
> 
> Note: unit test files need to be updated with this,
> I'm not bothering with posting them.
> 
> Changes from v1:
>     xsdt address is 64 bit
>     arm patch is now tested
> 
> Michael S. Tsirkin (4):
>   acpi: add API for 64 bit offsets
>   i386/acpi: collect 64 bit offsets for xsdt
>   i386/acpi: add XSDT
>   acpi: unify rsdp generation
> 
>  include/hw/acpi/acpi-defs.h | 15 +++++--
>  include/hw/acpi/aml-build.h |  7 +++-
>  hw/acpi/aml-build.c         | 99 +++++++++++++++++++++++++++++++++++++--------
>  hw/arm/virt-acpi-build.c    | 39 +++---------------
>  hw/i386/acpi-build.c        | 64 +++++++++++------------------
>  5 files changed, 129 insertions(+), 95 deletions(-)
> 

I tested this series with ArmVirtQemu (aka AAVMF) and OVMF too. (They
use common code in edk2.)

The ARM build works indeed, but that's only because in patch #4 we have

  build_rsdp(tables->rsdp, tables->linker, rsdt, 0);

ie. there's only an RSDT.

Due to patch #3 however, the OVMF client breaks:

  build_rsdp(tables->rsdp, tables->linker, rsdt, xsdt);

The problem is that the "directed acyclic graph" of ADD_POINTER edges is
no longer a tree. At least some tables can be reached on multiple paths.
(Eg. the FADT can be reached via RSDP->RSDT-> FADT, and also
RSDP->XSDT->FADT.)

This is a problem because EFI_ACPI_TABLE_PROTOCOL has "builtin
knowledge" about what tables may not be installed only once vs. several
times. Unfortunately, in this case both decisions have bad consequences:

- When FADT is passed to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for
the second time, the function returns EFI_ACCESS_DENIED, and the
linker/loader client bails out.

- When (eg.) the same SSDT is passed to
EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for the second time, the
function will happily install (a copy of) it again, and then we'll end
up with two copies of the same SSDT.

EFI_ACPI_TABLE_PROTOCOL manages both the RSDT and the XSDT internally.
As far as I can see, it puts each table in both. The RSDT and the XSDT
are not distinguished even on the UEFI spec level; it lumps them
together under "RSDT/XSDT" every time.

This is probably very frustrating; sorry about that.

Laszlo

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

On Tue, Jun 09, 2015 at 02:08:03AM +0200, Laszlo Ersek wrote:
> On 06/08/15 20:14, Michael S. Tsirkin wrote:
> > XSDT support allows using ACPI 2 features while
> > avoiding breaking legacy windows XP guests:
> > ACPI 2 tables are linked from XSDT only,
> > ACPI 1 tables from both RSDT and XSDT, this way
> > XP does not see ACPI 2 tables.
> > 
> > As a first step, this patchset generates v2 RSDP
> > and fills in XSDT matching RSDT exactly.
> > 
> > ARM can switch to XSDT as well, I'm not bothering
> > until there's an easy way to test that.
> > 
> > Note: unit test files need to be updated with this,
> > I'm not bothering with posting them.
> > 
> > Changes from v1:
> >     xsdt address is 64 bit
> >     arm patch is now tested
> > 
> > Michael S. Tsirkin (4):
> >   acpi: add API for 64 bit offsets
> >   i386/acpi: collect 64 bit offsets for xsdt
> >   i386/acpi: add XSDT
> >   acpi: unify rsdp generation
> > 
> >  include/hw/acpi/acpi-defs.h | 15 +++++--
> >  include/hw/acpi/aml-build.h |  7 +++-
> >  hw/acpi/aml-build.c         | 99 +++++++++++++++++++++++++++++++++++++--------
> >  hw/arm/virt-acpi-build.c    | 39 +++---------------
> >  hw/i386/acpi-build.c        | 64 +++++++++++------------------
> >  5 files changed, 129 insertions(+), 95 deletions(-)
> > 
> 
> I tested this series with ArmVirtQemu (aka AAVMF) and OVMF too. (They
> use common code in edk2.)
> 
> The ARM build works indeed, but that's only because in patch #4 we have
> 
>   build_rsdp(tables->rsdp, tables->linker, rsdt, 0);
> 
> ie. there's only an RSDT.
> 
> Due to patch #3 however, the OVMF client breaks:
> 
>   build_rsdp(tables->rsdp, tables->linker, rsdt, xsdt);
> 
> The problem is that the "directed acyclic graph" of ADD_POINTER edges is
> no longer a tree. At least some tables can be reached on multiple paths.
> (Eg. the FADT can be reached via RSDP->RSDT-> FADT, and also
> RSDP->XSDT->FADT.)
> 
> This is a problem because EFI_ACPI_TABLE_PROTOCOL has "builtin
> knowledge" about what tables may not be installed only once vs. several
> times. Unfortunately, in this case both decisions have bad consequences:
> 
> - When FADT is passed to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for
> the second time, the function returns EFI_ACCESS_DENIED, and the
> linker/loader client bails out.
> 
> - When (eg.) the same SSDT is passed to
> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for the second time, the
> function will happily install (a copy of) it again, and then we'll end
> up with two copies of the same SSDT.
> 
> EFI_ACPI_TABLE_PROTOCOL manages both the RSDT and the XSDT internally.
> As far as I can see, it puts each table in both. The RSDT and the XSDT
> are not distinguished even on the UEFI spec level; it lumps them
> together under "RSDT/XSDT" every time.
> 
> This is probably very frustrating; sorry about that.
> 
> Laszlo

Thanks for the info!  This is worth fixing.  Can you fix this without
protocol changes, or should we change the protocol to pass a hint that a
pointer is to another instance of a previously used table?

-- 
MST

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]

I would just patch OVMF to ignore the RSDT if there is an XSDT.

Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.

Paolo


-----Original Message-----
From: Michael S. Tsirkin [mst@redhat.com]
Received: martedì, 09 giu 2015, 7:31
To: Laszlo Ersek [lersek@redhat.com]
CC: qemu-devel@nongnu.org, ghammer@redhat.com, pbonzini@redhat.com, shannon.zhao@linaro.org, imammedo@redhat.com
Subject: Re: [PATCH v2 0/4]  acpi: xsdt support

On Tue, Jun 09, 2015 at 02:08:03AM +0200, Laszlo Ersek wrote:
> On 06/08/15 20:14, Michael S. Tsirkin wrote:
> > XSDT support allows using ACPI 2 features while
> > avoiding breaking legacy windows XP guests:
> > ACPI 2 tables are linked from XSDT only,
> > ACPI 1 tables from both RSDT and XSDT, this way
> > XP does not see ACPI 2 tables.
> > 
> > As a first step, this patchset generates v2 RSDP
> > and fills in XSDT matching RSDT exactly.
> > 
> > ARM can switch to XSDT as well, I'm not bothering
> > until there's an easy way to test that.
> > 
> > Note: unit test files need to be updated with this,
> > I'm not bothering with posting them.
> > 
> > Changes from v1:
> >     xsdt address is 64 bit
> >     arm patch is now tested
> > 
> > Michael S. Tsirkin (4):
> >   acpi: add API for 64 bit offsets
> >   i386/acpi: collect 64 bit offsets for xsdt
> >   i386/acpi: add XSDT
> >   acpi: unify rsdp generation
> > 
> >  include/hw/acpi/acpi-defs.h | 15 +++++--
> >  include/hw/acpi/aml-build.h |  7 +++-
> >  hw/acpi/aml-build.c         | 99 +++++++++++++++++++++++++++++++++++++--------
> >  hw/arm/virt-acpi-build.c    | 39 +++---------------
> >  hw/i386/acpi-build.c        | 64 +++++++++++------------------
> >  5 files changed, 129 insertions(+), 95 deletions(-)
> > 
> 
> I tested this series with ArmVirtQemu (aka AAVMF) and OVMF too. (They
> use common code in edk2.)
> 
> The ARM build works indeed, but that's only because in patch #4 we have
> 
>   build_rsdp(tables->rsdp, tables->linker, rsdt, 0);
> 
> ie. there's only an RSDT.
> 
> Due to patch #3 however, the OVMF client breaks:
> 
>   build_rsdp(tables->rsdp, tables->linker, rsdt, xsdt);
> 
> The problem is that the "directed acyclic graph" of ADD_POINTER edges is
> no longer a tree. At least some tables can be reached on multiple paths.
> (Eg. the FADT can be reached via RSDP->RSDT-> FADT, and also
> RSDP->XSDT->FADT.)
> 
> This is a problem because EFI_ACPI_TABLE_PROTOCOL has "builtin
> knowledge" about what tables may not be installed only once vs. several
> times. Unfortunately, in this case both decisions have bad consequences:
> 
> - When FADT is passed to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for
> the second time, the function returns EFI_ACCESS_DENIED, and the
> linker/loader client bails out.
> 
> - When (eg.) the same SSDT is passed to
> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for the second time, the
> function will happily install (a copy of) it again, and then we'll end
> up with two copies of the same SSDT.
> 
> EFI_ACPI_TABLE_PROTOCOL manages both the RSDT and the XSDT internally.
> As far as I can see, it puts each table in both. The RSDT and the XSDT
> are not distinguished even on the UEFI spec level; it lumps them
> together under "RSDT/XSDT" every time.
> 
> This is probably very frustrating; sorry about that.
> 
> Laszlo

Thanks for the info!  This is worth fixing.  Can you fix this without
protocol changes, or should we change the protocol to pass a hint that a
pointer is to another instance of a previously used table?

-- 
MST

[-- Attachment #2.1: Type: text/html, Size: 4737 bytes --]

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
> I would just patch OVMF to ignore the RSDT if there is an XSDT.
> 
> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
> 
> Paolo


It does BSOD.
Skipping RSDT sounds good.

> 
> -----Original Message-----
> From: Michael S. Tsirkin [mst@redhat.com]
> Received: martedì, 09 giu 2015, 7:31
> To: Laszlo Ersek [lersek@redhat.com]
> CC: qemu-devel@nongnu.org, ghammer@redhat.com, pbonzini@redhat.com, shannon.zhao@linaro.org, imammedo@redhat.com
> Subject: Re: [PATCH v2 0/4]  acpi: xsdt support
> 
> On Tue, Jun 09, 2015 at 02:08:03AM +0200, Laszlo Ersek wrote:
> > On 06/08/15 20:14, Michael S. Tsirkin wrote:
> > > XSDT support allows using ACPI 2 features while
> > > avoiding breaking legacy windows XP guests:
> > > ACPI 2 tables are linked from XSDT only,
> > > ACPI 1 tables from both RSDT and XSDT, this way
> > > XP does not see ACPI 2 tables.
> > > 
> > > As a first step, this patchset generates v2 RSDP
> > > and fills in XSDT matching RSDT exactly.
> > > 
> > > ARM can switch to XSDT as well, I'm not bothering
> > > until there's an easy way to test that.
> > > 
> > > Note: unit test files need to be updated with this,
> > > I'm not bothering with posting them.
> > > 
> > > Changes from v1:
> > >     xsdt address is 64 bit
> > >     arm patch is now tested
> > > 
> > > Michael S. Tsirkin (4):
> > >   acpi: add API for 64 bit offsets
> > >   i386/acpi: collect 64 bit offsets for xsdt
> > >   i386/acpi: add XSDT
> > >   acpi: unify rsdp generation
> > > 
> > >  include/hw/acpi/acpi-defs.h | 15 +++++--
> > >  include/hw/acpi/aml-build.h |  7 +++-
> > >  hw/acpi/aml-build.c         | 99 +++++++++++++++++++++++++++++++++++++--------
> > >  hw/arm/virt-acpi-build.c    | 39 +++---------------
> > >  hw/i386/acpi-build.c        | 64 +++++++++++------------------
> > >  5 files changed, 129 insertions(+), 95 deletions(-)
> > > 
> > 
> > I tested this series with ArmVirtQemu (aka AAVMF) and OVMF too. (They
> > use common code in edk2.)
> > 
> > The ARM build works indeed, but that's only because in patch #4 we have
> > 
> >   build_rsdp(tables->rsdp, tables->linker, rsdt, 0);
> > 
> > ie. there's only an RSDT.
> > 
> > Due to patch #3 however, the OVMF client breaks:
> > 
> >   build_rsdp(tables->rsdp, tables->linker, rsdt, xsdt);
> > 
> > The problem is that the "directed acyclic graph" of ADD_POINTER edges is
> > no longer a tree. At least some tables can be reached on multiple paths.
> > (Eg. the FADT can be reached via RSDP->RSDT-> FADT, and also
> > RSDP->XSDT->FADT.)
> > 
> > This is a problem because EFI_ACPI_TABLE_PROTOCOL has "builtin
> > knowledge" about what tables may not be installed only once vs. several
> > times. Unfortunately, in this case both decisions have bad consequences:
> > 
> > - When FADT is passed to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for
> > the second time, the function returns EFI_ACCESS_DENIED, and the
> > linker/loader client bails out.
> > 
> > - When (eg.) the same SSDT is passed to
> > EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for the second time, the
> > function will happily install (a copy of) it again, and then we'll end
> > up with two copies of the same SSDT.
> > 
> > EFI_ACPI_TABLE_PROTOCOL manages both the RSDT and the XSDT internally.
> > As far as I can see, it puts each table in both. The RSDT and the XSDT
> > are not distinguished even on the UEFI spec level; it lumps them
> > together under "RSDT/XSDT" every time.
> > 
> > This is probably very frustrating; sorry about that.
> > 
> > Laszlo
> 
> Thanks for the info!  This is worth fixing.  Can you fix this without
> protocol changes, or should we change the protocol to pass a hint that a
> pointer is to another instance of a previously used table?
> 
> -- 
> MST

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

On Tue, Jun 09, 2015 at 08:35:38AM +0200, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
> > I would just patch OVMF to ignore the RSDT if there is an XSDT.
> > 
> > Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
> > 
> > Paolo
> 
> 
> It does BSOD.
> Skipping RSDT sounds good.

Thought a full fix would be nicer - I suspect we'll have more things like this
in the future.

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Laszlo Ersek]

On 06/09/15 08:38, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2015 at 08:35:38AM +0200, Michael S. Tsirkin wrote:
>> On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
>>> I would just patch OVMF to ignore the RSDT if there is an XSDT.
>>>
>>> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
>>>
>>> Paolo
>>
>>
>> It does BSOD.
>> Skipping RSDT sounds good.
> 
> Thought a full fix would be nicer - I suspect we'll have more things like this
> in the future.

* I think I wasn't clear enough in my email. There are two problems. The
first problem is that the current linker/loader client in OVMF passes
the same table twice to EFI_ACPI_TABLE_PROTOCOL, if this patch series is
applied to QEMU. That may be fixable, perhaps in several ways.

However, the second problem is, whatever I give EFI_ACPI_TABLE_PROTOCOL,
it will link a copy of that table into the RSDT *and* the XSDT
automatically and indivisibly. So, if you prepare an SSDT with the
opcode that causes XP to BSOD and link that only into the XSDT, then
after OVMF passes it to EFI_ACPI_TABLE_PROTOCOL (only once), the guest
OS will find it in the RSDT and the XSDT both.

On the other hand XP is not a UEFI operating system, so this might not
matter.

* Not sure what is meant by "skipping RSDT". I'm only following
ADD_POINTER commands. Those commands say "increment the UINT[1248] value
at offset foo in *blob* bar, with the start address of blob baz". I have
no clue what table the incremented pointer lives in, I can only check
what table it (potentially) points to. I already omit passing the RSDT
to EFI_ACPI_TABLE_PROTOCOL when I detect it on the *target* side of a
pointer, but I can't recognize it on the source side.

There is no actual graph traversal in place; I only mentioned the DAG
for illustration. The direct reason is that there are two ADD_POINTER
commands (and two source locations to be incremented, respectively) for
each particular pointed-to (blob, offset).

* What does a full fix mean? ... I'm not sure this is worth the churn.
Same as the user is expected not to boot XP with OVMF as the firmware
(which is a command line / libvirt configuration question), he could
just as well be expected not to configure vmgenid for XP.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

On Tue, Jun 09, 2015 at 09:41:20AM +0200, Laszlo Ersek wrote:
> On 06/09/15 08:38, Michael S. Tsirkin wrote:
> > On Tue, Jun 09, 2015 at 08:35:38AM +0200, Michael S. Tsirkin wrote:
> >> On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
> >>> I would just patch OVMF to ignore the RSDT if there is an XSDT.
> >>>
> >>> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
> >>>
> >>> Paolo
> >>
> >>
> >> It does BSOD.
> >> Skipping RSDT sounds good.
> > 
> > Thought a full fix would be nicer - I suspect we'll have more things like this
> > in the future.
> 
> * I think I wasn't clear enough in my email. There are two problems. The
> first problem is that the current linker/loader client in OVMF passes
> the same table twice to EFI_ACPI_TABLE_PROTOCOL, if this patch series is
> applied to QEMU. That may be fixable, perhaps in several ways.
> 
> However, the second problem is, whatever I give EFI_ACPI_TABLE_PROTOCOL,
> it will link a copy of that table into the RSDT *and* the XSDT
> automatically and indivisibly. So, if you prepare an SSDT with the
> opcode that causes XP to BSOD and link that only into the XSDT, then
> after OVMF passes it to EFI_ACPI_TABLE_PROTOCOL (only once), the guest
> OS will find it in the RSDT and the XSDT both.
> 
> On the other hand XP is not a UEFI operating system, so this might not
> matter.

That's what I was going to ask. Can XP boot on UEFI somehow?

> * Not sure what is meant by "skipping RSDT". I'm only following
> ADD_POINTER commands. Those commands say "increment the UINT[1248] value
> at offset foo in *blob* bar, with the start address of blob baz". I have
> no clue what table the incremented pointer lives in, I can only check
> what table it (potentially) points to. I already omit passing the RSDT
> to EFI_ACPI_TABLE_PROTOCOL when I detect it on the *target* side of a
> pointer, but I can't recognize it on the source side.
> 
> There is no actual graph traversal in place; I only mentioned the DAG
> for illustration. The direct reason is that there are two ADD_POINTER
> commands (and two source locations to be incremented, respectively) for
> each particular pointed-to (blob, offset).
> 
> * What does a full fix mean? ... I'm not sure this is worth the churn.
> Same as the user is expected not to boot XP with OVMF as the firmware
> (which is a command line / libvirt configuration question), he could
> just as well be expected not to configure vmgenid for XP.
> 
> Thanks
> Laszlo

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Laszlo Ersek]

On 06/09/15 10:34, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2015 at 09:41:20AM +0200, Laszlo Ersek wrote:
>> On 06/09/15 08:38, Michael S. Tsirkin wrote:
>>> On Tue, Jun 09, 2015 at 08:35:38AM +0200, Michael S. Tsirkin wrote:
>>>> On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
>>>>> I would just patch OVMF to ignore the RSDT if there is an XSDT.
>>>>>
>>>>> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
>>>>>
>>>>> Paolo
>>>>
>>>>
>>>> It does BSOD.
>>>> Skipping RSDT sounds good.
>>>
>>> Thought a full fix would be nicer - I suspect we'll have more things like this
>>> in the future.
>>
>> * I think I wasn't clear enough in my email. There are two problems. The
>> first problem is that the current linker/loader client in OVMF passes
>> the same table twice to EFI_ACPI_TABLE_PROTOCOL, if this patch series is
>> applied to QEMU. That may be fixable, perhaps in several ways.
>>
>> However, the second problem is, whatever I give EFI_ACPI_TABLE_PROTOCOL,
>> it will link a copy of that table into the RSDT *and* the XSDT
>> automatically and indivisibly. So, if you prepare an SSDT with the
>> opcode that causes XP to BSOD and link that only into the XSDT, then
>> after OVMF passes it to EFI_ACPI_TABLE_PROTOCOL (only once), the guest
>> OS will find it in the RSDT and the XSDT both.
>>
>> On the other hand XP is not a UEFI operating system, so this might not
>> matter.
> 
> That's what I was going to ask. Can XP boot on UEFI somehow?

Only if OVMF is built with the SeaBIOS Compatibility Support Module. But
then it still qualifies as a legacy boot. (Ie. the OS will still exist
in a legacy BIOS environment.)

A CSM build of OVMF is available in Gerd's repo, but the internal
UEFI-to-CSM interface is nightmarish enough that I wasn't willing to
support it in RHEL, so we won't do that. The argument being, if you need
a legacy OS, just pick SeaBIOS as your guest firmware; in virt it's
easy to choose your firmware.

Thanks
Laszlo

> 
>> * Not sure what is meant by "skipping RSDT". I'm only following
>> ADD_POINTER commands. Those commands say "increment the UINT[1248] value
>> at offset foo in *blob* bar, with the start address of blob baz". I have
>> no clue what table the incremented pointer lives in, I can only check
>> what table it (potentially) points to. I already omit passing the RSDT
>> to EFI_ACPI_TABLE_PROTOCOL when I detect it on the *target* side of a
>> pointer, but I can't recognize it on the source side.
>>
>> There is no actual graph traversal in place; I only mentioned the DAG
>> for illustration. The direct reason is that there are two ADD_POINTER
>> commands (and two source locations to be incremented, respectively) for
>> each particular pointed-to (blob, offset).
>>
>> * What does a full fix mean? ... I'm not sure this is worth the churn.
>> Same as the user is expected not to boot XP with OVMF as the firmware
>> (which is a command line / libvirt configuration question), he could
>> just as well be expected not to configure vmgenid for XP.
>>
>> Thanks
>> Laszlo
> 

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Igor Mammedov]

On Tue, 9 Jun 2015 08:35:38 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
> > I would just patch OVMF to ignore the RSDT if there is an XSDT.
> > 
> > Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
> > 
> > Paolo
> 
> 
> It does BSOD.
We've had PCI0._CRS method with invalid for XP opcode and
it didn't BSOD unless execution reached it. (it was 64-bit PCI window if I recall right)

> Skipping RSDT sounds good.
>
> > 
> > -----Original Message-----
> > From: Michael S. Tsirkin [mst@redhat.com]
> > Received: martedì, 09 giu 2015, 7:31
> > To: Laszlo Ersek [lersek@redhat.com]
> > CC: qemu-devel@nongnu.org, ghammer@redhat.com, pbonzini@redhat.com, shannon.zhao@linaro.org, imammedo@redhat.com
> > Subject: Re: [PATCH v2 0/4]  acpi: xsdt support
> > 
> > On Tue, Jun 09, 2015 at 02:08:03AM +0200, Laszlo Ersek wrote:
> > > On 06/08/15 20:14, Michael S. Tsirkin wrote:
> > > > XSDT support allows using ACPI 2 features while
> > > > avoiding breaking legacy windows XP guests:
> > > > ACPI 2 tables are linked from XSDT only,
> > > > ACPI 1 tables from both RSDT and XSDT, this way
> > > > XP does not see ACPI 2 tables.
> > > > 
> > > > As a first step, this patchset generates v2 RSDP
> > > > and fills in XSDT matching RSDT exactly.
> > > > 
> > > > ARM can switch to XSDT as well, I'm not bothering
> > > > until there's an easy way to test that.
> > > > 
> > > > Note: unit test files need to be updated with this,
> > > > I'm not bothering with posting them.
> > > > 
> > > > Changes from v1:
> > > >     xsdt address is 64 bit
> > > >     arm patch is now tested
> > > > 
> > > > Michael S. Tsirkin (4):
> > > >   acpi: add API for 64 bit offsets
> > > >   i386/acpi: collect 64 bit offsets for xsdt
> > > >   i386/acpi: add XSDT
> > > >   acpi: unify rsdp generation
> > > > 
> > > >  include/hw/acpi/acpi-defs.h | 15 +++++--
> > > >  include/hw/acpi/aml-build.h |  7 +++-
> > > >  hw/acpi/aml-build.c         | 99 +++++++++++++++++++++++++++++++++++++--------
> > > >  hw/arm/virt-acpi-build.c    | 39 +++---------------
> > > >  hw/i386/acpi-build.c        | 64 +++++++++++------------------
> > > >  5 files changed, 129 insertions(+), 95 deletions(-)
> > > > 
> > > 
> > > I tested this series with ArmVirtQemu (aka AAVMF) and OVMF too. (They
> > > use common code in edk2.)
> > > 
> > > The ARM build works indeed, but that's only because in patch #4 we have
> > > 
> > >   build_rsdp(tables->rsdp, tables->linker, rsdt, 0);
> > > 
> > > ie. there's only an RSDT.
> > > 
> > > Due to patch #3 however, the OVMF client breaks:
> > > 
> > >   build_rsdp(tables->rsdp, tables->linker, rsdt, xsdt);
> > > 
> > > The problem is that the "directed acyclic graph" of ADD_POINTER edges is
> > > no longer a tree. At least some tables can be reached on multiple paths.
> > > (Eg. the FADT can be reached via RSDP->RSDT-> FADT, and also
> > > RSDP->XSDT->FADT.)
> > > 
> > > This is a problem because EFI_ACPI_TABLE_PROTOCOL has "builtin
> > > knowledge" about what tables may not be installed only once vs. several
> > > times. Unfortunately, in this case both decisions have bad consequences:
> > > 
> > > - When FADT is passed to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for
> > > the second time, the function returns EFI_ACCESS_DENIED, and the
> > > linker/loader client bails out.
> > > 
> > > - When (eg.) the same SSDT is passed to
> > > EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() for the second time, the
> > > function will happily install (a copy of) it again, and then we'll end
> > > up with two copies of the same SSDT.
> > > 
> > > EFI_ACPI_TABLE_PROTOCOL manages both the RSDT and the XSDT internally.
> > > As far as I can see, it puts each table in both. The RSDT and the XSDT
> > > are not distinguished even on the UEFI spec level; it lumps them
> > > together under "RSDT/XSDT" every time.
> > > 
> > > This is probably very frustrating; sorry about that.
> > > 
> > > Laszlo
> > 
> > Thanks for the info!  This is worth fixing.  Can you fix this without
> > protocol changes, or should we change the protocol to pass a hint that a
> > pointer is to another instance of a previously used table?
> > 
> > -- 
> > MST
> 

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
> I would just patch OVMF to ignore the RSDT if there is an XSDT.
> 
> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
> 
> Paolo

Oh I think I misunderstood what you were saying.
First I think you were thinking of _REV, not _OSI.

Second, yes, I think the non-taken branch is not evaluated
for invalid opcodes. I will test this now.

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
> I would just patch OVMF to ignore the RSDT if there is an XSDT.
> 
> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
> 
> Paolo


So it's even easier. The following doesn't crash XP:

            Method(ADDR, 0, Serialized) {
		/* Region is local to method to avoid crashing ACPI 1.0 guests */
                DataTableRegion(IDPT, "UEFI", "BXPC", "VMGENI");
	        Field (IDPT, AnyAcc, NoLock, Preserve) {
		  Offset (54),
		  VGIA, 64     // address of "etc/acpi/vmgenid" blob
	        }
		Return(Add(VGIA), 40);
            }

Simply because XP doesn't ever call ADDR.
Method must be serialized since attempts to create two
regions or fields with the same name would crash OSPM.


-- 
MST

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Laszlo Ersek]

On 06/09/15 11:49, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
>> I would just patch OVMF to ignore the RSDT if there is an XSDT.
>>
>> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
>>
>> Paolo
> 
> 
> So it's even easier. The following doesn't crash XP:
> 
>             Method(ADDR, 0, Serialized) {
> 		/* Region is local to method to avoid crashing ACPI 1.0 guests */
>                 DataTableRegion(IDPT, "UEFI", "BXPC", "VMGENI");
> 	        Field (IDPT, AnyAcc, NoLock, Preserve) {
> 		  Offset (54),
> 		  VGIA, 64     // address of "etc/acpi/vmgenid" blob
> 	        }
> 		Return(Add(VGIA), 40);
>             }
> 
> Simply because XP doesn't ever call ADDR.
> Method must be serialized since attempts to create two
> regions or fields with the same name would crash OSPM.

That sounds like a huge relief to me, so thank you for researching it.

Laszlo

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

On Tue, Jun 09, 2015 at 04:02:08PM +0200, Laszlo Ersek wrote:
> On 06/09/15 11:49, Michael S. Tsirkin wrote:
> > On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
> >> I would just patch OVMF to ignore the RSDT if there is an XSDT.
> >>
> >> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
> >>
> >> Paolo
> > 
> > 
> > So it's even easier. The following doesn't crash XP:
> > 
> >             Method(ADDR, 0, Serialized) {
> > 		/* Region is local to method to avoid crashing ACPI 1.0 guests */
> >                 DataTableRegion(IDPT, "UEFI", "BXPC", "VMGENI");
> > 	        Field (IDPT, AnyAcc, NoLock, Preserve) {
> > 		  Offset (54),
> > 		  VGIA, 64     // address of "etc/acpi/vmgenid" blob
> > 	        }
> > 		Return(Add(VGIA), 40);
> >             }
> > 
> > Simply because XP doesn't ever call ADDR.
> > Method must be serialized since attempts to create two
> > regions or fields with the same name would crash OSPM.
> 
> That sounds like a huge relief to me, so thank you for researching it.
> 
> Laszlo

Mind you, I still think it's worth it to support XSDT eventually,
but the immediate need to do this in 2.4 timeframe is gone.

-- 
MST

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Laszlo Ersek]

On 06/09/15 16:05, Michael S. Tsirkin wrote:
> On Tue, Jun 09, 2015 at 04:02:08PM +0200, Laszlo Ersek wrote:
>> On 06/09/15 11:49, Michael S. Tsirkin wrote:
>>> On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
>>>> I would just patch OVMF to ignore the RSDT if there is an XSDT.
>>>>
>>>> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
>>>>
>>>> Paolo
>>>
>>>
>>> So it's even easier. The following doesn't crash XP:
>>>
>>>             Method(ADDR, 0, Serialized) {
>>> 		/* Region is local to method to avoid crashing ACPI 1.0 guests */
>>>                 DataTableRegion(IDPT, "UEFI", "BXPC", "VMGENI");
>>> 	        Field (IDPT, AnyAcc, NoLock, Preserve) {
>>> 		  Offset (54),
>>> 		  VGIA, 64     // address of "etc/acpi/vmgenid" blob
>>> 	        }
>>> 		Return(Add(VGIA), 40);
>>>             }
>>>
>>> Simply because XP doesn't ever call ADDR.
>>> Method must be serialized since attempts to create two
>>> regions or fields with the same name would crash OSPM.
>>
>> That sounds like a huge relief to me, so thank you for researching it.
>>
>> Laszlo
> 
> Mind you, I still think it's worth it to support XSDT eventually,
> but the immediate need to do this in 2.4 timeframe is gone.

... I'll have to make the OVMF linker/loader a little bit smarter, so
that this QEMU patch series doesn't break it. Because, unfortunately,
XSDT seems to be a hard requirement for DataTableRegion(), according to
the ACPI spec. (Thus, unless SeaBIOS installs an XSDT too,
DataTableRegion() may not work in the ADDR method of the vmgenid device.)

(FWIW I checked the ACPICA source, and AFAICS, DataTableRegion() *of
ACPICA* should locate tables from the RSDT too just fine. But, IIRC,
Windows does not use ACPICA, and the ACPI spec requires XSDT. Quite
unfortunate.)

The idea I have for OVMF is to track the offsets for each blob from
which tables have been installed already. Then I can detect further
pointers (located in any other blobs) pointing to the same offsets, and
forego duplicate table installations. This should fix the problem I
reported in this thread, and then this patch set should work for OVMF too.

... I've come across this today while writing "docs/specs/vmgenid.txt".

Laszlo

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Michael S. Tsirkin]

On Thu, Aug 27, 2015 at 04:27:00PM +0200, Laszlo Ersek wrote:
> On 06/09/15 16:05, Michael S. Tsirkin wrote:
> > On Tue, Jun 09, 2015 at 04:02:08PM +0200, Laszlo Ersek wrote:
> >> On 06/09/15 11:49, Michael S. Tsirkin wrote:
> >>> On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
> >>>> I would just patch OVMF to ignore the RSDT if there is an XSDT.
> >>>>
> >>>> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
> >>>>
> >>>> Paolo
> >>>
> >>>
> >>> So it's even easier. The following doesn't crash XP:
> >>>
> >>>             Method(ADDR, 0, Serialized) {
> >>> 		/* Region is local to method to avoid crashing ACPI 1.0 guests */
> >>>                 DataTableRegion(IDPT, "UEFI", "BXPC", "VMGENI");
> >>> 	        Field (IDPT, AnyAcc, NoLock, Preserve) {
> >>> 		  Offset (54),
> >>> 		  VGIA, 64     // address of "etc/acpi/vmgenid" blob
> >>> 	        }
> >>> 		Return(Add(VGIA), 40);
> >>>             }
> >>>
> >>> Simply because XP doesn't ever call ADDR.
> >>> Method must be serialized since attempts to create two
> >>> regions or fields with the same name would crash OSPM.
> >>
> >> That sounds like a huge relief to me, so thank you for researching it.
> >>
> >> Laszlo
> > 
> > Mind you, I still think it's worth it to support XSDT eventually,
> > but the immediate need to do this in 2.4 timeframe is gone.
> 
> ... I'll have to make the OVMF linker/loader a little bit smarter, so
> that this QEMU patch series doesn't break it. Because, unfortunately,
> XSDT seems to be a hard requirement for DataTableRegion(), according to
> the ACPI spec. (Thus, unless SeaBIOS installs an XSDT too,
> DataTableRegion() may not work in the ADDR method of the vmgenid device.)

That's not really true. ACPI is simply pushing XSDT everywhere,
you will find it hardly ever mentions legacy tables like RSDP anymore.

And in practice, this works fine with all existing guests.


So yes let's do it, but no, don't make this a dependency of
this series.


> (FWIW I checked the ACPICA source, and AFAICS, DataTableRegion() *of
> ACPICA* should locate tables from the RSDT too just fine. But, IIRC,
> Windows does not use ACPICA, and the ACPI spec requires XSDT. Quite
> unfortunate.)
> 
> The idea I have for OVMF is to track the offsets for each blob from
> which tables have been installed already. Then I can detect further
> pointers (located in any other blobs) pointing to the same offsets, and
> forego duplicate table installations. This should fix the problem I
> reported in this thread, and then this patch set should work for OVMF too.
> 
> ... I've come across this today while writing "docs/specs/vmgenid.txt".
> 
> Laszlo

Re: [Qemu-devel] [PATCH v2 0/4] acpi: xsdt support

[Laszlo Ersek]

On 08/27/15 19:58, Michael S. Tsirkin wrote:
> On Thu, Aug 27, 2015 at 04:27:00PM +0200, Laszlo Ersek wrote:
>> On 06/09/15 16:05, Michael S. Tsirkin wrote:
>>> On Tue, Jun 09, 2015 at 04:02:08PM +0200, Laszlo Ersek wrote:
>>>> On 06/09/15 11:49, Michael S. Tsirkin wrote:
>>>>> On Tue, Jun 09, 2015 at 02:02:31AM -0400, Paolo Bonzini wrote:
>>>>>> I would just patch OVMF to ignore the RSDT if there is an XSDT.
>>>>>>
>>>>>> Alternatively, can you check for ACPI 2.0 support via _OSI, and load the ACPI 2.0 bits via LoadTable? Hopefully XP does not BSOD if the invalid (for ACPI 1.0) opcodes are in a Then block or in a separate method... Then you can use just an RSDT.
>>>>>>
>>>>>> Paolo
>>>>>
>>>>>
>>>>> So it's even easier. The following doesn't crash XP:
>>>>>
>>>>>             Method(ADDR, 0, Serialized) {
>>>>> 		/* Region is local to method to avoid crashing ACPI 1.0 guests */
>>>>>                 DataTableRegion(IDPT, "UEFI", "BXPC", "VMGENI");
>>>>> 	        Field (IDPT, AnyAcc, NoLock, Preserve) {
>>>>> 		  Offset (54),
>>>>> 		  VGIA, 64     // address of "etc/acpi/vmgenid" blob
>>>>> 	        }
>>>>> 		Return(Add(VGIA), 40);
>>>>>             }
>>>>>
>>>>> Simply because XP doesn't ever call ADDR.
>>>>> Method must be serialized since attempts to create two
>>>>> regions or fields with the same name would crash OSPM.
>>>>
>>>> That sounds like a huge relief to me, so thank you for researching it.
>>>>
>>>> Laszlo
>>>
>>> Mind you, I still think it's worth it to support XSDT eventually,
>>> but the immediate need to do this in 2.4 timeframe is gone.
>>
>> ... I'll have to make the OVMF linker/loader a little bit smarter, so
>> that this QEMU patch series doesn't break it. Because, unfortunately,
>> XSDT seems to be a hard requirement for DataTableRegion(), according to
>> the ACPI spec. (Thus, unless SeaBIOS installs an XSDT too,
>> DataTableRegion() may not work in the ADDR method of the vmgenid device.)
> 
> That's not really true. ACPI is simply pushing XSDT everywhere,
> you will find it hardly ever mentions legacy tables like RSDP anymore.
> 
> And in practice, this works fine with all existing guests.
> 
> 
> So yes let's do it, but no, don't make this a dependency of
> this series.

Okay, thanks. I made a note for later.
Laszlo

>> (FWIW I checked the ACPICA source, and AFAICS, DataTableRegion() *of
>> ACPICA* should locate tables from the RSDT too just fine. But, IIRC,
>> Windows does not use ACPICA, and the ACPI spec requires XSDT. Quite
>> unfortunate.)
>>
>> The idea I have for OVMF is to track the offsets for each blob from
>> which tables have been installed already. Then I can detect further
>> pointers (located in any other blobs) pointing to the same offsets, and
>> forego duplicate table installations. This should fix the problem I
>> reported in this thread, and then this patch set should work for OVMF too.
>>
>> ... I've come across this today while writing "docs/specs/vmgenid.txt".
>>
>> Laszlo