Re: [edk2] [PATCH 3/3] OvmfPkg: AcpiPlatformDxe: don't rely on unstable QEMU interface

Subject: Re: [edk2] [PATCH 3/3] OvmfPkg: AcpiPlatformDxe: don't rely on unstable QEMU interface

From: Jordan Justen <jljusten@gmail.com>

To: Laszlo Ersek <lersek@redhat.com>

Date: 2014-06-16 08:10:27

On Tue, May 20, 2014 at 3:52 PM, Laszlo Ersek  wrote:
> The fw_cfg file "etc/acpi/tables" is not a stable guest interface -- QEMU
> could rename it in the future, and/or introduce additional fw_cfg files
> with ACPI payload. Only the higher-level "etc/table-loader" file is
> considered stable, which contains a sequence of loader/linker commands, to
> be executed by the firmware.

How does this sound instead: ... "which contains a sequence of
commands to assist firmware with reading QEMU ACPI tables from the
FwCfg interface."

> Because edk2 provides automatic linking and checksumming for ACPI tables,

"automatic linking and checksumming" => "publishing support" ?

> we only handle the Allocate commands (of which the prior OVMF
> functionality is a subset).

> Even in the Allocate commands we ignore the
> allocation hints (Alignment and Zone); they only need manual handling in
> SeaBIOS.

