Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU

[Laszlo Ersek]

On 11/22/13 19:10, Jordan Justen wrote:
> On Tue, Nov 12, 2013 at 7:11 AM, Laszlo Ersek <address@hidden> wrote:
>> Qemu v1.7.0-rc0 features an ACPI linker/loader interface, available over
>> fw_cfg, written by Michael Tsirkin.
>>
>> Qemu composes all ACPI tables on the host side, according to the target
>> hardware configuration, and makes the tables available to any guest
>> firmware over fw_cfg.
>>
>> The feature moves the burden of keeping ACPI tables up-to-date from boot
>> firmware to qemu (which is the source of hardware configuration anyway).
>>
>> This patch adds client code for this feature.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <address@hidden>
>> ---
>>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h |   7 +-
>>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c |  12 +-
>>  OvmfPkg/AcpiPlatformDxe/Qemu.c         | 204 
>> +++++++++++++++++++++++++++++++++
>>  3 files changed, 215 insertions(+), 8 deletions(-)
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h 
>> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> index 21107cd..c643fa1 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
>> @@ -10,7 +10,7 @@
>>    THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>>    WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
>> IMPLIED.
>>
>> -**/
>> +**/
>>
>>  #ifndef _ACPI_PLATFORM_H_INCLUDED_
>>  #define _ACPI_PLATFORM_H_INCLUDED_
>> @@ -61,5 +61,10 @@ InstallXenTables (
>>    IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>>    );
>>
>> +EFI_STATUS
>> +EFIAPI
>> +InstallQemuLinkedTables (
>> +  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>> +  );
>>  #endif
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c 
>> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> index 6e0b610..084c393 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
>> @@ -256,16 +256,14 @@ AcpiPlatformEntryPoint (
>>
>>    if (XenDetected ()) {
>>      Status = InstallXenTables (AcpiTable);
>> -    if (EFI_ERROR (Status)) {
>> -      Status = FindAcpiTablesInFv (AcpiTable);
>> -    }
>>    } else {
>> +    Status = InstallQemuLinkedTables (AcpiTable);
>> +  }
>> +
>> +  if (EFI_ERROR (Status)) {
>>      Status = FindAcpiTablesInFv (AcpiTable);
>>    }
>> -  if (EFI_ERROR (Status)) {
>> -    return Status;
>> -  }
>>
>> -  return EFI_SUCCESS;
>> +  return Status;
>>  }
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
>> index 06bd463..c572f8a 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
>> @@ -515,3 +515,207 @@ QemuInstallAcpiTable (
>>             );
>>  }
>>
>> +
>> +/**
>> +  Download the ACPI table data file from QEMU and interpret it.
>> +
>> +  Starting with v1.7.0-rc0, QEMU provides the following three files for 1.7+
>> +  machine types:
>> +  - etc/acpi/rsdp
>> +  - etc/acpi/tables
>> +  - etc/table-loader
>> +
>> +  "etc/acpi/rsdp" and "etc/acpi/tables" are similar, they are only kept
>> +  separate because they have different allocation requirements in SeaBIOS.
>> +
>> +  Both of these fw_cfg files contain preformatted ACPI payload. 
>> "etc/acpi/rsdp"
>> +  contains only the RSDP table, while "etc/acpi/tables" contains all other
>> +  tables, concatenated.
>> +
>> +  The tables in these two files have been filled in by qemu, but two kinds 
>> of
>> +  fields are incomplete in each table: pointers to other tables, and 
>> checksums
>> +  (which depend on the pointers). The pointers are pre-initialized with
>> +  *relative* offsets, but their final values depend on where the actual 
>> files
>> +  will be placed in memory by the guest. That is, the pointer fields need 
>> to be
>> +  "relocated" (incremented) by the base addresses of where "/etc/acpi/rsdp" 
>> and
>> +  "/etc/acpi/tables" will be placed in guest memory.
>> +
>> +  This is where the third file, "/etc/table-loader" comes into the picture. 
>> It
>> +  is a linker/loader script that has several command types:
>> +
>> +    One command type instructs the guest to download the other two files.
>> +
>> +    Another command type instructs the guest to increment ("absolutize") a
>> +    pointer field (having a relative initial value) in some file, with the
>> +    dynamic base address of the same (or another) file.
>> +
>> +    The third command type instructs the guest to compute checksums over
>> +    ranges and to store them.
>> +
>> +  In edk2, EFI_ACPI_TABLE_PROTOCOL knows about table relationships -- it
>> +  handles linkage automatically when a table is installed. The protocol 
>> takes
>> +  care of checksumming too. RSDP is installed automatically. Hence we only 
>> need
>> +  to care about the "etc/acpi/tables" file, determining the boundaries of 
>> each
>> +  table and installing it.
> 
> I'm wondering if some of the information in this comment might have a
> better home in the commit message. Will it help in maintaining the
> code to have it here? Or, maybe a more terse version can live here?
> 
> Of course, I rarely comment my code enough, which is much worse. So,
> feel free to ignore this feedback. :)

I can move the text from "Starting with v1.7.0-rc0..." until here to the
commit message.

> 
>> +  @param[in] AcpiProtocol  The ACPI table protocol used to install tables.
>> +
>> +  @retval  EFI_UNSUPPORTED       Firmware configuration is unavailable.
>> +
>> +  @retval  EFI_NOT_FOUND         The host doesn't export the 
>> "etc/acpi/tables"
>> +                                 fw_cfg file.
>> +
>> +  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed.
>> +
>> +  @return                        Status codes returned by
>> +                                 AcpiProtocol->InstallAcpiTable().
>> +
>> +**/
>> +
>> +//
>> +// We'll be saving the keys of installed tables so that we can roll them 
>> back
>> +// in case of failure. 128 tables should be enough for anyone (TM).
>> +//
>> +#define INSTALLED_TABLES_MAX 128
>> +
>> +//
>> +// This macro produces three arguments for DEBUG(), for the format string
>> +// "%-*.*a" -- left-justify, take field width, take number of characters to
>> +// consume, ASCII character string. The argument must be a valid pointer.
>> +//
>> +#define DBGSTR(ptr) (sizeof *(ptr)), (sizeof *(ptr)), ((CONST CHAR8 *) 
>> (ptr))
>> +
>> +//
>> +// Introduce a macro for the format string as well, bracketed by embedded
>> +// double quotes.
>> +//
>> +#define DBGFMT "\"%-*.*a\""
> 
> I think that needing these macros is a sign that perhaps there are
> excessive debug messages. :)

