Re: [SeaBIOS] [Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Paolo Bonzini]

On 25/07/2017 18:23, Paolo Bonzini wrote:
> On 25/07/2017 18:14, Laszlo Ersek wrote:
> > "No regressions became apparent in tests with a range of Windows
> >    (XP-10)"
> >
> > In theory, w2k falls within that range.
>
> Nope, Windows 2000 is like NT 5.0, XP is like NT 5.1. :(
>
> One possibility is to fix it in SeaBIOS instead: if you get a 2.0 FADT
> and an XSDT and no RSDT, it can build an RSDT and a 1.0 FADT itself,
> patching the RSDT to point to it.
>
> It's a hack, but it's the only place I see to make it "just work".  And
> it could be extended nicely in the future.
>
> All QEMU would have to do is to provide an XSDT _instead_ of an RSDT.

Completely untested code follows.  Machine type compatibility code 
should not be necessary because new QEMU always starts with a new BIOS.

Not sure I'll have much time for this in the next few days, so help
is appreciated.

Paolo

QEMU:

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 36a6cc450e..ea750d54d9 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1573,33 +1573,6 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
     g_array_free(tables->vmgenid, mfre);
 }

-/* Build rsdt table */
-void
-build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
-           const char *oem_id, const char *oem_table_id)
-{
-    int i;
-    unsigned rsdt_entries_offset;
-    AcpiRsdtDescriptorRev1 *rsdt;
-    const unsigned table_data_len = (sizeof(uint32_t) * table_offsets->len);
-    const unsigned rsdt_entry_size = sizeof(rsdt->table_offset_entry[0]);
-    const size_t rsdt_len = sizeof(*rsdt) + table_data_len;
-
-    rsdt = acpi_data_push(table_data, rsdt_len);
-    rsdt_entries_offset = (char *)rsdt->table_offset_entry - table_data->data;
-    for (i = 0; i < table_offsets->len; ++i) {
-        uint32_t ref_tbl_offset = g_array_index(table_offsets, uint32_t, i);
-        uint32_t rsdt_entry_offset = rsdt_entries_offset + rsdt_entry_size * i;
-
-        /* rsdt->table_offset_entry to be filled by Guest linker */
-        bios_linker_loader_add_pointer(linker,
-            ACPI_BUILD_TABLE_FILE, rsdt_entry_offset, rsdt_entry_size,
-            ACPI_BUILD_TABLE_FILE, ref_tbl_offset);
-    }
-    build_header(linker, table_data,
-                 (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id);
-}
-
 /* Build xsdt table */
 void
 build_xsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6b7bade183..ad00f6affd 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2557,12 +2557,12 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
 }