How does this sound instead: "OVMF only uses the Allocate command to
find the names of FwCfg files to read and publish as ACPI tables."

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  OvmfPkg/AcpiPlatformDxe/QemuLoader.h | 86 ++++++++++++++++++++++++++++++++++++
>  OvmfPkg/AcpiPlatformDxe/Qemu.c       | 54 +++++++++++++++++++---
>  2 files changed, 135 insertions(+), 5 deletions(-)
>  create mode 100644 OvmfPkg/AcpiPlatformDxe/QemuLoader.h
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuLoader.h b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
> new file mode 100644
> index 0000000..7b69f90
> --- /dev/null
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuLoader.h
> @@ -0,0 +1,86 @@
> +/** @file
> +  Command structures for the QEMU linker/loader interface.
> +
> +  Copyright (C) 2014, Red Hat, Inc.
> +
> +  This program and the accompanying materials are licensed and made available
> +  under the terms and conditions of the BSD License which accompanies this
> +  distribution.  The full text of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  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 __QEMU_LOADER_H__
> +#define __QEMU_LOADER_H__
> +
> +#include 
> +
> +//
> +// The types and the documentation reflects the SeaBIOS interface. In OVMF we
> +// use a minimal subset of it.
> +//
> +#define QEMU_LOADER_FNAME_SIZE 56

Where does this number come from?

> +
> +typedef enum {
> +  QemuLoaderCmdAllocate = 1,

Maybe a comment around here saying that we only look at the Allocate
command, and only to get FwCfg filenames?

With those changes:
Series Reviewed-by: Jordan Justen 

> +  QemuLoaderCmdAddPointer,
> +  QemuLoaderCmdAddChecksum
> +} QEMU_LOADER_COMMAND_TYPE;
> +
> +typedef enum {
> +  QemuLoaderAllocHigh = 1,
> +  QemuLoaderAllocFSeg
> +} QEMU_LOADER_ALLOC_ZONE;
> +
> +#pragma pack (1)
> +//
> +// QemuLoaderCmdAllocate: download the fw_cfg file named File, to a buffer
> +// allocated in the zone specified by Zone, aligned at a multiple of Alignment.
> +//
> +typedef struct {
> +  UINT8  File[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated
> +  UINT32 Alignment;                    // power of two
> +  UINT8  Zone;                         // QEMU_LOADER_ALLOC_ZONE values
> +} QEMU_LOADER_ALLOCATE;
> +
> +//
> +// QemuLoaderCmdAddPointer: the bytes at
> +// [PointerOffset..PointerOffset+PointerSize) in the file PointerFile contain a
> +// relative pointer (an offset) into PointeeFile. Increment the relative
> +// pointer's value by the base address of where PointeeFile's contents have
> +// been placed (when QemuLoaderCmdAllocate has been executed for PointeeFile).
> +//
> +typedef struct {
> +  UINT8  PointerFile[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated
> +  UINT8  PointeeFile[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated
> +  UINT32 PointerOffset;
> +  UINT8  PointerSize;                         // one of 1, 2, 4, 8
> +} QEMU_LOADER_ADD_POINTER;
> +
> +//
> +// QemuLoaderCmdAddChecksum: calculate the UINT8 checksum (as per
> +// CalculateChecksum8()) of the range [Start..Start+Length) in File. Store the
> +// UINT8 result at ResultOffset in the same File.
> +//
> +typedef struct {
> +  UINT8  File[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated
> +  UINT32 ResultOffset;
> +  UINT32 Start;
> +  UINT32 Length;
> +} QEMU_LOADER_ADD_CHECKSUM;
> +
> +typedef struct {
> +  UINT32 Type;                             // QEMU_LOADER_COMMAND_TYPE values
> +  union {
> +    QEMU_LOADER_ALLOCATE     Allocate;
> +    QEMU_LOADER_ADD_POINTER  AddPointer;
> +    QEMU_LOADER_ADD_CHECKSUM AddChecksum;
> +    UINT8                    Padding[124];
> +  } Command;
> +} QEMU_LOADER_ENTRY;
> +#pragma pack ()
> +
> +#endif
> diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> index 5a96d76..70f3ff6 100644
> --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
> +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> @@ -15,8 +15,9 @@
>
>  **/
>
>  #include "AcpiPlatform.h"
> +#include "QemuLoader.h"
>  #include 
>  #include 
>  #include 
>  #include 
> @@ -794,10 +795,9 @@ InstallQemuLinkedTables (
>
>    @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed, or more than
>                                   INSTALLED_TABLES_MAX tables found.
>
> -  @retval  EFI_PROTOCOL_ERROR    Found truncated or invalid ACPI table header
> -                                 in the fw_cfg contents.
> +  @retval  EFI_PROTOCOL_ERROR    Found invalid fw_cfg contents.
>
>    @return                        Status codes returned by
>                                   AcpiProtocol->InstallAcpiTable().
>
> @@ -811,19 +811,62 @@ InstallAllQemuLinkedTables (
>  {
>    UINTN                *InstalledKey;
>    INT32                Installed;
>    EFI_STATUS           Status;
> +  FIRMWARE_CONFIG_ITEM LoaderItem;
> +  UINTN                LoaderSize;
> +  UINT8                *Loader;
> +  QEMU_LOADER_ENTRY    *Entry, *End;
>
>    InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
>    if (InstalledKey == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
>    Installed = 0;
>
> -  Status = InstallQemuLinkedTables ("etc/acpi/tables", AcpiProtocol,
> -             InstalledKey, &Installed);
> +  Status = QemuFwCfgFindFile ("etc/table-loader", &LoaderItem, &LoaderSize);
> +  if (EFI_ERROR (Status)) {
> +    goto FreeInstalledKey;
> +  }
> +  if (LoaderSize % sizeof *Entry != 0) {
> +    Status = EFI_PROTOCOL_ERROR;
> +    goto FreeInstalledKey;
> +  }
> +
> +  Loader = AllocatePool (LoaderSize);
> +  if (Loader == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto FreeInstalledKey;
> +  }
> +
> +  QemuFwCfgSelectItem (LoaderItem);
> +  QemuFwCfgReadBytes (LoaderSize, Loader);
> +
> +  Entry = (QEMU_LOADER_ENTRY *)Loader;
> +  End   = (QEMU_LOADER_ENTRY *)(Loader + LoaderSize);
> +  while (Entry < End) {
> +    if (Entry->Type == QemuLoaderCmdAllocate) {
> +      QEMU_LOADER_ALLOCATE *Allocate;
> +
> +      Allocate = &Entry->Command.Allocate;
> +      if (Allocate->File[sizeof Allocate->File - 1] != '\0') {
> +        Status = EFI_PROTOCOL_ERROR;
> +        break;
> +      }
> +
> +      Status = InstallQemuLinkedTables ((CHAR8 *)Allocate->File, AcpiProtocol,
> +                 InstalledKey, &Installed);
> +      if (EFI_ERROR (Status)) {
> +        ASSERT (Status != EFI_INVALID_PARAMETER);
> +        break;
> +      }
> +    }
> +    ++Entry;
> +  }
> +
> +  FreePool (Loader);
> +
>    if (EFI_ERROR (Status)) {
> -    ASSERT (Status != EFI_INVALID_PARAMETER);
>      //
>      // Roll back partial installation.
>      //
>      while (Installed > 0) {
> @@ -833,7 +876,8 @@ InstallAllQemuLinkedTables (
>    } else {
>      DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>    }
>
> +FreeInstalledKey:
>    FreePool (InstalledKey);
>    return Status;
>  }
> --
> 1.8.3.1
>
>
> ------------------------------------------------------------------------------
> "Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE
> Instantly run your Selenium tests across 300+ browser/OS combos.
> Get unparalleled scalability from the best Selenium testing platform available
> Simple to use. Nothing to install. Get started now for free."
> http://p.sf.net/sfu/SauceLabs
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions
Find What Matters Most in Your Big Data with HPCC Systems
Open Source. Fast. Scalable. Simple. Ideal for Dirty Data.
Leverages Graph Analysis for Fast Processing & Easy Data Exploration
http://p.sf.net/sfu/hpccsystems
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel