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

[Phil Dennis-Jordan]

Thanks for the in-depth reply - apologies for abandoning the thread
for a few days. Looks like a bunch of things have already been hashed
out, but I have a few more comments:

On 31 January 2017 at 17:28, Laszlo Ersek <address@hidden> wrote:
> On 01/31/17 15:58, Michael S. Tsirkin wrote:
>> On Tue, Jan 31, 2017 at 03:31:46PM +0100, Phil Dennis-Jordan wrote:
>>> On 18 January 2017 at 18:19, Igor Mammedov <address@hidden> wrote:
>>>> On Wed, 18 Jan 2017 18:30:59 +0200
>>>> "Michael S. Tsirkin" <address@hidden> 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.
>>
>> + lersek
>>
>>> 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
>
> Unlike SeaBIOS, OVMF can only use EFI_ACPI_TABLE_PROTOCOL to install
> ACPI tables.
>
> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs a *copy* of the
> table that is passed in. In addition, EFI_ACPI_TABLE_PROTOCOL (whose
> implementation is in generic edk2 code, not in OVMF platform code) has
> built-in "smarts" for handling and linking together the various specific
> tables defined by the ACPI spec.
>
> The way we were able to make EFI_ACPI_TABLE_PROTOCOL cooperate with
> QEMU's linker/loader is a two-phase process.
>
> In the first phase, the fw_cfg blobs are allocated, downloaded, linked
> together, and checksummed, in AcpiNVS type memory, as instructed by the
> linker/loader script.
>
> In the second phase, all ADD_POINTER commands are processed again, and
> the targets of those pointers are investigated for ACPI SDT headers.
> Every such pointer target that looks like an ACPI SDT header is passed
> to EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable(); except root tables like
> RSDT etc. This member function handles the actual copying, installation,
> and cross-table linking (to the extent specified in the ACPI spec).
>
> Additionally, for each allocated  / downloaded fw_cfg blob, the second
> phase tracks if there is at least one pointer into the blob that does
> *not* point to an ACPI SDT header. If that's the case, then it implies
> the presence of some non-ACPI-table-buffer within the blob (that is
> referenced by some other field elsewhere). In turn such fw_cfg blobs are
> not freed at the end of the whole process. (If an fw_cfg blob turns out
> to host *only* ACPI tables -- which were then all copied for
> installation, see above -- then the blob is released in the end.)
>
> Finally, EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() may return
> EFI_ACCESS_DENIED (quoting the UEFI spec):
>
> "The table signature matches a table already present in the system and
> platform policy does not allow duplicate tables of this type."
>
> (And see edk2 commit 5966402ed51c, "MdeModulePkg/IntelFrameworkModulePkg
> ACPI: Follow the new UEFI 2.4a spec to return EFI_ACCESS_DENIED for
> duplicated FADT, FACS or DSDT installation.", 2014-05-06.)
>
>
> ... BTW, we discussed the question of multiply-pointed-to tables before,
> in connection with XSDT support. Under "XSDT support", I mean an XSDT
> built by QEMU explicitly (because EFI_ACPI_TABLE_PROTOCOL handles both
> XSDT and RSDT internally, automatically). Clearly, if some ACPI table is
> referenced from QEMU's RSDT and XSDT both, that immediately triggers the
> same problem.
>
> Looking at my older notes, I find two references:
>
> Message-Id: <address@hidden>
> URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg03528.html
>
> Message-Id: <address@hidden>
> URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg04636.html
>
> Looking further at my notes, I find the following idea:
>
>     in the second pass only, maintain a global rbtree that contains
>     VOID* objects, pointing to actual physcal addresses in the relocated
>     blobs that have already been passed to InstallAcpiTable. This will
>     ensure that no address is passed to InstallAcpiTable twice
>
> IOW, it's simple memoization for ADD_POINTER targets (in absolute
> guest-phys addr space) so that duplicate
> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() can be avoided.
>
> If this feature is important, I can file an upstream OVMF bug for it,
> and work on it. (I already have another open BZ for the linker/loader
> anyway, <https://bugzilla.tianocore.org/show_bug.cgi?id=359>.)
>
> Please confirm if this is necessary.

That sounds good. Some background on the specific issue I'm trying to solve:

 - OS X/macOS guests currently can't reboot with upstream Qemu, as it
expects the reset register to be advertised by the FADT, but x86 Qemu
currently only publishes a rev1 FADT, which lacks said register spec.
(OVMF still requires a large array of patches to boot OS X/macOS,
which is a separate issue.)
 - I therefore would like to update Qemu such that it publishes a rev3
(ACPI 2.0) or newer FADT, including the reset register.
 - Windows 10 appears to require both DSDT AND X_DSDT to be filled for
rev3-rev4 FADTs, else it won't boot at all. (Not sure about 5+)
 - Guest OS backwards compatibility without a flag is desirable, and
ACPI 1.0 era guest OSes will not try to find the DSDT via X_DSDT, so
we need to fill both.
 - This should work with both SeaBIOS and OVMF, so Qemu needs to set
up linker commands for both DSDT and X_DSDT, and OVMF itself needs to
produce a FADT with both fields filled.

Your EDK2 patch fixes the problem with values OVMF writes to
DSDT/X_DSDT, but the issue of it refusing Qemu's linker commands for
those two still needs to be solved so that SeaBIOS can boot a wide
range of OSes.

I'd be happy to give writing the memoising patch a go myself if that
helps. Looks like setting up an ORDERED_COLLECTION during phase 2
might be the simplest solution?

>>> 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
>
> Yes. This is precisely what I meant above with:
>
>     EFI_ACPI_TABLE_PROTOCOL (whose implementation is in generic edk2
>     code, not in OVMF platform code) has built-in "smarts" for handling
>     and linking together the various specific tables defined by the
>     ACPI spec.
>
>>>
>>> Both of these are easily fixed, and I will submit a corresponding patch to 
>>> EDK2.
>
> What easy fixes do you have in mind?
>
> For problem (1), simply ignoring EFI_ACCESS_DENIED from
> EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() is not right. Unless we can
> ensure in a future-proof way that an error code gets returned for *only
> one* error scenario and nothing else, suppressing the error code is
> risky. I'd much rather prevent a function call (with memoization) that
> we know in advance will fail.

That's been my prototype workaround - the memoisation should be pretty
straightforward too though.

> For symptom (2), I'm unsure if we can call that a problem at all. You'd
> have to prove that "MdeModulePkg/Universal/Acpi/AcpiTableDxe" violates
> the ACPI spec (or that it's within the spec, but works sub-optimally),
> when it ensures exclusivity between FADT.DSDT and FADT.X_DSDT.
>
> The ACPI 6.1 spec says,
>
> - DSDT: [...] If the X_DSDT field contains a non-zero value then this
>   field must be zero.
> - X_DSDT: [...] If the DSDT field contains a non-zero value then this
>   field must be zero.
>
> Therefore the EFI_ACPI_TABLE_PROTOCOL implementation in edk2 seems
> conformant & correct to me.
>
> Related edk2 commits:
>
> - the one that added the code you reference above:
>
> f9bbb8d9c3f0 ("MdeModulePkg: AcpiTableDxe: make 4 GB table allocation
> limit optional", 2016-02-17)
>
> - the earlier, similar one, that enforces exclusivity between
> FADT.FIRMWARE_CTRL and FADT.X_FIRMWARE_CTRL:
>
> f798e8bff773 ("MdeModulePkg: Acpi: enforce exclusion between
> FirmwareCtrl and XFirmwareCtrl", 2015-01-26)

Between you you've already established the apparent revision-dependent
rules on whether or not DSDT and X_DSDT are mutually exclusive, so I
have nothing to add there, other than that Windows 10 fails to boot if
they're not both filled for rev3 and rev4 FADTs. (I'm so far having
some issues with producing a rev5 FADT it likes, regardless of DSDT
pointers.)

The X_FIRMWARE_CTRL vs FIRMWARE_CTRL situation appears to me to be
slightly different. They're already explicitly mutually exclusive from
ACPI 4.0 onwards, not 5.1. Note however, that it's not specified that
one should be preferred over the other in the sub-4G case, not even in
6.1. Note also that both ACPI 3 and 4 specify FADT revision 4, but
only 4.0 makes the fields mutually exclusive.

I therefore think the safest behaviour for the FIRMWARE_CTRL fields,
also with a view to backwards compatibility, is to always use the
32-bit variant where possible, and the X_ field only if a 64-bit
pointer is necessary. I have thus far encountered no issues with
booting real OSes using this policy.

>>> 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?
>
> No input from me on this one.
>
>>> 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.
>
> I disagree. The 6.1 release of the ACPI spec requires exclusivity.
>
> The changelog at the top of the spec lists:
>
> Revision    Change Description                         Affected Sections
> ----------  -----------------------------------------  -----------------
> 6.0 Errata  1393 In FADT: if X_DSDT field is           Table 5-34
>             non-zero, DSDT field should be ignored or
>             deprecated
>
> The number "1393" is most likely the Mantis ticket number:
>
> https://mantis.uefi.org/mantis/view.php?id=1393
>
> (Unfortunately, I don't have the necessary credentials to verify if this
> Mantis ticket actually corresponds to this change. I could do that for
> Platform Init or UEFI spec items -- without quoting any specifics --,
> but apparently my ASWG membership level isn't high enough for this one.)
>
>>> Q2b: FIRMWARE_CTRL/X_FIRMWARE_CTRL: leave X_FIRMWARE_CTRL zero.
>
> Sounds good.
>
>>>
>>> Q2c: X_PM1a_EVT_BLK, X_PM1a_CNT_BLK, X_PM_TMR_BLK: These all state
>>> "This is a required field" for both variants.
>
> Hmmm, I don't think so (again, from ACPI 6.1):
>
> - PM1a_EVT_BLK: [...] This is a required field. If the X_PM1a_CNT_BLK
>   field contains a non-zero value then this field must be zero.
> - X_PM1a_EVT_BLK: [...] This is a required field. If the PM1a_EVT_BLK
>   field contains a non-zero value then this field must be zero.
>
> (Not checking the rest.)
>
>>>
>>> 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!
>
> In summary, here's my opinion:
>
> - I think that setting both FADT.DSDT and FADT.X_DSDT to nonzero values
> simultaneously would break the ACPI 6.1 spec (update originating from
> Mantis #1393, most likely)
>
> - "MdeModulePkg/Universal/Acpi/AcpiTableDxe", implementing
> EFI_ACPI_TABLE_PROTOCOL in edk2, appears to conform to this requirement
> specifically
>
> - if remedying symptom (1) were only necessary for setting both DSDT and
> X_DSDT -- which, I claim, should not be done --, then I'd like to
> postpone the above memoization indefinitely. It's too complex to be
> added speculatively.
>
> Thank you,
> Laszlo