static GArray *
-build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
+build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
 {
     AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
-    unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
-    unsigned rsdt_pa_offset =
-        (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
+    unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
+    unsigned xsdt_pa_offset =
+        (char *)&rsdp->xsdt_physical_address - rsdp_table->data;

bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
                              true /* fseg memory */);
@@ -2571,8 +2571,8 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
     memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
     /* Address to be filled by Guest linker */
     bios_linker_loader_add_pointer(linker,
-        ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
-        ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
+        ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
+        ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);

/* Checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
@@ -2621,7 +2621,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     PCMachineState *pcms = PC_MACHINE(machine);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     GArray *table_offsets;
-    unsigned facs, dsdt, rsdt, fadt;
+    unsigned facs, dsdt, xsdt, fadt;
     AcpiPmInfo pm;
     AcpiMiscInfo misc;
     AcpiMcfgInfo mcfg;
@@ -2730,12 +2730,12 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     }

/* RSDT is pointed to by RSDP */
-    rsdt = tables_blob->len;
-    build_rsdt(tables_blob, tables->linker, table_offsets,
+    xsdt = tables_blob->len;
+    build_xsdt(tables_blob, tables->linker, table_offsets,
                slic_oem.id, slic_oem.table_id);

/* RSDP is in FSEG memory, so allocate it separately */
-    build_rsdp(tables->rsdp, tables->linker, rsdt);
+    build_rsdp(tables->rsdp, tables->linker, xsdt);

/* We'll expose it all to Guest so we want to reduce
      * chance of size changes.

SeaBIOS:
> From 73b0facdb7349f5dbf24dc675647b68eeeec0ff4 Mon Sep 17 00:00:00 2001

From: Paolo Bonzini 
Date: Tue, 25 Jul 2017 18:50:19 +0200
Subject: [PATCH 1/2] seabios: build RSDT from XSDT

Old operating systems would like to have a v1 FADT, but new
operating systems would like to have v3.

Since old operating systems do not know about XSDTs, the
solution is to point the RSDT to a v1 FADT and the XSDT to a
v3 FADT.

But, OVMF is not able to do that and barfs when it sees the
second FADT---plus really it's only BIOS operating systems
such as win2k that complain.  So instead: 1) make QEMU provide
an XSDT only; 2) build the RSDT and v1 FADT in SeaBIOS.

This patch makes SeaBIOS build an RSDT out of an existing XSDT.

Signed-off-by: Paolo Bonzini 
---
 src/fw/paravirt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 src/std/acpi.h    | 11 +++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
index 5b23d78..19a4abe 100644
--- a/src/fw/paravirt.c
+++ b/src/fw/paravirt.c
@@ -25,6 +25,7 @@
 #include "x86.h" // cpuid
 #include "xen.h" // xen_biostable_setup
 #include "stacks.h" // yield
+#include "std/acpi.h"

// Amount of continuous ram under 4Gig
 u32 RamSize;
@@ -147,6 +148,46 @@ static void msr_feature_control_setup(void)
         wrmsr_smp(MSR_IA32_FEATURE_CONTROL, feature_control_bits);
 }

+static void
+build_compatibility_rsdt(void)
+{
+    if (RsdpAddr->rsdt_physical_address)
+        return;
+
+    u64 xsdt_addr = RsdpAddr->xsdt_physical_address;
+    if (xsdt_addr & ~0xffffffffULL)
+        return;
+
+    struct xsdt_descriptor_rev1 *xsdt = (void*)(u32)xsdt_addr;
+    void *end = (void*)xsdt + xsdt->length;
+    struct rsdt_descriptor_rev1 *rsdt;
+    int rsdt_size = offsetof(struct rsdt_descriptor_rev1, table_offset_entry[0]);
+    int i;
+    for (i=0; (void*)&xsdt->table_offset_entry[i] < end; i++) {
+        u64 tbl_addr = xsdt->table_offset_entry[i];
+        if (!tbl_addr || (tbl_addr & ~0xffffffffULL))
+            continue;
+        rsdt_size += 4;
+    }
+
+    rsdt = malloc_high(rsdt_size);
+    RsdpAddr->rsdt_physical_address = (u32)rsdt;
+
+    memcpy(rsdt, xsdt, sizeof(struct acpi_table_header));
+    rsdt->signature = RSDT_SIGNATURE;
+    rsdt->length = rsdt_size;
+    rsdt->revision = 1;
+    int j;
+    for (i=j=0; (void*)&xsdt->table_offset_entry[i] < end; i++) {
+        u64 tbl_addr = xsdt->table_offset_entry[i];
+        if (!tbl_addr || (tbl_addr & ~0xffffffffULL))
+            continue;
+        rsdt->table_offset_entry[j++] = (u32)tbl_addr;
+    }
+
+    rsdt->checksum -= checksum(rsdt, rsdt_size);
+}
+
 void
 qemu_platform_setup(void)
 {
@@ -186,8 +227,10 @@ qemu_platform_setup(void)

RsdpAddr = find_acpi_rsdp();

-        if (RsdpAddr)
+        if (RsdpAddr) {
+            build_compatibility_rsdt();
             return;
+        }

/* If present, loader should have installed an RSDP.
          * Not installed? We might still be able to continue
diff --git a/src/std/acpi.h b/src/std/acpi.h
index c2ea707..caf5e7e 100644
--- a/src/std/acpi.h
+++ b/src/std/acpi.h
@@ -133,6 +133,17 @@ struct rsdt_descriptor_rev1
 } PACKED;

/*
+ * ACPI 2.0 Root System Description Table (RSDT)
+ */
+#define XSDT_SIGNATURE 0x54445358 // RSDT
+struct xsdt_descriptor_rev1
+{
+    ACPI_TABLE_HEADER_DEF       /* ACPI common table header */
+    u64 table_offset_entry[0];  /* Array of pointers to other */
+    /* ACPI tables */
+} PACKED;
+
+/*
  * ACPI 1.0 Firmware ACPI Control Structure (FACS)
  */
 #define FACS_SIGNATURE 0x53434146 // FACS

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Phil Dennis-Jordan]

Thanks for this, Paolo. Very interesting idea.

I couldn't get things working initially, but with a few fixups on the
SeaBIOS side I can boot both legacy and modern OSes. See comments
inline below for details on changes required.

Successfully booted (only a brief test):
- Windows 2000
- Windows XP (32 bit)
- Windows 7 (32 bit)
- Windows 10 (64 bit, SeaBIOS)
- Windows 10 (64 bit, OVMF)
- macOS 10.12 (patched OVMF)

On Tue, Jul 25, 2017 at 7:10 PM, Paolo Bonzini  wrote:
> On 25/07/2017 18:23, Paolo Bonzini wrote:
> > On 25/07/2017 18:14, Laszlo Ersek wrote:
> > > "No regressions became apparent in tests with a range of Windows
> > >    (XP-10)"
> > >
> > > In theory, w2k falls within that range.
> >
> > Nope, Windows 2000 is like NT 5.0, XP is like NT 5.1. :(
> >
> > One possibility is to fix it in SeaBIOS instead: if you get a 2.0 FADT
> > and an XSDT and no RSDT, it can build an RSDT and a 1.0 FADT itself,
> > patching the RSDT to point to it.
> >
> > It's a hack, but it's the only place I see to make it "just work".  And
> > it could be extended nicely in the future.
> >
> > All QEMU would have to do is to provide an XSDT _instead_ of an RSDT.
>
> Completely untested code follows.  Machine type compatibility code
> should not be necessary because new QEMU always starts with a new BIOS.
>
> Not sure I'll have much time for this in the next few days, so help
> is appreciated.
>
> Paolo
>
> QEMU:
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 36a6cc450e..ea750d54d9 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1573,33 +1573,6 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
>      g_array_free(tables->vmgenid, mfre);
>  }
>
> -/* Build rsdt table */
> -void
> -build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
>       const char *oem_id, const char *oem_table_id)
>
> -{
> int i;
> unsigned rsdt_entries_offset;
> AcpiRsdtDescriptorRev1 *rsdt;
> const unsigned table_data_len = (sizeof(uint32_t) * table_offsets->len);
> const unsigned rsdt_entry_size = sizeof(rsdt->table_offset_entry[0]);
> const size_t rsdt_len = sizeof(*rsdt) + table_data_len;
> rsdt = acpi_data_push(table_data, rsdt_len);
> rsdt_entries_offset = (char *)rsdt->table_offset_entry - table_data->data;
> for (i = 0; i < table_offsets->len; ++i) {
>    uint32_t ref_tbl_offset = g_array_index(table_offsets, uint32_t, i);
>    uint32_t rsdt_entry_offset = rsdt_entries_offset + rsdt_entry_size * i;
>    /* rsdt->table_offset_entry to be filled by Guest linker */
>    bios_linker_loader_add_pointer(linker,
>        ACPI_BUILD_TABLE_FILE, rsdt_entry_offset, rsdt_entry_size,
>        ACPI_BUILD_TABLE_FILE, ref_tbl_offset);
> }
> build_header(linker, table_data,
>             (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id);
>
> -}
>
> /* Build xsdt table */
>  void
>  build_xsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6b7bade183..ad00f6affd 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2557,12 +2557,12 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
>  }
>
> static GArray *
> -build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
> +build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_tbl_offset)
>  {
>      AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
> unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
> unsigned rsdt_pa_offset =
>    (char *)&rsdp->rsdt_physical_address - rsdp_table->data;
>
> unsigned xsdt_pa_size = sizeof(rsdp->xsdt_physical_address);
>
> unsigned xsdt_pa_offset =
>    (char *)&rsdp->xsdt_physical_address - rsdp_table->data;
>
> bios_linker_loader_alloc(linker, ACPI_BUILD_RSDP_FILE, rsdp_table, 16,
>                             true /* fseg memory */);
>
> @@ -2571,8 +2571,8 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
>      memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, 6);
>      /* Address to be filled by Guest linker */
>      bios_linker_loader_add_pointer(linker,
>    ACPI_BUILD_RSDP_FILE, rsdt_pa_offset, rsdt_pa_size,
>    ACPI_BUILD_TABLE_FILE, rsdt_tbl_offset);
>    ACPI_BUILD_RSDP_FILE, xsdt_pa_offset, xsdt_pa_size,
>    ACPI_BUILD_TABLE_FILE, xsdt_tbl_offset);
>
> /* Checksum to be filled by Guest linker */
>    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
>
> @@ -2621,7 +2621,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>      PCMachineState *pcms = PC_MACHINE(machine);
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>      GArray *table_offsets;
> unsigned facs, dsdt, rsdt, fadt;
> unsigned facs, dsdt, xsdt, fadt;
> AcpiPmInfo pm;
> AcpiMiscInfo misc;
> AcpiMcfgInfo mcfg;
>
> @@ -2730,12 +2730,12 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>      }
>  /* RSDT is pointed to by RSDP */
> rsdt = tables_blob->len;
> build_rsdt(tables_blob, tables->linker, table_offsets,
>
> xsdt = tables_blob->len;
>
> build_xsdt(tables_blob, tables->linker, table_offsets,
>            slic_oem.id, slic_oem.table_id);
>
> /* RSDP is in FSEG memory, so allocate it separately */
> build_rsdp(tables->rsdp, tables->linker, rsdt);
>
> build_rsdp(tables->rsdp, tables->linker, xsdt);
>
> /* We'll expose it all to Guest so we want to reduce
> chance of size changes.
>
> SeaBIOS:
>
> From 73b0facdb7349f5dbf24dc675647b68eeeec0ff4 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini 
> Date: Tue, 25 Jul 2017 18:50:19 +0200
> Subject: [PATCH 1/2] seabios: build RSDT from XSDT
>
> Old operating systems would like to have a v1 FADT, but new
> operating systems would like to have v3.
>
> Since old operating systems do not know about XSDTs, the
> solution is to point the RSDT to a v1 FADT and the XSDT to a
> v3 FADT.
>
> But, OVMF is not able to do that and barfs when it sees the
> second FADT---plus really it's only BIOS operating systems
> such as win2k that complain.  So instead: 1) make QEMU provide
> an XSDT only; 2) build the RSDT and v1 FADT in SeaBIOS.
>
> This patch makes SeaBIOS build an RSDT out of an existing XSDT.
> Signed-off-by: Paolo Bonzini
>
> src/fw/paravirt.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  src/std/acpi.h    | 11 +++++++++++
>  2 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 5b23d78..19a4abe 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -25,6 +25,7 @@
>  #include "x86.h" // cpuid
>  #include "xen.h" // xen_biostable_setup
>  #include "stacks.h" // yield
> +#include "std/acpi.h"
>
> // Amount of continuous ram under 4Gig
>  u32 RamSize;
> @@ -147,6 +148,46 @@ static void msr_feature_control_setup(void)
>          wrmsr_smp(MSR_IA32_FEATURE_CONTROL, feature_control_bits);
>  }
>
> +static void
> +build_compatibility_rsdt(void)
> +{
> if (RsdpAddr->rsdt_physical_address)
>    return;
> u64 xsdt_addr = RsdpAddr->xsdt_physical_address;
> if (xsdt_addr & ~0xffffffffULL)
>    return;
> struct xsdt_descriptor_rev1 *xsdt = (void*)(u32)xsdt_addr;
> void *end = (void*)xsdt + xsdt->length;
> struct rsdt_descriptor_rev1 *rsdt;
> int rsdt_size = offsetof(struct rsdt_descriptor_rev1, table_offset_entry[0]);
> int i;
> for (i=0; (void*)&xsdt->table_offset_entry[i] < end; i++) {
>    u64 tbl_addr = xsdt->table_offset_entry[i];
>    if (!tbl_addr || (tbl_addr & ~0xffffffffULL))
>        continue;
>    rsdt_size += 4;
> }
> rsdt = malloc_high(rsdt_size);
> RsdpAddr->rsdt_physical_address = (u32)rsdt;

Modifying the RSDP like this invalidates both its checksums, so they
need to be adjusted. Something like this:

RsdpAddr->checksum -= checksum(RsdpAddr, offsetof(struct
rsdp_descriptor, length));
    RsdpAddr->extended_checksum -= checksum(RsdpAddr, sizeof(struct
rsdp_descriptor));
> memcpy(rsdt, xsdt, sizeof(struct acpi_table_header));
> rsdt->signature = RSDT_SIGNATURE;
> rsdt->length = rsdt_size;
> rsdt->revision = 1;
> int j;
> for (i=j=0; (void*)&xsdt->table_offset_entry[i] < end; i++) {
>    u64 tbl_addr = xsdt->table_offset_entry[i];
>    if (!tbl_addr || (tbl_addr & ~0xffffffffULL))
>        continue;
>    rsdt->table_offset_entry[j++] = (u32)tbl_addr;
> }
> rsdt->checksum -= checksum(rsdt, rsdt_size);
>
> +}
>
> void
>  qemu_platform_setup(void)
>  {
> @@ -186,8 +227,10 @@ qemu_platform_setup(void)
>      RsdpAddr = find_acpi_rsdp();
>
>    if (RsdpAddr)
>    if (RsdpAddr) {
>        build_compatibility_rsdt();
>        return;
>    }
>
>    /* If present, loader should have installed an RSDP.
>     * Not installed? We might still be able to continue
>
> diff --git a/src/std/acpi.h b/src/std/acpi.h
> index c2ea707..caf5e7e 100644
> --- a/src/std/acpi.h
> +++ b/src/std/acpi.h
> @@ -133,6 +133,17 @@ struct rsdt_descriptor_rev1
>  } PACKED;
>
> /*
> ACPI 2.0 Root System Description Table (RSDT)
> */

Comment should be "ACPI 2.0 Extended System Description Table (XSDT)"
> +#define XSDT_SIGNATURE 0x54445358 // RSDT

The signature is "XSDT", that works out to 0x58445358
> +struct xsdt_descriptor_rev1
> +{
> ACPI_TABLE_HEADER_DEF       /* ACPI common table header */
> u64 table_offset_entry[0];  /* Array of pointers to other */
> /* ACPI tables */
>
> +} PACKED;
>
> +/*
> ACPI 1.0 Firmware ACPI Control Structure (FACS)
>
> */
> #define FACS_SIGNATURE 0x53434146 // FACS
>
> 2.13.3
>
> From 46c7a296d27bd487cfd89a40973b1e61426dfbd0 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini 
> Date: Tue, 25 Jul 2017 18:59:20 +0200
> Subject: [PATCH 2/2] seabios: create v1 FADT in compatibility RSDT
>
> This patch presents a v1 FADT inside the compatibility RSDT, so
> that old operating systems are not broken.
> Signed-off-by: Paolo Bonzini
>
> src/fw/paravirt.c | 18 +++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/src/fw/paravirt.c b/src/fw/paravirt.c
> index 19a4abe..a9b9e80 100644
> --- a/src/fw/paravirt.c
> +++ b/src/fw/paravirt.c
> @@ -148,6 +148,18 @@ static void msr_feature_control_setup(void)
>          wrmsr_smp(MSR_IA32_FEATURE_CONTROL, feature_control_bits);
>  }
>
> +static void*
> +build_rev1_fadt(struct fadt_descriptor_rev1 *fadt_v3)
> +{
> struct fadt_descriptor_rev1 *fadt_v1 = malloc_high(sizeof *fadt_v1);
> memcpy(fadt_v1, fadt_v3, sizeof *fadt_v1);
> fadt_v1->length = sizeof *fadt_v1;
> fadt_v1->revision = 1;

We should mask out the flags bits that are reserved in ACPI 1.0 as
they refer to fields not present in this revision's FADT. e.g.

// upper 23 bits reserved in rev1 FADT
    fadt_v1->flags &= 0x1ff;
> fadt_v1->checksum -= checksum(fadt_v1, fadt_v1->length);
> return fadt_v1;
>
> +}
>
> static void
>  build_compatibility_rsdt(void)
>  {
> @@ -182,6 +194,12 @@ build_compatibility_rsdt(void)
>          u64 tbl_addr = xsdt->table_offset_entry[i];
>          if (!tbl_addr || (tbl_addr & ~0xffffffffULL))
>              continue;
>    struct acpi_table_header *tbl = (void*)(u32)tbl_addr;
>    /* The RSDT should only have an ACPI 1.0 FADT according to
>     * the spec.
>     */
>    if (tbl->signature == FACP_SIGNATURE && tbl->revision > 1)
>        tbl_addr = (u32)build_rev1_fadt((void *)tbl);
>    rsdt->table_offset_entry[j++] = (u32)tbl_addr;
> }

