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

[Michael S. Tsirkin]

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.

-- 
MST