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

[Phil Dennis-Jordan <phil@philjordan.eu>]

On 18 January 2017 at 18:19, Igor Mammedov <imammedo@redhat.com> wrote:
> On Wed, 18 Jan 2017 18:30:59 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Wed, Jan 18, 2017 at 12:45:54PM +0100, Phil Dennis-Jordan wrote:
> [...]
>
>> > 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.
> if XP and legacy linux survive,
> I'd skip adding option as probably there won't be any users,
> in unlikely case such user surfaces we always can add option later
> but we can't do it other way around (i.e. take it away).

I have now finally solved the mystery of why my FADT patch has been
going so disastrously wrong - I've now got working code, but I'd
appreciate some guidance on the best way to structure a patch to
minimise further back-and forth. The culprit turned out to be OVMF,
specifically 2 bugs/shortcomings:

1. It completely gives up on parsing Qemu's ACPI tables if more than
one "add pointer" linker command points to the same table. In this
case, if you add a command for both the DSDT and X_DSDT fields of the
FADT, it aborts completely and uses fallback tables. (The following
InstallAcpiTable call fails if called twice with the same table type.)
https://github.com/tianocore/edk2/blob/master/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c#L518

2. After applying all the linker commands, it goes and rewrites part
of the FADT anyway. Specifically, it rewrites the DSDT and X_DSDT
fields - and it always sets one of them to 0. Which one depends on
whether the DSDT is above the 4G barrier:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c#L650

Both of these are easily fixed, and I will submit a corresponding patch to EDK2.

With that fixed, the rest of the FADT provided by Qemu is accepted by
OVMF and the operating systems. On the Qemu side, it does mean we'll
need to still retain the ACPI 1.0 tables for backwards compatibility.

Q1: How should the option be structured and named? Should the FADT
revision be selectable via a sub-option on -machine? Or as a
standalone option? Something else?


Q2: To avoid any more confusion, I'd appreciate
confirmation/clarification on the X_ and non-X FADT fields in the case
where 32-bit pointers suffice.

Q2a: DSDT/X_DSDT: Both variants appear to be de-facto required.

Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.

Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
"This is a required field" for both variants.

Q2d: GPE0_BLK/X_GPE0_BLK: Both variants state "if this register block
is not supported, this field contains zero." - I understand this to
mean that when the register block IS supported and 32-bit, both
variants must be filled.

In other words, only X_FIRMWARE_CTRL stays zero in Qemu's x86 case.


I'll come up with a revised patch in the next few days.

Thanks for your help and patience so far, everyone!