With these changes:

Reviewed-by: Phil Dennis-Jordan

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Kevin O'Connor]

On Tue, Jul 25, 2017 at 07:10:21PM +0200, Paolo Bonzini wrote:
> On 25/07/2017 18:23, Paolo Bonzini wrote:
> > On 25/07/2017 18:14, Laszlo Ersek wrote:
> > > "No regressions became apparent in tests with a range of Windows
> > >    (XP-10)"
> > >
> > > In theory, w2k falls within that range.
> >
> > Nope, Windows 2000 is like NT 5.0, XP is like NT 5.1. :(
> >
> > One possibility is to fix it in SeaBIOS instead: if you get a 2.0 FADT
> > and an XSDT and no RSDT, it can build an RSDT and a 1.0 FADT itself,
> > patching the RSDT to point to it.
> >
> > It's a hack, but it's the only place I see to make it "just work".  And
> > it could be extended nicely in the future.

It's an impressive hack!
> > All QEMU would have to do is to provide an XSDT _instead_ of an RSDT.

[...]
> SeaBIOS:
>
> From 73b0facdb7349f5dbf24dc675647b68eeeec0ff4 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini 
> Date: Tue, 25 Jul 2017 18:50:19 +0200
> Subject: [PATCH 1/2] seabios: build RSDT from XSDT
>
> Old operating systems would like to have a v1 FADT, but new
> operating systems would like to have v3.
>
> Since old operating systems do not know about XSDTs, the
> solution is to point the RSDT to a v1 FADT and the XSDT to a
> v3 FADT.
>
> But, OVMF is not able to do that and barfs when it sees the
> second FADT---plus really it's only BIOS operating systems
> such as win2k that complain.  So instead: 1) make QEMU provide
> an XSDT only; 2) build the RSDT and v1 FADT in SeaBIOS.
>
> This patch makes SeaBIOS build an RSDT out of an existing XSDT.

