On 05/24/14 09:39, Jordan Justen wrote:
> On Tue, May 20, 2014 at 3:52 PM, Laszlo Ersek <ler...@redhat.com> 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.
>
> I'm not sure I understand the term 'link' being used in this context.
Linking in this context means setting pointers in one fw_cfg file to
another (incrementing the prior, fw_cfg file-relative (==offset) value
of the pointer with the base address of the target file, thereby making
the pointer absolute).
The word "link" is used as in "linked list"; tables in the fw_cfg files
are linked (connected) together by pointers. I think "linked list" is
common term, but I'm not a native speaker.
Also, more technically, relocation of object files (updating target
addresses based on relocation entries) is part of what a binary linker
does, and a very similar thing happens here. QEMU_LOADER_ADD_POINTER is
actually a relocation entry, and when the client code processes them
(which this patchset doesn't do, but SeaBIOS does), it is "linking".
> Executed sounds a bit odd too.
>
> Essentially fw-cfg allows OVMF to read some blobs from QEMU and plop
> them down in guest memory.
Linking is important in SeaBIOS and it is part of the interface, we just
process the data a bit differently (without interpreting the
QEMU_LOADER_ADD_POINTER ("linker") commands), because the full interface
doesn't match OVMF very well.
> So, loading, tables, blobs and maybe
> interpret are terms that make sense to me. But I think link and
> execute are confusing (to say the least).
I can replace "execute" with "interpret". And, although I think "link"
is appropriate as far as the command proper goes, it's just one word in
the commit message and I can simply remove it.
>
> Something like a network interface 'link' status makes sense, and is
> well known in that context.
Other than this, do you find the code acceptable? (I'm asking to
determine the scope of changes in v2 -- just commit msg wording or code
tweaks too.)
Thanks!
Laszlo
> -Jordan
>
>> Because edk2 provides automatic linking and checksumming for ACPI tables,
>> 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.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>> 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 <Include/Base.h>
>> +
>> +//
>> +// The types and the documentation reflects the SeaBIOS interface. In OVMF
>> we
>> +// use a minimal subset of it.
>> +//
>> +#define QEMU_LOADER_FNAME_SIZE 56
>> +
>> +typedef enum {
>> + QemuLoaderCmdAllocate = 1,
>> + 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 <Library/BaseMemoryLib.h>
>> #include <Library/MemoryAllocationLib.h>
>> #include <Library/QemuFwCfgLib.h>
>> #include <Library/DxeServicesTableLib.h>
>> @@ -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
------------------------------------------------------------------------------
The best possible search technologies are now affordable for all companies.
Download your FREE open source Enterprise Search Engine today!
Our experts will assist you in its installation for $59/mo, no commitment.
Test it for FREE on our Cloud platform anytime!
http://pubads.g.doubleclick.net/gampad/clk?id=145328191&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
