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

[Laszlo Ersek]

On 02/06/17 17:30, Phil Dennis-Jordan wrote:
> 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:

Vice versa... Sorry about responding a bit late. Yesterday I severely
overworked myself and for all of today I've been struggling with a
terrible headache. Having had trouble at forming coherent thoughts, I've
sort of procrastinated following up here.

> 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.
> 

I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=368>.

> Your EDK2 patch

For the record:
[1] https://lists.01.org/pipermail/edk2-devel/2017-February/007072.html

> 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?

Thanks for the offer. :)

* If you'd like to help me with my load and reach a good "development
  throughput", then I prefer to write the patch myself. On this
  *specific* occasion, I think it will be faster.

  I intend to send an OVMF series this week that addresses both
  TianoCore#368 (see above) and TianoCore#359 too (mentioned earlier).
  Both are for OvmfPkg/AcpiPlatformDxe, and it makes sense to round
  them up.

  I plan to CC the QEMU stakeholders as appropriate. Your feedback
  would be greatly appreciated; the same way as [1] is very helpful
  (thanks again for it!)

* If your main interest is rather to get into OVMF development, then I
  positively welcome that, and encourage you to send the patch.
  Completing the patch will likely take longer (the edk2 coding style
  is... arcane... and your reviewer is somewhat pedantic :)), but if
  you plan to get into OVMF development, then it's worth both our times.

(The above should not be misunderstood as "Laszlo doesn't value one-off
contributions" -- it really depends on feature size and area. Hence the
emphasis on "specific" above.)

Your call :) If possible, please let me know it tomorrow.

> 
>>>> 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.

It's also more correct for ACPI table *types* that are permitted to have
several (different!) instances installed. For those table types, passing
the exact same table to InstallAcpiTable() might succeed -- and that
would be even worse. I commented on this in TianoCore#368.

> 
>> 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.)

I think it's prudent to stick with as low a version as possible.

> 
> 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.

Sounds good to me.

And, I think the edk2 code already does this. Commit f798e8bff773
("MdeModulePkg: Acpi: enforce exclusion between FirmwareCtrl and
XFirmwareCtrl", 2015-01-26) is one half of the puzzle.

The other half is that MdeModulePkg/Universal/Acpi/AcpiTableDxe, as
built into OVMF (Ia32 and X64), keeps the tables under 4GB. So in
practice it's always the 32-bit FIRMWARE_CTRL field that gets set.

Thanks!
Laszlo

> 
>>>> 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
>