I'd really prefer not to have SeaBIOS go back to producing ACPI
tables.

As an alternative, how about some other possible hacks:

1 - ovmf filters out the extra tables that it barfs on.

2 - change ovmf to read the fw_cfg linker script
    'etc/table-loader-ovmf' instead of '/etc/table-loader' and change
    qemu to generate two linker scripts - one for seabios and one for
    ovmf.

3 - same as 2, but change seabios to use 'etc/table-loader-seabios'
    and leave ovmf unchanged.

4 - change seabios to read both the linker script 'etc/table-loader'
    _and_ some new linker script '/etc/table-loader-legacy'.  Have
    qemu introduce the RSDT/FADTv1 in the "legacy" linker script.

-Kevin

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Paolo Bonzini]

On 26/07/2017 00:01, Kevin O'Connor wrote:
> On Tue, Jul 25, 2017 at 07:10:21PM +0200, Paolo Bonzini wrote:
> > On 25/07/2017 18:23, Paolo Bonzini wrote:
> > > On 25/07/2017 18:14, Laszlo Ersek wrote:
> > > > "No regressions became apparent in tests with a range of Windows
> > > >    (XP-10)"
> > > >
> > > > In theory, w2k falls within that range.
> > >
> > > Nope, Windows 2000 is like NT 5.0, XP is like NT 5.1. :(
> > >
> > > One possibility is to fix it in SeaBIOS instead: if you get a 2.0 FADT
> > > and an XSDT and no RSDT, it can build an RSDT and a 1.0 FADT itself,
> > > patching the RSDT to point to it.
> > >
> > > It's a hack, but it's the only place I see to make it "just work".  And
> > > it could be extended nicely in the future.
>
> It's an impressive hack!
> > > All QEMU would have to do is to provide an XSDT _instead_ of an RSDT.
>
> [...]
> > SeaBIOS:
> >
> > From 73b0facdb7349f5dbf24dc675647b68eeeec0ff4 Mon Sep 17 00:00:00 2001
> > From: Paolo Bonzini 
> > Date: Tue, 25 Jul 2017 18:50:19 +0200
> > Subject: [PATCH 1/2] seabios: build RSDT from XSDT
> >
> > Old operating systems would like to have a v1 FADT, but new
> > operating systems would like to have v3.
> >
> > Since old operating systems do not know about XSDTs, the
> > solution is to point the RSDT to a v1 FADT and the XSDT to a
> > v3 FADT.
> >
> > But, OVMF is not able to do that and barfs when it sees the
> > second FADT---plus really it's only BIOS operating systems
> > such as win2k that complain.  So instead: 1) make QEMU provide
> > an XSDT only; 2) build the RSDT and v1 FADT in SeaBIOS.
> >
> > This patch makes SeaBIOS build an RSDT out of an existing XSDT.
>
> I'd really prefer not to have SeaBIOS go back to producing ACPI
> tables.

