one irrelevant style comment in the middle
On 05/14/14 00:13, Gabriel L. Somlo wrote:
> Locate QEMU SMBIOS data in fw_cfg and install it via the
> SMBIOS protocol.
>
> Starting with qemu-2.1, on pc/x86 machines of type >= 2.1, full
> SMBIOS tables are generated and inserted into fw_cfg (i.e., no
> per-field patching of locally generated structures is required).
>
> Aside from new code to extract a SMBIOS blob from fw_cfg, this
> patch utilizes the pre-existing (Xen) infrastructure to handle
> final SMBIOS table creation.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gabriel Somlo <[email protected]>
> ---
> OvmfPkg/SmbiosPlatformDxe/Qemu.c | 87
> +++++++++++++++++++++++++
> OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c | 24 +++++--
> OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.h | 23 +++++++
> OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf | 3 +
> 4 files changed, 132 insertions(+), 5 deletions(-)
> create mode 100644 OvmfPkg/SmbiosPlatformDxe/Qemu.c
>
> diff --git a/OvmfPkg/SmbiosPlatformDxe/Qemu.c
> b/OvmfPkg/SmbiosPlatformDxe/Qemu.c
> new file mode 100644
> index 0000000..f18e2c1
> --- /dev/null
> +++ b/OvmfPkg/SmbiosPlatformDxe/Qemu.c
> @@ -0,0 +1,87 @@
> +/** @file
> + Find and extract QEMU SMBIOS data from fw_cfg.
> +
> + Copyright (C) 2014, Gabriel L. Somlo <[email protected]>
> +
> + 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.
> +**/
> +
> +#include "SmbiosPlatformDxe.h"
> +#include <Library/QemuFwCfgLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +
> +//
> +// fw_cfg file names for the SMBIOS entry point and tables
> +//
> +#define QEMU_ANCHOR "etc/smbios/smbios-anchor"
> +#define QEMU_TABLES "etc/smbios/smbios-tables"
> +
> +/**
> + Locates and extracts the QEMU SMBIOS data if present in fw_cfg
> +
> + @return Address of extracted QEMU SMBIOS data
> +
> +**/
> +UINT8 *
> +GetQemuSmbiosTables (
> + VOID
> + )
> +{
> + SMBIOS_TABLE_ENTRY_POINT QemuAnchor;
> + FIRMWARE_CONFIG_ITEM Anchor, Tables;
> + UINTN AnchorSz, TablesSz;
> + UINT8 *QemuTables;
> +
> + if (QemuFwCfgFindFile (QEMU_ANCHOR, &Anchor, &AnchorSz) != RETURN_SUCCESS
> ||
> + QemuFwCfgFindFile (QEMU_TABLES, &Tables, &TablesSz) != RETURN_SUCCESS
> ||
> + AnchorSz != sizeof (QemuAnchor) ||
> + TablesSz == 0) {
> + return NULL;
> + }
> +
> + //
> + // We copy the entry point structure to perform some additional checks,
> + // but discard it upon return.
> + //
> + QemuFwCfgSelectItem (Anchor);
> + QemuFwCfgReadBytes (AnchorSz, &QemuAnchor);
> +
> + if (AsciiStrnCmp ((CHAR8 *)QemuAnchor.AnchorString, "_SM_", 4) ||
> + AsciiStrnCmp ((CHAR8 *)QemuAnchor.IntermediateAnchorString, "_DMI_",
> 5) ||
> + TablesSz != QemuAnchor.TableLength) {
> + return NULL;
> + }
> +
> + QemuTables = AllocatePool (TablesSz);
> + if (QemuTables == NULL) {
> + return NULL;
> + }
> +
> + QemuFwCfgSelectItem (Tables);
> + QemuFwCfgReadBytes (TablesSz, QemuTables);
> +
> + return QemuTables;
> +}
> +
> +/**
> + Frees the dynamically allocated portion of the extracted QEMU SMBIOS data
> +
> + @param[in] QemuTables Address of QEMU SMBIOS data from
> GetQemuSmbiosTables()
> +
> + It is the caller's responsibility to ensure QemuTables was allocated and
> + returned from GetQemuSmbiosTables().
> +
> + **/
Normally the comment block on a function has the following structure:
- short subject
- a bit more elaboration
- param list
- retval list
In this case this would mean moving the responsibility language just
below the subject line. But I explicitly don't want you to respin just
because of this.
> +VOID
> +FreeQemuSmbiosTables (
> + IN UINT8 *QemuTables
> + )
> +{
> + FreePool (QemuTables);
> +}
> diff --git a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> index ac48fb7..9d3f515 100644
> --- a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> +++ b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.c
> @@ -84,20 +84,20 @@ SmbiosTableLength (
> Install all structures from the given SMBIOS structures block
>
> @param Smbios SMBIOS protocol
> - @param EntryPointStructure SMBIOS entry point structures block
> + @param TableAddress SMBIOS tables starting address
>
> **/
> EFI_STATUS
> InstallAllStructures (
> IN EFI_SMBIOS_PROTOCOL *Smbios,
> - IN SMBIOS_TABLE_ENTRY_POINT *EntryPointStructure
> + IN UINT8 *TableAddress
> )
> {
> EFI_STATUS Status;
> SMBIOS_STRUCTURE_POINTER SmbiosTable;
> EFI_SMBIOS_HANDLE SmbiosHandle;
>
> - SmbiosTable.Raw = (UINT8*)(UINTN) EntryPointStructure->TableAddress;
> + SmbiosTable.Raw = TableAddress;
> if (SmbiosTable.Raw == NULL) {
> return EFI_INVALID_PARAMETER;
> }
> @@ -145,6 +145,7 @@ SmbiosTablePublishEntry (
> EFI_STATUS Status;
> EFI_SMBIOS_PROTOCOL *Smbios;
> SMBIOS_TABLE_ENTRY_POINT *EntryPointStructure;
> + UINT8 *SmbiosTables;
>
> //
> // Find the SMBIOS protocol
> @@ -159,11 +160,24 @@ SmbiosTablePublishEntry (
> }
>
> //
> - // Add Xen SMBIOS data if found
> + // Add Xen or QEMU SMBIOS data if found
> //
> EntryPointStructure = GetXenSmbiosTables ();
> if (EntryPointStructure != NULL) {
> - Status = InstallAllStructures (Smbios, EntryPointStructure);
> + SmbiosTables = (UINT8*)(UINTN)EntryPointStructure->TableAddress;
> + } else {
> + SmbiosTables = GetQemuSmbiosTables ();
> + }
> +
> + if (SmbiosTables != NULL) {
> + Status = InstallAllStructures (Smbios, SmbiosTables);
> +
> + //
> + // Free SmbiosTables if allocated by Qemu (i.e., NOT by Xen):
> + //
> + if (EntryPointStructure == NULL) {
> + FreeQemuSmbiosTables (SmbiosTables);
> + }
> }
>
> return Status;
> diff --git a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> index bf99e43..8d37355 100644
> --- a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> +++ b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.h
> @@ -40,6 +40,29 @@ GetXenSmbiosTables (
>
>
> /**
> + Locates and extracts the QEMU SMBIOS table data if present in fw_cfg
> +
> + @return Address of extracted QEMU SMBIOS data
> +
> +**/
> +UINT8 *
> +GetQemuSmbiosTables (
> + VOID
> + );
> +
> +/**
> + Frees the dynamically allocated portion of the extracted QEMU SMBIOS data
> +
> + @param[in] QemuTables Address of QEMU SMBIOS data from
> GetQemuSmbiosTables()
> +
> +**/
> +VOID
> +FreeQemuSmbiosTables (
> + IN UINT8 *QemuTables
> + );
> +
> +
> +/**
> Validates the SMBIOS entry point structure
>
> @param EntryPointStructure SMBIOS entry point structure
> diff --git a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> index 7058284..6596392 100644
> --- a/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> +++ b/OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
> @@ -32,6 +32,7 @@
> SmbiosPlatformDxe.h
> SmbiosPlatformDxe.c
> Xen.c
> + Qemu.c
>
> [Packages]
> MdePkg/MdePkg.dec
> @@ -45,6 +46,8 @@
> UefiDriverEntryPoint
> DebugLib
> HobLib
> + QemuFwCfgLib
> + MemoryAllocationLib
>
> [Protocols]
> gEfiSmbiosProtocolGuid # PROTOCOL ALWAYS_CONSUMED
>
I welcome this patchset (and especially your matching qemu-side work).
It unifies SMBIOS handling in OVMF for Xen and Qemu, and (similarly to
ACPI tables) the burden to provide up-to-date payload is now fully on
the hypervisor / emulator.
Reviewed-by: Laszlo Ersek <[email protected]>
Jordan, what are your thoughts?
Thanks
Laszlo
------------------------------------------------------------------------------
"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
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