There's no such thing as an excessive debug message, if you can disable
it :) When communication over a binary protocol goes wrong, the first
thing you add is a hexdump with some human-friendly parsing. I just
don't want to start from zero every time that happens.

> 
>> +EFI_STATUS
>> +EFIAPI
>> +InstallQemuLinkedTables (
>> +  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>> +  )
>> +{
>> +  STATIC CONST CHAR8   Func[] = "InstallQemuLinkedTables";
>> +  EFI_STATUS           Status;
>> +  FIRMWARE_CONFIG_ITEM TablesFile;
>> +  UINTN                TablesFileSize;
>> +  UINT8                *Tables;
>> +  UINTN                *InstalledKey;
>> +  UINTN                Processed;
>> +  INT32                Installed;
>> +
>> +  Status = QemuFwCfgFindFile ("etc/acpi/tables", &TablesFile, 
>> &TablesFileSize);
>> +  if (EFI_ERROR (Status)) {
>> +    DEBUG ((EFI_D_INFO, "%a: \"etc/acpi/tables\" interface unavailable: 
>> %r\n",
>> +      Func, Status));
>> +    return Status;
>> +  }
>> +
>> +  Tables = AllocatePool (TablesFileSize);
>> +  if (Tables == NULL) {
>> +    return EFI_OUT_OF_RESOURCES;
>> +  }
>> +
>> +  QemuFwCfgSelectItem (TablesFile);
>> +  QemuFwCfgReadBytes (TablesFileSize, Tables);
>> +
>> +  InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
>> +  if (InstalledKey == NULL) {
>> +    Status = EFI_OUT_OF_RESOURCES;
>> +    goto FreeTables;
>> +  }
>> +
>> +  Processed = 0;
>> +  Installed = 0;
>> +  while (Processed < TablesFileSize) {
>> +    UINTN                       Remaining;
>> +    EFI_ACPI_DESCRIPTION_HEADER *Probe;
>> +
>> +    Remaining = TablesFileSize - Processed;
>> +    if (Remaining < sizeof *Probe) {
>> +      DEBUG ((EFI_D_VERBOSE, "%a: truncated ACPI table header at offset "
>> +        "0x%Lx\n", Func, (UINT64) Processed));
>> +      Status = EFI_PROTOCOL_ERROR;
>> +      break;
>> +    }
>> +
>> +    Probe = (EFI_ACPI_DESCRIPTION_HEADER *) (Tables + Processed);
>> +    DEBUG ((EFI_D_VERBOSE, "%a: offset 0x%016Lx:"
>> +      " Signature=" DBGFMT
>> +      " Length=0x%08x"
>> +      " Revision=0x%02x"
>> +      " OemId=" DBGFMT
>> +      " OemTableId=" DBGFMT
>> +      " OemRevision=0x%08x"
>> +      " CreatorId=" DBGFMT
>> +      " CreatorRevision=0x%08x\n",
>> +      Func, (UINT64) Processed,
>> +      DBGSTR (&Probe->Signature),
>> +      Probe->Length,
>> +      Probe->Revision,
>> +      DBGSTR (&Probe->OemId),
>> +      DBGSTR (&Probe->OemTableId),
>> +      Probe->OemRevision,
>> +      DBGSTR (&Probe->CreatorId),
>> +      Probe->CreatorRevision));
> 
> Yep. :)
> 
> Do we need to print all those fields?
> 
> Anyway, maybe a better centralized place for this would be:
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> 
> Also, I think we try to keep debug messages under 80 characters.

We seem to think completely differently about debug messages :)