Me too, but this is different from SeaBIOS producing ACPI tables.
(Patched) QEMU produces entirely valid ACPI 2.0 tables, while SeaBIOS is
only producing compatibility glue for old OSes.  Compared to producing
ACPI tables, SeaBIOS needs no knowledge of the underlying hardware, only
of the limitations of those old OSes.  Responsibilities between QEMU and
SeaBIOS are nicely split.
> As an alternative, how about some other possible hacks:
>
> 1 - ovmf filters out the extra tables that it barfs on.
>
> 2 - change ovmf to read the fw_cfg linker script
>     'etc/table-loader-ovmf' instead of '/etc/table-loader' and change
>     qemu to generate two linker scripts - one for seabios and one for
>     ovmf.
>
> 3 - same as 2, but change seabios to use 'etc/table-loader-seabios'
>     and leave ovmf unchanged.
>
> 4 - change seabios to read both the linker script 'etc/table-loader'
>     _and_ some new linker script '/etc/table-loader-legacy'.  Have
>     qemu introduce the RSDT/FADTv1 in the "legacy" linker script.

(4) would be acceptable I guess.  However I think it's a bit worse
because fw-cfg files are a somewhat scarce resource.  The "legacy"
aspect is something that SeaBIOS is in the best position to address,
because it knows what OSes are running on it; QEMU instead only takes
care of describing the hardware.

