[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt |
Date: |
Wed, 18 Jan 2017 18:30:59 +0200 |
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)
- [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Phil Dennis-Jordan, 2017/01/18
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Igor Mammedov, 2017/01/18
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Phil Dennis-Jordan, 2017/01/31
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Michael S. Tsirkin, 2017/01/31
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Igor Mammedov, 2017/01/31
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Phil Dennis-Jordan, 2017/01/31
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Igor Mammedov, 2017/01/31
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Laszlo Ersek, 2017/01/31
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Michael S. Tsirkin, 2017/01/31
- Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Laszlo Ersek, 2017/01/31
Re: [Qemu-devel] [PATCH RFC] acpi: add reset register to fadt, Phil Dennis-Jordan, 2017/01/19