I can cut most of the fields though, and simply keep the signature (and
remove the format string macro too, because for the signature I'd only
use it once). Would that work for you?

I'd rather not try to patch MdeModulePkg without an actual bug to fix.

> 
>> +    if (Remaining < Probe->Length || Probe->Length < sizeof *Probe) {
>> +      DEBUG ((EFI_D_VERBOSE, "%a: invalid ACPI table header at offset 
>> 0x%Lx\n",
>> +        Func, (UINT64) Processed));
>> +      Status = EFI_PROTOCOL_ERROR;
>> +      break;
>> +    }
>> +
>> +    //
>> +    // skip automatically handled "root" tables: RSDT, XSDT
>> +    //
>> +    if (Probe->Signature !=
>> +                        
>> EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE &&
>> +        Probe->Signature !=
>> +                    
>> EFI_ACPI_2_0_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
>> +      if (Installed == INSTALLED_TABLES_MAX) {
>> +        DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n", 
>> Func,
>> +          INSTALLED_TABLES_MAX));
>> +        Status = EFI_OUT_OF_RESOURCES;
>> +        break;
>> +      }
>> +
>> +      Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol, Probe,
>> +                 Probe->Length, &InstalledKey[Installed]);
> 
> We do have a local InstallAcpiTable function. (Just remove "AcpiProtocol->")

Yes, I saw that. But, we don't have a wrapper for the rollback on the
error path (where we uninstall the tables). Since I used AcpiProtocol->
in the rollback part, I wanted to be consistent here.

Of course I can add a wrapper for the uninstall function too :)

Thanks
Laszlo