Paolo

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Kevin O'Connor]

On Wed, Jul 26, 2017 at 09:20:16AM +0200, Paolo Bonzini wrote:
> On 26/07/2017 00:01, Kevin O'Connor wrote:
> > On Tue, Jul 25, 2017 at 07:10:21PM +0200, Paolo Bonzini wrote:
> > > On 25/07/2017 18:23, Paolo Bonzini wrote:
> > > > On 25/07/2017 18:14, Laszlo Ersek wrote:
> > > > > "No regressions became apparent in tests with a range of Windows
> > > > >    (XP-10)"
> > > > >
> > > > > In theory, w2k falls within that range.
> > > >
> > > > Nope, Windows 2000 is like NT 5.0, XP is like NT 5.1. :(
> > > >
> > > > One possibility is to fix it in SeaBIOS instead: if you get a 2.0 FADT
> > > > and an XSDT and no RSDT, it can build an RSDT and a 1.0 FADT itself,
> > > > patching the RSDT to point to it.
> > > >
> > > > It's a hack, but it's the only place I see to make it "just work".  And
> > > > it could be extended nicely in the future.
> >
> > It's an impressive hack!
> > > > All QEMU would have to do is to provide an XSDT _instead_ of an RSDT.
> >
> > [...]
> > > SeaBIOS:
> > >
> > > From 73b0facdb7349f5dbf24dc675647b68eeeec0ff4 Mon Sep 17 00:00:00 2001
> > > From: Paolo Bonzini 
> > > Date: Tue, 25 Jul 2017 18:50:19 +0200
> > > Subject: [PATCH 1/2] seabios: build RSDT from XSDT
> > >
> > > Old operating systems would like to have a v1 FADT, but new
> > > operating systems would like to have v3.
> > >
> > > Since old operating systems do not know about XSDTs, the
> > > solution is to point the RSDT to a v1 FADT and the XSDT to a
> > > v3 FADT.
> > >
> > > But, OVMF is not able to do that and barfs when it sees the
> > > second FADT---plus really it's only BIOS operating systems
> > > such as win2k that complain.  So instead: 1) make QEMU provide
> > > an XSDT only; 2) build the RSDT and v1 FADT in SeaBIOS.
> > >
> > > This patch makes SeaBIOS build an RSDT out of an existing XSDT.
> >
> > I'd really prefer not to have SeaBIOS go back to producing ACPI
> > tables.
>
> Me too, but this is different from SeaBIOS producing ACPI tables.
> (Patched) QEMU produces entirely valid ACPI 2.0 tables, while SeaBIOS is
> only producing compatibility glue for old OSes.  Compared to producing
> ACPI tables, SeaBIOS needs no knowledge of the underlying hardware, only
> of the limitations of those old OSes.  Responsibilities between QEMU and
> SeaBIOS are nicely split.
> > As an alternative, how about some other possible hacks:
> >
> > 1 - ovmf filters out the extra tables that it barfs on.
> >
> > 2 - change ovmf to read the fw_cfg linker script
> >     'etc/table-loader-ovmf' instead of '/etc/table-loader' and change
> >     qemu to generate two linker scripts - one for seabios and one for
> >     ovmf.
> >
> > 3 - same as 2, but change seabios to use 'etc/table-loader-seabios'
> >     and leave ovmf unchanged.
> >
> > 4 - change seabios to read both the linker script 'etc/table-loader'
> >     _and_ some new linker script '/etc/table-loader-legacy'.  Have
> >     qemu introduce the RSDT/FADTv1 in the "legacy" linker script.
>
> (4) would be acceptable I guess.  However I think it's a bit worse
> because fw-cfg files are a somewhat scarce resource.  The "legacy"
> aspect is something that SeaBIOS is in the best position to address,
> because it knows what OSes are running on it; QEMU instead only takes
> care of describing the hardware.

SeaBIOS is used with both modern and legacy OSes, and it doesn't have
any knowledge about what kind of OS will be used.  If anything, I'd
argue that QEMU has more knowledge about the guest OS than SeaBIOS
does (due to command-line options like machine version).

As I see it, fundamentally the proposal here is to deploy different
ACPI tables when using SeaBIOS then when using OVMF.  I think that's
fine, but I think we should directly address that issue then.

Specifically, I have the following concerns with the original approach:

A - It would require deploying SeaBIOS and QEMU in lock-step.  To get
    this in for QEMU v2.10 would require making QEMU changes during
    the soft freeze and would require a SeaBIOS "stable" release that
    introduces ACPI table manipulation.

B - I don't have full confidence the proposed ACPI changes wont expose
    a quirk in some obscure OS from the last 25 years.  If it does
    expose a quirk, any work-around would likely require deploying a
    new SeaBIOS and QEMU in lock-step.

C - We'd be introducing "shared ownership" of the acpi tables.  Some
    of the tables would be produced by QEMU and some of them by
    SeaBIOS.  Explaining when and why to future developers would be a
    challenge.

-Kevin

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Paolo Bonzini]

