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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
> About 2 years ago, Reza Jelveh submitted essentially this same patch:
> https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05832.html
> 
> It adds the reset register defined in ACPI 2.0 to the x86 FADT, which fixes rebooting for Darwin/OS X/macOS guests.
> 
> I'm trying to revive this as part of an effort to get recent versions of OS X/macOS working in Qemu/KVM with OVMF and without the need for out-of-tree patches. I've adapted the original patch so it cleanly applies on current master. The unaddressed issues last time around were:
> 
>  1. Does this work with a range of Windows versions?
> 
> I've tested this new version of the patch with KVM from Ubuntu's 4.4.0-51-generic kernel, using the following guests:
> 
>  * Windows XP SP2 and SP3, x86. "pc" (FX440) machine, IDE disk, SeaBIOS.
>  * Windows 7 SP 1, x86. "pc" (FX440) machine, AHCI disk, SeaBIOS.
>  * Windows 10 1607, x86-64, q35 machine, AHCI disk, OVMF from 2017-01-17 EDK2 master.
>  * OS X 10.9 to 10.11, macOS 10.12; patched OVMF [1]
> 
> In all cases I went through a fresh install, checked basic functionality, and rebooting. I noticed nothing different in any of the Windows VMs, and of course the Darwin-based ones no longer hang when attempting to reboot.
> 
> 
>  2. The reset register is defined in ACPI 2.0, this still advertises 1.0.
> 
> I have to admit, I know very little about how ACPI works, so I'm not sure I got this right, but: I subsequently added a line
>     rsdp->revision = 0x02;
> to build_rsdp() in acpi-build.c, then booted the aforementioned Windows VMs, and the macOS 10.12 one with this change. I noticed no change or ill effect whatsoever.

For RSDP revision, I don't think we need to bother with it. People do
mix revisions of tables in practice.

I think what's important is the Fadt format revision. That one was 1 for 1.0b and 3 for 2.0.

See page 112, Table 5-5 Fixed ACPI Description Table Format in acpi spec
1.0b.

Now look at page 110 in spec 2.0, this time
"Table 5-8 Fixed ACPI Description Table (FADT) Format".

You will see that FADT revision is now 3, and there are a bunch of new
fields after the reset register. I think you have to fill them in,
you probably can get away with copying the non-extended values.




> I suspect more might be involved in enabling ACPI 2.0, and it should probably be an option so as to avoid regressions. I don't know what the best approach would be for this, so comments welcome. Should adding the reset register to the FADT also be configurable?

I would say an option to make FADT use rev 3 format would be a good
idea.

I'd make it the default if XP survives.

> 
> Additionally, please suggest any further guest OSes and configurations I should test which seem likely to exhibit regressions with this change.

Pls also try XP on q35. Paolo explained how on the thread you link to.

> 
> (First post to this list - apologies if I messed up any of the conventions! Thanks, phil)
> 
> [1] EDK2 fork with OS X/macOS compatibility patches: https://github.com/pmj/edk2/tree/mac-patches

It looks ok - you need to add your S.O.B (and maybe the original one)
to certify the origin of the patch. And you want to wrap lines around
80 characters.

> ---
>  hw/i386/acpi-build.c        | 5 +++++
>  include/hw/acpi/acpi-defs.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 0c8912f..93781a0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -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_val = 0xf;
> +    fadt->reset_reg.address_space_id   = 1;
> +    fadt->reset_reg.register_bit_width = 8;
> +    fadt->reset_reg.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
>       */

So this works for PC too because RCR_IOPORT has the same value.
I think QEMU_BUILD_BUG_ON(ICH9_RST_CNT_IOPORT != RCR_IOPORT)
and a comment would be helpful here.


> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 4cc3630..5755570 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -140,6 +140,8 @@ struct AcpiFadtDescriptorRev1
>      uint8_t  reserved4a;             /* Reserved */
>      uint8_t  reserved4b;             /* Reserved */
>      uint32_t flags;
> +    Acpi20GenericAddress reset_reg;
> +    uint8_t reset_val;
>  } QEMU_PACKED;
>  typedef struct AcpiFadtDescriptorRev1 AcpiFadtDescriptorRev1;

I think you want a new AcpiFadtDescriptorRev3 structure.

> -- 
> 2.3.2 (Apple Git-55)