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 <ler...@redhat.com> 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 <imamm...@redhat.com> wrote: >>>>> On Wed, 18 Jan 2017 18:30:59 +0200 >>>>> "Michael S. Tsirkin" <m...@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. >>> >>> + 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: <20150827205706-mutt-send-email-...@redhat.com> >> URL: http://lists.nongnu.org/archive/html/qemu-devel/2015-08/msg03528.html >> >> Message-Id: <20151228141917-mutt-send-email-...@redhat.com> >> 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 >