> > (4) would be acceptable I guess.  However I think it's a bit worse
> > because fw-cfg files are a somewhat scarce resource.  The "legacy"
> > aspect is something that SeaBIOS is in the best position to address,
> > because it knows what OSes are running on it; QEMU instead only takes
> > care of describing the hardware.
>
> SeaBIOS is used with both modern and legacy OSes, and it doesn't have
> any knowledge about what kind of OS will be used.  If anything, I'd
> argue that QEMU has more knowledge about the guest OS than SeaBIOS
> does (due to command-line options like machine version).

No, the machine type is independent from the guest OS.  The aim is to
let "qemu-system-x86_64 -m MEMORYSZ image.qcow2" work just fine with
every guest OS.
> As I see it, fundamentally the proposal here is to deploy different
> ACPI tables when using SeaBIOS then when using OVMF.  I think that's
> fine, but I think we should directly address that issue then.

The different ACPI tables are essentially a bug in OVMF.  They can be
fixed to be the same.
> Specifically, I have the following concerns with the original approach:
>
> A - It would require deploying SeaBIOS and QEMU in lock-step.  To get
>     this in for QEMU v2.10 would require making QEMU changes during
>     the soft freeze and would require a SeaBIOS "stable" release that
>     introduces ACPI table manipulation.

Yes.
> B - I don't have full confidence the proposed ACPI changes wont expose
>     a quirk in some obscure OS from the last 25 years.  If it does
>     expose a quirk, any work-around would likely require deploying a
>     new SeaBIOS and QEMU in lock-step.

Well, the solution is proposed by ACPI itself, and the worst that can
happen is that some OS prefers the RSDT to the XSDT (which we already
test on Windows 2000).
> C - We'd be introducing "shared ownership" of the acpi tables.  Some
>     of the tables would be produced by QEMU and some of them by
>     SeaBIOS.  Explaining when and why to future developers would be a
>     challenge.

The advantage is that the same shared ownership is already present in
OVMF.  The RSDP/RSDT/XSDT are entirely created by the firmware in OVMF.
(The rev1 FADT isn't but that's just missing code; the table manager
in general would be ready for that).  In any case this doesn't seem
like something that cannot be solved by code comments.

Paolo

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Gerd Hoffmann]

Hi,
> > SeaBIOS is used with both modern and legacy OSes, and it doesn't
> > have
> > any knowledge about what kind of OS will be used.  If anything, I'd
> > argue that QEMU has more knowledge about the guest OS than SeaBIOS
> > does (due to command-line options like machine version).
>
> No, the machine type is independent from the guest OS.

Well, not really.  Old guests (winxp & older) don't do very well on pc.
 Modern ones are better served with q35, because they either don't boot
on pc (macos I think) or can use the features of the modern hardware,
like pci express.

I still like the idea to have pc machine type provide the older stuff. 
The hardware we emulate for the pc machine type is old enough that old
acpi versions should have no problems describing it.
> > A - It would require deploying SeaBIOS and QEMU in lock-step.  To
> > get
> >     this in for QEMU v2.10 would require making QEMU changes during
> >     the soft freeze and would require a SeaBIOS "stable" release
> > that
> >     introduces ACPI table manipulation.
>
> Yes.

Since the switch to qemu-generated acpi tables we were able to avoid
that kind of lockstep updates, I'd prefer to not loose that.
> > B - I don't have full confidence the proposed ACPI changes wont
> > expose
> >     a quirk in some obscure OS from the last 25 years.  If it does
> >     expose a quirk, any work-around would likely require deploying
> > a
> >     new SeaBIOS and QEMU in lock-step.
>
> Well, the solution is proposed by ACPI itself, and the worst that can
> happen is that some OS prefers the RSDT to the XSDT (which we already
> test on Windows 2000).
> > C - We'd be introducing "shared ownership" of the acpi
> > tables.  Some
> >     of the tables would be produced by QEMU and some of them by
> >     SeaBIOS.  Explaining when and why to future developers would be
> > a
> >     challenge.
>
> The advantage is that the same shared ownership is already present in
> OVMF.  The RSDP/RSDT/XSDT are entirely created by the firmware in
> OVMF.

Hmm, seabios could do the same then, and we would not need a lockstep
update?

cheers,
  Gerd

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Paolo Bonzini]

On 27/07/2017 10:39, Gerd Hoffmann wrote:
> > > C - We'd be introducing "shared ownership" of the acpi
> > > tables.  Some
> > >     of the tables would be produced by QEMU and some of them by
> > >     SeaBIOS.  Explaining when and why to future developers would be
> > > a
> > >     challenge.
> >
> > The advantage is that the same shared ownership is already present in
> > OVMF.  The RSDP/RSDT/XSDT are entirely created by the firmware in
> > OVMF.
>
> Hmm, seabios could do the same then, and we would not need a lockstep
> update?

That's fine by me too.  I'll try to make a patch.

Paolo

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Kevin O'Connor]

On Wed, Jul 26, 2017 at 04:21:23PM -0400, Paolo Bonzini wrote:
> > As I see it, fundamentally the proposal here is to deploy different
> > ACPI tables when using SeaBIOS then when using OVMF.  I think that's
> > fine, but I think we should directly address that issue then.
>
> The different ACPI tables are essentially a bug in OVMF.  They can be
> fixed to be the same.
> > Specifically, I have the following concerns with the original approach:
> >
> > A - It would require deploying SeaBIOS and QEMU in lock-step.  To get
> >     this in for QEMU v2.10 would require making QEMU changes during
> >     the soft freeze and would require a SeaBIOS "stable" release that
> >     introduces ACPI table manipulation.
>
> Yes.
> > B - I don't have full confidence the proposed ACPI changes wont expose
> >     a quirk in some obscure OS from the last 25 years.  If it does
> >     expose a quirk, any work-around would likely require deploying a
> >     new SeaBIOS and QEMU in lock-step.
>
> Well, the solution is proposed by ACPI itself, and the worst that can
> happen is that some OS prefers the RSDT to the XSDT (which we already
> test on Windows 2000).

