On 09/05/14 10:52, Laszlo Ersek wrote: > On 08/26/14 02:12, Jordan Justen wrote: >> On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <ler...@redhat.com> wrote: >>> On 08/26/14 00:24, Jordan Justen wrote: >>>> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <ler...@redhat.com> wrote: >>>>> Recent changes in the QEMU ACPI table generator have shown that our >>>>> limited client for that interface is insufficient and/or brittle. >>>>> >>>>> Implement the full interface utilizing OrderedCollectionLib for addressing >>>>> fw_cfg blobs by name. >>>>> >>>>> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >>>>> client, because: >>>>> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short >>>>> of a complete ACPI parser, >>>>> - and it is fully sufficient to install the RSD PTR as an EFI >>>>> configuration table for the guest OS to see everything. >>>>> >>>>> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >>>>> restrictive convenience; let's stop using it. >>>> >>>> Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) >>>> >>>> Is this required? >>> >>> Depends on how good (how accurate) ACPI tables you want to have in your >>> VMs. For a few qemu releases now, qemu has been the "master" of ACPI >>> payload generation. SeaBIOS too has kept its own builtin tables, yes, >>> but when the QEMU generated payload is available, it interprets this >>> linker/loader script just the same. >>> >>> If you want PCI hotplug, pvpanic support; or, going forward, memory >>> hotplug, then yes, those things need ACPI support, and the ACPI stuff >>> for them is now developed in QEMU (and dynamically generated by it, >>> dependent on the actual hardware config). >>> >>>> What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now >>>> have two drivers that want to install the pointer. >>> >>> Yes, I checked that. The UEFI spec says that whenever you install a >>> configuration table that's already present (by GUID), the reference is >>> updated. >>> >>> InstallConfigurationTable() >>> >>> [...] >>> * If Guid is present in the System Table, and Table is not NULL, then >>> the (Guid, Table) pair is updated with the new Table value. >>> [...] >>> >>> For this reason, the first thing InstallAllQemuLinkedTables() does is >>> check, with EfiGetSystemConfigurationTable(), for the presence of any of >>> the ACPI GUIDs in the config table. >>> >>> There's no "absolute" cure against another driver in OVMF just grabbing >>> EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then >>> completely hide the tables installed by this patchset, because >>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's >>> RSD PTR's address in the UEFI config table with its own). >>> >>> But ACPI tables are not randomly installed in OVMF, we keep it >>> centralized in AcpiTableDxe. >> >> I don't agree with this statement. Rather, I would say that OVMF >> follows the EDK II method of publishing tables, which means >> EFI_ACPI_TABLE_PROTOCOL. >> >> In the past we were a good little sample platform, and provided the >> ACPI tables directly. Well, that is less and less the case. But, is it >> a good idea to stop using EFI_ACPI_TABLE_PROTOCOL? >> >> What about MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c, which >> uses EFI_ACPI_TABLE_PROTOCOL? >> >> Do we need to keep monitoring which EDK II drivers decide to use >> EFI_ACPI_TABLE_PROTOCOL, or can we find a way to make these work >> together? If the answer is no, then I wonder if this is something that >> UEFI or EDK II needs to consider. > > I think I might have found a solution -- at this point it's just an > idea, but I want to run it by you guys first before I start implementing > it. The idea is based on this patchset, in an incremental fashion. > > Where the current code calls gBS->InstallConfigurationTable(), we shall > replace that call with further processing. > > Michael has proposed before to check the *targets* of all the pointer > commands. Namely, a pointed-to "something" in the blob that hosts it is > with a good chance an ACPI table. Not guaranteed, but likely. > > Now, after all is done by the current patchset *except* linking the RSDP > into the UEFI config table, the payload we have in AcpiNVS memory is a > nicely cross-linked, checksummed forest of ACPI stuff. We re-scan the > QEMU commands. We ignore everything different from AddPointer. When an > AddPointer call is found, the following statements are true: > > (a) the pointer field in the blob that the AddPointer call would > originally patch *now* contains a valid, absolute physical address. > > (b) the pointed-to byte-array may or may not be an ACPI table. It could > be an ACPI table, or it could be some funny area that QEMU has > preallocated, like the target of the TCPA.LASA field, or the target of > the BGRT.ImageAddress field, or something else. > > (c) *if* the pointed-to byte-array is an ACPI table, then we'll have it > already checksummed, since we're past the first complete pass of the > processing. > > So the idea is, look at the target area, > - determine if the remaining size in that blob (the pointed-to blob) > could still contain an ACPI table header, > - if so, check the presumed "length" field in that header, and see if > it's self-consistent (ie. >= sizeof(header), and <= remaining size in > blob) > - if so, run a checksum on the bytes that presumably constitute the > ACPI table > - if it sums to zero, we attempt to install the target area with > EFI_ACPI_TABLE_PROTOCOL, otherwise we leave it alone. > > Now, EFI_ACPI_TABLE_PROTOCOL makes deep copies of the tables it gets, > but we'll leave the entire forest that we've build during the first > processing in place. Why? Because this way all the stuff that *didn't* > look like an ACPI table during the procedure above will remain in place, > and the pointers *to* them will remain valid, in those ACPI table copies > that EFI_ACPI_TABLE_PROTOCOL creates. > > For example, consider a BGRT table, where QEMU places both the BGRT in > the blob, and the image data right after it (and sets BGRT.ImageAddress > to the relative address inside the blob where the image data starts). > The above procedure will result in: > > - the BGRT table itself being allocated twice, once by our first pass, > and once by EFI_ACPI_TABLE_PROTOCOL itself (from the second pass). The > first instance will be leaked (it won't be reachable from the system > table that EFI_ACPI_TABLE_PROTOCOL now manages), but it's not grave. > > - the BGRT image data will be allocated only once, from our first pass, > and it will be referenced *both* from our first-pass BGRT table (which > is irrelevant) *and* from the BGRT copy that EFI_ACPI_TABLE_PROTOCOL > installs (which is relevant). > > This approach would leak a few pages of AcpiNVS memory, and it has a > slight chance to call EFI_ACPI_TABLE_PROTOCOL.InstallTable() with > garbage (that has a valid-looking Length field and checksums to zero). > But it would remain compatible with EFI_ACPI_TABLE_PROTOCOL, and it > wouldn't miss anything from QEMU's payload that in fact *is* an ACPI table.
In addition, I could tighten the Length + checksum validation with ACPI_BUILD_APPNAME6 and ACPI_BUILD_APPNAME4 checks, according to qemu's build_header() function -- if Michael agrees that these are stable. IOW, the OEMID would have to be "BOCHS ", and the first four bytes of OEMTableID would have to be "BXPC". I think these four checks together are pretty strong: a static check for a *10-byte* signature (in effect), and a dynamic check for length + checksum. I think this should be acceptable. Thanks, Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
