Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt

[Phil Dennis-Jordan]

On 23 January 2017 at 12:12, Igor Mammedov <address@hidden> wrote:
>> For reference, my approach to filling out the Xdsdt/Xfacs fields in
>> build_fadt() is essentially the same as for the 32-bit variants from
>> rev1:
>>
>> unsigned xfacs_offset = (char *)&fadt->Xfacs - table_data->data;
>> bios_linker_loader_add_pointer(linker,
>>         ACPI_BUILD_TABLE_FILE, xfacs_offset, sizeof(fadt->Xfacs),
>>         ACPI_BUILD_TABLE_FILE, facs_tbl_offset);
>>
>> Suggestions welcome.
> You are not supposed to fill in the same values in X_FOO as in FOO.
> Spec says that only either one of above should be set.

Thanks for clarifying that. I'd been looking at the ACPI 2.0 spec,
which isn't clear on this point - the 5.0 one unambiguously agrees
with you.

I've tested the FADT change through again. Leaving Xdsdt and Xfacs as
0 in the Rev3 does indeed allow macOS to boot, as previously reported.
Rebooting also works, which is the whole point of the patch. I'm still
no further with getting Windows 10 past the "ACPI BIOS ERROR"
bluescreen, however, even after some more experimenting with the
various fields.
I've tried a Rev5 FADT too, the Win10 bootloader just hangs in this
cace, I don't even get a bluescreen. macOS is still happy.
I've dumped the ACPI tables that OVMF ultimately exports with an EFI
shell utility, and it all looks ok to me there; I also added various
debug output to OVMF's ACPI code, and again everything looks pretty
sane there. I guess my problem is I don't know what I'm looking for as
I haven't found a way for Windows to tell me exactly what it doesn't
like.

I'm basically stumped on the Windows 10 issue, but I'd still like to
fix macOS guest rebooting. Would a patch be acceptable that introduces
a Rev3 FADT option? (or Rev5 - this'll be much less code as the struct
already exists) If so, what nature should the option take?

For reference, my latest FADT Rev3 prototype patch follows, in case
anyone can spot some obvious problem. (I've kept this as compact as
possible as opposed to optimising code quality for upstreaming.) The
Rev5 FADT is identical except it uses sizeof(*fadt) and of course 5 as
the revision.

---
 hw/i386/acpi-build.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1c928ab..19dde8b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -271,7 +271,7 @@ build_facs(GArray *table_data, BIOSLinker *linker)
 }

 /* Load chipset information in FADT */
-static void fadt_setup(AcpiFadtDescriptorRev1 *fadt, AcpiPmInfo *pm)
+static void fadt_setup(AcpiFadtDescriptorRev5_1 *fadt, AcpiPmInfo *pm)
 {
     fadt->model = 1;
     fadt->reserved1 = 0;
@@ -296,6 +296,11 @@ static void fadt_setup(AcpiFadtDescriptorRev1
*fadt, AcpiPmInfo *pm)
                               (1 << ACPI_FADT_F_SLP_BUTTON) |
                               (1 << ACPI_FADT_F_RTC_S4));
     fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_USE_PLATFORM_CLOCK);
+    fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_RESET_REG_SUP);
+    fadt->reset_value = 0xf;
+    fadt->reset_register.space_id   = 1;
+    fadt->reset_register.bit_width = 8;
+    fadt->reset_register.address            = ICH9_RST_CNT_IOPORT;
     /* APIC destination mode ("Flat Logical") has an upper limit of 8 CPUs
      * For more than 8 CPUs, "Clustered Logical" mode has to be used
      */
@@ -303,6 +308,15 @@ static void fadt_setup(AcpiFadtDescriptorRev1
*fadt, AcpiPmInfo *pm)
         fadt->flags |= cpu_to_le32(1 << ACPI_FADT_F_FORCE_APIC_CLUSTER_MODEL);
     }
     fadt->century = RTC_CENTURY;
+
+    fadt->xpm1a_event_block.address = cpu_to_le64(pm->io_base);
+    fadt->xpm1a_event_block.space_id = 1;
+    fadt->xpm1a_control_block.address = cpu_to_le64(pm->io_base + 0x04);
+    fadt->xpm1a_control_block.space_id = 1;
+    fadt->xpm_timer_block.address = cpu_to_le64(pm->io_base + 0x08);
+    fadt->xpm_timer_block.space_id = 1;
+    fadt->xgpe0_block.address = cpu_to_le64(pm->gpe0_blk);
+    fadt->xgpe0_block.space_id = 1;
 }


@@ -312,7 +326,7 @@ build_fadt(GArray *table_data, BIOSLinker *linker,
AcpiPmInfo *pm,
            unsigned facs_tbl_offset, unsigned dsdt_tbl_offset,
            const char *oem_id, const char *oem_table_id)
 {
-    AcpiFadtDescriptorRev1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
+    AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, 244);
     unsigned fw_ctrl_offset = (char *)&fadt->firmware_ctrl - table_data->data;
     unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;

@@ -328,7 +342,7 @@ build_fadt(GArray *table_data, BIOSLinker *linker,
AcpiPmInfo *pm,
         ACPI_BUILD_TABLE_FILE, dsdt_tbl_offset);

     build_header(linker, table_data,
-                 (void *)fadt, "FACP", sizeof(*fadt), 1, oem_id, oem_table_id);
+                 (void *)fadt, "FACP", 244, 3, oem_id, oem_table_id);
 }

 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
--