Parsing the XSDT is a different code path in the OSes - it could
expose a quirk.  I'm fine with the new layout of the ACPI tables, but
I think we need to be prepared that the change could have a ripple
effect.
> > C - We'd be introducing "shared ownership" of the acpi tables.  Some
> >     of the tables would be produced by QEMU and some of them by
> >     SeaBIOS.  Explaining when and why to future developers would be a
> >     challenge.
>
> The advantage is that the same shared ownership is already present in
> OVMF.  The RSDP/RSDT/XSDT are entirely created by the firmware in OVMF.
> (The rev1 FADT isn't but that's just missing code; the table manager
> in general would be ready for that).  In any case this doesn't seem
> like something that cannot be solved by code comments.

I'd argue that the shared ownership in the EDK2 code was a poor design
choice.  Case in point - we're only having this conversation because
of its limitations - SeaBIOS is capable of deploying the acpi tables
in the proposed layout without any code changes today.  I'm not
against changing SeaBIOS, but it's a priority for me that we continue
to make it possible to deploy future ACPI table changes (no matter how
quirky) in a way that does not require future SeaBIOS releases.

-Kevin

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Laszlo Ersek]

On 07/27/17 16:59, Kevin O'Connor wrote:
> On Wed, Jul 26, 2017 at 04:21:23PM -0400, Paolo Bonzini wrote:
> > > C - We'd be introducing "shared ownership" of the acpi tables.  Some
> > >     of the tables would be produced by QEMU and some of them by
> > >     SeaBIOS.  Explaining when and why to future developers would be
> > >     a challenge.
> >
> > The advantage is that the same shared ownership is already present in
> > OVMF.  The RSDP/RSDT/XSDT are entirely created by the firmware in
> > OVMF. (The rev1 FADT isn't but that's just missing code; the table
> > manager in general would be ready for that).  In any case this
> > doesn't seem like something that cannot be solved by code comments.
>
> I'd argue that the shared ownership in the EDK2 code was a poor design
> choice.

The reason we can't just exclude the reference implementation of
EFI_ACPI_TABLE_PROTOCOL from OVMF whole-sale, and reimplement the ACPI
linker/loader from scratch, is that some other (independent) edk2
modules will want to use EFI_ACPI_TABLE_PROTOCOL for installing their
own (one-off) tables, such as IBFT, BGRT and so on, *in addition to*
QEMU's. Given that these ACPI tables mostly do *not* describe hardware
(but software features and/or configuration), it's hard to claim that
they should also be generated by QEMU.

Therefore the dual origin for ACPI tables looks unavoidable in UEFI,
it's just that there should be a lot more flexible "connect" from QEMU's
linker/loader to the installed ACPI tables than EFI_ACPI_TABLE_PROTOCOL.

Basically this is a fight over ownership. Each of QEMU's ACPI
linker/loader and EFI_ACPI_TABLE_PROTOCOL thinks that it fully owns the
root of the table tree. :(
> Case in point - we're only having this conversation because of its
> limitations - SeaBIOS is capable of deploying the acpi tables in the
> proposed layout without any code changes today.

Yes.

But let's not forget that SeaBIOS is capable of delegating the full
low-level construction of the table tree to QEMU because no independent
/ 3rd party BIOS-level code wants to install its own tables (again,
IBFT, BGRT, ...) This is not true of UEFI, where the guiding principle
of the standardized interfaces is to enable cooperation between
independent, binary-only modules. (So, for example, if you shove a new
PCI add-on card in your motherboard, the UEFI driver in that oprom could
install a separate ACPI table, by looking up and calling
EFI_ACPI_TABLE_PROTOCOL.)
> I'm not against changing SeaBIOS, but it's a priority for me that we
> continue to make it possible to deploy future ACPI table changes (no
> matter how quirky) in a way that does not require future SeaBIOS
> releases.

It's a good goal.

I apologize for forgetting the context, but what exactly was the
argument against:

- splitting modern ACPI generation from ancient ACPI generation (so that
we can assign separate maintainers to ancient vs. modern),

- restricting ancient ACPI generation to old machine types?

Thanks,
Laszlo

[Qemu-devel] Commit 77af8a2b95b79699de650965d5228772743efe84 breaks Windows 2000 support

[Gerd Hoffmann]

Hi,
> It's a good goal.
>
> I apologize for forgetting the context, but what exactly was the
> argument against:
> splitting modern ACPI generation from ancient ACPI generation (so
>
> that
> we can assign separate maintainers to ancient vs. modern),
> restricting ancient ACPI generation to old machine types?

I think this is the only sensible solution for 2.10:  Have "pc" provide
a rev1 FADT and "q35" provide a rev3 FADT.

Or just revert the rev3 FADT patch and try again in the 2.11 devel
cycle.

We are in qemu freeze and we don't even have a clear plan yet how to
implement the "RSDT points to rev1 and XSDT points to rev3" thing,
which IMO is a clear indicator that it isn't 2.10 material.

Beside that I'll be in my summer vacation next three weeks, so I can't
take care to create a seabios 1.10.3 release and update qemu ...

cheers,
  Gerd