Re: [Qemu-devel] [edk2] [PATCH 4/4] OvmfPkg: AcpiPlatformDxe: implement QEMU's full ACPI table loader interface

[Laszlo Ersek]

On 09/07/14 10:50, Michael S. Tsirkin wrote:
> On Fri, Sep 05, 2014 at 10:52:16AM +0200, Laszlo Ersek wrote:
>> On 08/26/14 02:12, Jordan Justen wrote:
>>> On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <address@hidden> wrote:
>>>> On 08/26/14 00:24, Jordan Justen wrote:
>>>>> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <address@hidden> 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.
>>
>> Deal?
>>
>> Laszlo
> 
> Overall, I agree, but I think it's better to make this more robust.
> Let's pass a flag to help EFI idenitify ACPI versus non ACPI tables
> as they are allocated.
> I can see us sticking this info in alloc command: we have this old idea
> to allocate some tables out of E820_ACPI, and this continues this idea,
> and has the advantage of forcing people to allocate these separately so
> you will then be able to free the ACPI tables instead of leaking them.

Okay, thanks.

The v2 series that I posted at

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/9579/focus=9578

and is also visible at

https://github.com/lersek/edk2/commits/acpi_full_v2

uses the "BLOB.HostsOnlyTableData" boolean field to track if we find any
pointers (pointing into the given fw_cfg file) that does *not* turn out
to point to an ACPI table (at which moment the field in the pointed-to
BLOB is set to FALSE).

If, after processing the complete linker/loader script, this field still
has its initial TRUE value, then we free the blob at the end, even in v2.

In my current testing (see the log excerpt in the v2 blurb), both
"etc/acpi/rsdp" and "etc/acpi/tables" turn out to host only ACPI tables
(because there happen to be no pointers into the former at all, and all
pointers into the latter happen to identify ACPI tables, as the parser
finds), hence both fw_cfg blobs are freed at the end, and nothing is
leaked, even in v2.

IIUC, you're talking about a new field in the allocate command, or maybe
some new values / bits in the allocation zone field. I can imagine three
kinds of promises:
(a) "contains only ACPI table data"
(b) "contains only non-ACPI table data"
(c) "mixed"

(Each three would have to be associated with a nonzero value, for
compatibility. Case (c) ("mixed") also includes the case when this info
is not yet available -- ie. running on an earlier qemu.)

I'd store this tri-state in a new enum field of BLOB. Let's call it
provisionally BLOB.ContentsType.

In case (c) ("mixed, or info unavailable"), the current v2 logic using
BLOB.HostsOnlyTableData would remain intact.

In case (b) ("contains only non-ACPI table data"), I'd short-circuit the
ACPI table header probing in Process2ndPassCmdAddPointer(), after
locating the pointed-to blob and seeing that Blob2->ContentsType ==
CASE_B. Meaning, the code would proceed straight to setting
Blob2->ostsOnlyTableData = FALSE, and ultimately the corresponding
fw_cfg file would be preserved.

In case (a) ("contains only ACPI table data"), the current logic using
BLOB.HostsOnlyTableData would have to be preserved and remain intact
again. This is because:

- I still have to parse into the target blob in order to see the type
  of the table living there. Minimally, the FACS has a different header
  layout from the rest, and I need the table size field to install the
  table with EFI_ACPI_TABLE_PROTOCOL.

- As a corollary to having to continue to parse, it is possible that
  this parsing would fail to identify the table, *iff* QEMU at some
  point provided an ACPI table whose header format was different from
  *both* the one of FACS *and* the normal table desc header.

  When this happens, the code needs to be able to set
  HostsOnlyTableData to FALSE, just the same as it does in v2; even
  though the BLOB has been promised to contain only ACPI table data.
  Because this way the pointed-to fw_cfg file would be preserved (same
  as in v2), which would be the right thing to do.

  Namely, we wouldn't be able to install such a funky table with
  EFI_ACPI_TABLE_PROTOCOL. However, the table would remain safe & sound
  in AcpiNVS type memory, from the first pass, and remain pointed-to by
  whatever *other* table that references it (and that we installed with
  EFI_ACPI_TABLE_PROTOCOL). Meaning, the table surviving in the first
  pass allocation could still be reached by the operating system later.

  This is the reason why I'd have to keep the current (v2) code around
  even for case (a): despite the promise in the new field of the
  Allocate command, OVMF's header parsing heuristics could nonetheless
  run into something unexpected, and then it's better to preserve the
  unknown-format table in the first-pass AcpiNVS area than to lose it.

Given these, additional client code for the new info is a straight
upgrade from v2. Once you implement this extra info in the Allocate
command (with nonzero values for compatibility),
- I'd introduce BLOB.ContentsType,
- set it along the other BLOB fields, in ProcessCmdAllocate(),
- and in Process2ndPassCmdAddPointer() I'd short-circuit the parsing
  if Blob2.ContentsType == CASE_B, and set Blob2->HostsOnlyTableData =
  FALSE at once.

This implies two things:
- the proposed new bit of info would conditionally let OVMF save some
  parsing, but it wouldn't allow it to drop any code,
- and I could implement the additional client logic incrementally,
  after v2 is committed.

Thanks,
Laszlo