[PATCH 0/5] OvmfPkg/QemuFwCfgLib: support the DMA-like interface

[PATCH 0/5] OvmfPkg/QemuFwCfgLib: support the DMA-like interface

[Laszlo Ersek]

I plan to use the DMA interface of QEMU's fw_cfg in upcoming features
(one short term, another long term).

The first four patches in the series refactor the current library
instances (and even the lib class) slightly, while the last patch adds
the feature to OVMF.

Repo:   https://github.com/lersek/edk2/
Branch: ovmf_fwcfg_dma

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>

Thanks
Laszlo

Laszlo Ersek (5):
  ArmVirtPkg/QemuFwCfgLib: remove superfluous
    InternalQemuFwCfgIsAvailable()
  OvmfPkg/QemuFwCfgLib: move InternalQemuFwCfgIsAvailable() to lib
    instances
  OvmfPkg/IndustryStandard: add QemuFwCfgDma.h
  ArmVirtPkg/QemuFwCfgLib: rebase lib instance to
    OvmfPkg/IndustryStandard
  OvmfPkg/QemuFwCfgLib: support QEMU's DMA-like fw_cfg access method

 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c      | 55 +++-----------
 OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h     | 50 +++++++++++++
 OvmfPkg/Include/Library/QemuFwCfgLib.h              | 16 -----
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         | 75 ++++++++++++++++++++
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf       |  1 +
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 46 ++++++++++++
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c      | 29 +++++++-
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         | 17 ++++-
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf    |  1 +
 9 files changed, 225 insertions(+), 65 deletions(-)
 create mode 100644 OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
 create mode 100644 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h

-- 
2.9.2

[PATCH 1/5] ArmVirtPkg/QemuFwCfgLib: remove superfluous InternalQemuFwCfgIsAvailable()

[Laszlo Ersek]

InternalQemuFwCfgIsAvailable() is an API that is incorrectly exposed by
the "OvmfPkg/Include/Library/QemuFwCfgLib.h" library class header; the API
is meant to be used internally to library instances (if it's needed at
all). ArmVirtPkg's instance has no use for it actually, so simplify the
code and remove the function definition.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 31 ++++----------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 8ecbe3fb5fe6..2fd8d9050566 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -75,25 +75,6 @@ typedef struct {
 
 
 /**
-  Returns a boolean indicating if the firmware configuration interface is
-  available for library-internal purposes.
-
-  This function never changes fw_cfg state.
-
-  @retval TRUE   The interface is available internally.
-  @retval FALSE  The interface is not available internally.
-**/
-BOOLEAN
-EFIAPI
-InternalQemuFwCfgIsAvailable (
-  VOID
-  )
-{
-  return (BOOLEAN)(mFwCfgSelectorAddress != 0 && mFwCfgDataAddress != 0);
-}
-
-
-/**
   Returns a boolean indicating if the firmware configuration interface
   is available or not.
 
@@ -109,7 +90,7 @@ QemuFwCfgIsAvailable (
   VOID
   )
 {
-  return InternalQemuFwCfgIsAvailable ();
+  return (BOOLEAN)(mFwCfgSelectorAddress != 0 && mFwCfgDataAddress != 0);
 }
 
 
@@ -187,7 +168,7 @@ QemuFwCfgInitialize (
     FwCfgDmaAddress = 0;
   }
 
-  if (InternalQemuFwCfgIsAvailable ()) {
+  if (QemuFwCfgIsAvailable ()) {
     UINT32 Signature;
 
     QemuFwCfgSelectItem (QemuFwCfgItemSignature);
@@ -231,7 +212,7 @@ QemuFwCfgSelectItem (
   IN FIRMWARE_CONFIG_ITEM QemuFwCfgItem
   )
 {
-  if (InternalQemuFwCfgIsAvailable ()) {
+  if (QemuFwCfgIsAvailable ()) {
     MmioWrite16 (mFwCfgSelectorAddress, SwapBytes16 ((UINT16)QemuFwCfgItem));
   }
 }
@@ -360,7 +341,7 @@ QemuFwCfgReadBytes (
   IN VOID  *Buffer
   )
 {
-  if (InternalQemuFwCfgIsAvailable ()) {
+  if (QemuFwCfgIsAvailable ()) {
     InternalQemuFwCfgReadBytes (Size, Buffer);
   } else {
     ZeroMem (Buffer, Size);
@@ -384,7 +365,7 @@ QemuFwCfgWriteBytes (
   IN VOID                   *Buffer
   )
 {
-  if (InternalQemuFwCfgIsAvailable ()) {
+  if (QemuFwCfgIsAvailable ()) {
     UINTN Idx;
 
     for (Idx = 0; Idx < Size; ++Idx) {
@@ -494,7 +475,7 @@ QemuFwCfgFindFile (
   UINT32 Count;
   UINT32 Idx;
 
-  if (!InternalQemuFwCfgIsAvailable ()) {
+  if (!QemuFwCfgIsAvailable ()) {
     return RETURN_UNSUPPORTED;
   }
 
-- 
2.9.2

Re: [PATCH 1/5] ArmVirtPkg/QemuFwCfgLib: remove superfluous InternalQemuFwCfgIsAvailable()

[Leif Lindholm]

On Thu, Dec 01, 2016 at 06:56:29PM +0100, Laszlo Ersek wrote:
> InternalQemuFwCfgIsAvailable() is an API that is incorrectly exposed by
> the "OvmfPkg/Include/Library/QemuFwCfgLib.h" library class header; the API
> is meant to be used internally to library instances (if it's needed at
> all). ArmVirtPkg's instance has no use for it actually, so simplify the
> code and remove the function definition.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Looks sane to me.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 31 ++++----------------
>  1 file changed, 6 insertions(+), 25 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index 8ecbe3fb5fe6..2fd8d9050566 100644
> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> @@ -75,25 +75,6 @@ typedef struct {
>  
>  
>  /**
> -  Returns a boolean indicating if the firmware configuration interface is
> -  available for library-internal purposes.
> -
> -  This function never changes fw_cfg state.
> -
> -  @retval TRUE   The interface is available internally.
> -  @retval FALSE  The interface is not available internally.
> -**/
> -BOOLEAN
> -EFIAPI
> -InternalQemuFwCfgIsAvailable (
> -  VOID
> -  )
> -{
> -  return (BOOLEAN)(mFwCfgSelectorAddress != 0 && mFwCfgDataAddress != 0);
> -}
> -
> -
> -/**
>    Returns a boolean indicating if the firmware configuration interface
>    is available or not.
>  
> @@ -109,7 +90,7 @@ QemuFwCfgIsAvailable (
>    VOID
>    )
>  {
> -  return InternalQemuFwCfgIsAvailable ();
> +  return (BOOLEAN)(mFwCfgSelectorAddress != 0 && mFwCfgDataAddress != 0);
>  }
>  
>  
> @@ -187,7 +168,7 @@ QemuFwCfgInitialize (
>      FwCfgDmaAddress = 0;
>    }
>  
> -  if (InternalQemuFwCfgIsAvailable ()) {
> +  if (QemuFwCfgIsAvailable ()) {
>      UINT32 Signature;
>  
>      QemuFwCfgSelectItem (QemuFwCfgItemSignature);
> @@ -231,7 +212,7 @@ QemuFwCfgSelectItem (
>    IN FIRMWARE_CONFIG_ITEM QemuFwCfgItem
>    )
>  {
> -  if (InternalQemuFwCfgIsAvailable ()) {
> +  if (QemuFwCfgIsAvailable ()) {
>      MmioWrite16 (mFwCfgSelectorAddress, SwapBytes16 ((UINT16)QemuFwCfgItem));
>    }
>  }
> @@ -360,7 +341,7 @@ QemuFwCfgReadBytes (
>    IN VOID  *Buffer
>    )
>  {
> -  if (InternalQemuFwCfgIsAvailable ()) {
> +  if (QemuFwCfgIsAvailable ()) {
>      InternalQemuFwCfgReadBytes (Size, Buffer);
>    } else {
>      ZeroMem (Buffer, Size);
> @@ -384,7 +365,7 @@ QemuFwCfgWriteBytes (
>    IN VOID                   *Buffer
>    )
>  {
> -  if (InternalQemuFwCfgIsAvailable ()) {
> +  if (QemuFwCfgIsAvailable ()) {
>      UINTN Idx;
>  
>      for (Idx = 0; Idx < Size; ++Idx) {
> @@ -494,7 +475,7 @@ QemuFwCfgFindFile (
>    UINT32 Count;
>    UINT32 Idx;
>  
> -  if (!InternalQemuFwCfgIsAvailable ()) {
> +  if (!QemuFwCfgIsAvailable ()) {
>      return RETURN_UNSUPPORTED;
>    }
>  
> -- 
> 2.9.2
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 2/5] OvmfPkg/QemuFwCfgLib: move InternalQemuFwCfgIsAvailable() to lib instances

[Laszlo Ersek]

InternalQemuFwCfgIsAvailable() is an API that is incorrectly exposed by
the "OvmfPkg/Include/Library/QemuFwCfgLib.h" library class header; the API
is meant to be used internally to library instances (if it's needed at
all).

In OvmfPkg, we have two lib instances (for SEC and PEI/DXE); they provide
different implementations of InternalQemuFwCfgIsAvailable(), for the
shared file "OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c". Move the API
declaration to a new internal header called "QemuFwCfgLibInternal.h", and
drop EFIAPI in the process.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf       |  1 +
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf    |  1 +
 OvmfPkg/Include/Library/QemuFwCfgLib.h              | 16 ----------
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 33 ++++++++++++++++++++
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         |  2 ++
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c      |  3 +-
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         |  2 +-
 7 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
index a95e1e730c2c..66ac77850915 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
@@ -32,6 +32,7 @@ [Defines]
 #
 
 [Sources]
+  QemuFwCfgLibInternal.h
   QemuFwCfgLib.c
   QemuFwCfgPeiDxe.c
 
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
index 03a659c9b082..c1d6a54b1a39 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSecLib.inf
@@ -30,6 +30,7 @@ [Defines]
 #
 
 [Sources]
+  QemuFwCfgLibInternal.h
   QemuFwCfgLib.c
   QemuFwCfgSec.c
 
diff --git a/OvmfPkg/Include/Library/QemuFwCfgLib.h b/OvmfPkg/Include/Library/QemuFwCfgLib.h
index baaa257d6188..7c29422fbd72 100644
--- a/OvmfPkg/Include/Library/QemuFwCfgLib.h
+++ b/OvmfPkg/Include/Library/QemuFwCfgLib.h
@@ -206,22 +206,6 @@ QemuFwCfgFindFile (
 
 
 /**
-  Returns a boolean indicating if the firmware configuration interface is
-  available for library-internal purposes.
-
-  This function never changes fw_cfg state.
-
-  @retval    TRUE   The interface is available internally.
-  @retval    FALSE  The interface is not available internally.
-**/
-BOOLEAN
-EFIAPI
-InternalQemuFwCfgIsAvailable (
-  VOID
-  );
-
-
-/**
   Determine if S3 support is explicitly enabled.
 
   @retval  TRUE   if S3 support is explicitly enabled.
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
new file mode 100644
index 000000000000..5b162bf98739
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
@@ -0,0 +1,33 @@
+/** @file
+  Internal interfaces specific to the QemuFwCfgLib instances in OvmfPkg.
+
+  Copyright (C) 2016, 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_FW_CFG_LIB_INTERNAL_H__
+#define __QEMU_FW_CFG_LIB_INTERNAL_H__
+
+/**
+  Returns a boolean indicating if the firmware configuration interface is
+  available for library-internal purposes.
+
+  This function never changes fw_cfg state.
+
+  @retval    TRUE   The interface is available internally.
+  @retval    FALSE  The interface is not available internally.
+**/
+BOOLEAN
+InternalQemuFwCfgIsAvailable (
+  VOID
+  );
+
+#endif
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 5c96d2af2532..804d5b0e42be 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -22,6 +22,8 @@
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 
+#include "QemuFwCfgLibInternal.h"
+
 
 /**
   Reads an 8-bit I/O port fifo into a block of memory.
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
index f693cff29e01..88d88c0edf69 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
@@ -17,6 +17,8 @@
 #include <Library/DebugLib.h>
 #include <Library/QemuFwCfgLib.h>
 
+#include "QemuFwCfgLibInternal.h"
+
 STATIC BOOLEAN mQemuFwCfgSupported = FALSE;
 
 
@@ -83,7 +85,6 @@ QemuFwCfgInitialize (
   @retval    FALSE  The interface is not available internally.
 **/
 BOOLEAN
-EFIAPI
 InternalQemuFwCfgIsAvailable (
   VOID
   )
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
index 88c32ce89a72..56c59ca3f01d 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
@@ -19,6 +19,7 @@
 #include <Library/DebugLib.h>
 #include <Library/QemuFwCfgLib.h>
 
+#include "QemuFwCfgLibInternal.h"
 
 /**
   Returns a boolean indicating if the firmware configuration interface
@@ -67,7 +68,6 @@ QemuFwCfgIsAvailable (
   @retval    FALSE  The interface is not available internally.
 **/
 BOOLEAN
-EFIAPI
 InternalQemuFwCfgIsAvailable (
   VOID
   )
-- 
2.9.2

[PATCH 3/5] OvmfPkg/IndustryStandard: add QemuFwCfgDma.h

[Laszlo Ersek]

Add the type and macro definitions related to QEMU's DMA-like fw_cfg
access method in a dedicated header file.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h | 50 ++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
new file mode 100644
index 000000000000..37a5804adb05
--- /dev/null
+++ b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
@@ -0,0 +1,50 @@
+/** @file
+  Macro and type definitions related to QEMU's DMA-like fw_cfg access method,
+  based on "docs/specs/fw_cfg.txt" in the QEMU tree.
+
+  Copyright (C) 2016, 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 __FW_CFG_DMA__
+#define __FW_CFG_DMA__
+
+#include <Base.h>
+
+//
+// If the following bit is set in the UINT32 fw_cfg revision / feature bitmap
+// -- read from key 0x0001 with the basic IO Port or MMIO method --, then the
+// DMA interface is available.
+//
+#define FW_CFG_F_DMA BIT1
+
+//
+// Communication structure for the DMA access method. All fields are encoded in
+// big endian.
+//
+#pragma pack (1)
+typedef struct {
+  UINT32 Control;
+  UINT32 Length;
+  UINT64 Address;
+} FW_CFG_DMA_ACCESS;
+#pragma pack ()
+
+//
+// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
+//
+#define FW_CFG_DMA_CTL_ERROR  BIT0
+#define FW_CFG_DMA_CTL_READ   BIT1
+#define FW_CFG_DMA_CTL_SKIP   BIT2
+#define FW_CFG_DMA_CTL_SELECT BIT3
+#define FW_CFG_DMA_CTL_WRITE  BIT4
+
+#endif
-- 
2.9.2

Re: [PATCH 3/5] OvmfPkg/IndustryStandard: add QemuFwCfgDma.h

[Jordan Justen]

On 2016-12-01 09:56:31, Laszlo Ersek wrote:
> Add the type and macro definitions related to QEMU's DMA-like fw_cfg
> access method in a dedicated header file.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h | 50 ++++++++++++++++++++

What do you think about just
OvmfPkg/Include/IndustryStandard/QemuFwCfg.h?

Arguably, the FIRMWARE_CONFIG_ITEM enums could be moved there as
well...

Then again, I think we could also just put this content into
OvmfPkg/Include/Library/QemuFwCfgLib.h.

-Jordan

>  1 file changed, 50 insertions(+)
> 
> diff --git a/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
> new file mode 100644
> index 000000000000..37a5804adb05
> --- /dev/null
> +++ b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
> @@ -0,0 +1,50 @@
> +/** @file
> +  Macro and type definitions related to QEMU's DMA-like fw_cfg access method,
> +  based on "docs/specs/fw_cfg.txt" in the QEMU tree.
> +
> +  Copyright (C) 2016, 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 __FW_CFG_DMA__
> +#define __FW_CFG_DMA__
> +
> +#include <Base.h>
> +
> +//
> +// If the following bit is set in the UINT32 fw_cfg revision / feature bitmap
> +// -- read from key 0x0001 with the basic IO Port or MMIO method --, then the
> +// DMA interface is available.
> +//
> +#define FW_CFG_F_DMA BIT1
> +
> +//
> +// Communication structure for the DMA access method. All fields are encoded in
> +// big endian.
> +//
> +#pragma pack (1)
> +typedef struct {
> +  UINT32 Control;
> +  UINT32 Length;
> +  UINT64 Address;
> +} FW_CFG_DMA_ACCESS;
> +#pragma pack ()
> +
> +//
> +// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
> +//
> +#define FW_CFG_DMA_CTL_ERROR  BIT0
> +#define FW_CFG_DMA_CTL_READ   BIT1
> +#define FW_CFG_DMA_CTL_SKIP   BIT2
> +#define FW_CFG_DMA_CTL_SELECT BIT3
> +#define FW_CFG_DMA_CTL_WRITE  BIT4
> +
> +#endif
> -- 
> 2.9.2
> 
> 

Re: [PATCH 3/5] OvmfPkg/IndustryStandard: add QemuFwCfgDma.h

[Laszlo Ersek]

On 12/01/16 20:34, Jordan Justen wrote:
> On 2016-12-01 09:56:31, Laszlo Ersek wrote:
>> Add the type and macro definitions related to QEMU's DMA-like fw_cfg
>> access method in a dedicated header file.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h | 50 ++++++++++++++++++++
> 
> What do you think about just
> OvmfPkg/Include/IndustryStandard/QemuFwCfg.h?
> 
> Arguably, the FIRMWARE_CONFIG_ITEM enums could be moved there as
> well...
> 
> Then again, I think we could also just put this content into
> OvmfPkg/Include/Library/QemuFwCfgLib.h.

Adding this stuff to "OvmfPkg/Include/Library/QemuFwCfgLib.h" sounds
good to me; I'll do that in v2.

Thanks!
Laszlo

> -Jordan
> 
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
>> new file mode 100644
>> index 000000000000..37a5804adb05
>> --- /dev/null
>> +++ b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
>> @@ -0,0 +1,50 @@
>> +/** @file
>> +  Macro and type definitions related to QEMU's DMA-like fw_cfg access method,
>> +  based on "docs/specs/fw_cfg.txt" in the QEMU tree.
>> +
>> +  Copyright (C) 2016, 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 __FW_CFG_DMA__
>> +#define __FW_CFG_DMA__
>> +
>> +#include <Base.h>
>> +
>> +//
>> +// If the following bit is set in the UINT32 fw_cfg revision / feature bitmap
>> +// -- read from key 0x0001 with the basic IO Port or MMIO method --, then the
>> +// DMA interface is available.
>> +//
>> +#define FW_CFG_F_DMA BIT1
>> +
>> +//
>> +// Communication structure for the DMA access method. All fields are encoded in
>> +// big endian.
>> +//
>> +#pragma pack (1)
>> +typedef struct {
>> +  UINT32 Control;
>> +  UINT32 Length;
>> +  UINT64 Address;
>> +} FW_CFG_DMA_ACCESS;
>> +#pragma pack ()
>> +
>> +//
>> +// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
>> +//
>> +#define FW_CFG_DMA_CTL_ERROR  BIT0
>> +#define FW_CFG_DMA_CTL_READ   BIT1
>> +#define FW_CFG_DMA_CTL_SKIP   BIT2
>> +#define FW_CFG_DMA_CTL_SELECT BIT3
>> +#define FW_CFG_DMA_CTL_WRITE  BIT4
>> +
>> +#endif
>> -- 
>> 2.9.2
>>
>>

Re: [PATCH 3/5] OvmfPkg/IndustryStandard: add QemuFwCfgDma.h

[Jordan Justen]

On 2016-12-01 12:48:51, Laszlo Ersek wrote:
> On 12/01/16 20:34, Jordan Justen wrote:
> > On 2016-12-01 09:56:31, Laszlo Ersek wrote:
> >> Add the type and macro definitions related to QEMU's DMA-like fw_cfg
> >> access method in a dedicated header file.
> >>
> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >>  OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h | 50 ++++++++++++++++++++
> > 
> > What do you think about just
> > OvmfPkg/Include/IndustryStandard/QemuFwCfg.h?
> > 
> > Arguably, the FIRMWARE_CONFIG_ITEM enums could be moved there as
> > well...
> > 
> > Then again, I think we could also just put this content into
> > OvmfPkg/Include/Library/QemuFwCfgLib.h.
> 
> Adding this stuff to "OvmfPkg/Include/Library/QemuFwCfgLib.h" sounds
> good to me; I'll do that in v2.
> 

Ok. By the way, I looked over the series, and with that change you can
add Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

I'll let you decide if you want to send out a v2.

-Jordan

> 
> > -Jordan
> > 
> >>  1 file changed, 50 insertions(+)
> >>
> >> diff --git a/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
> >> new file mode 100644
> >> index 000000000000..37a5804adb05
> >> --- /dev/null
> >> +++ b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
> >> @@ -0,0 +1,50 @@
> >> +/** @file
> >> +  Macro and type definitions related to QEMU's DMA-like fw_cfg access method,
> >> +  based on "docs/specs/fw_cfg.txt" in the QEMU tree.
> >> +
> >> +  Copyright (C) 2016, 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 __FW_CFG_DMA__
> >> +#define __FW_CFG_DMA__
> >> +
> >> +#include <Base.h>
> >> +
> >> +//
> >> +// If the following bit is set in the UINT32 fw_cfg revision / feature bitmap
> >> +// -- read from key 0x0001 with the basic IO Port or MMIO method --, then the
> >> +// DMA interface is available.
> >> +//
> >> +#define FW_CFG_F_DMA BIT1
> >> +
> >> +//
> >> +// Communication structure for the DMA access method. All fields are encoded in
> >> +// big endian.
> >> +//
> >> +#pragma pack (1)
> >> +typedef struct {
> >> +  UINT32 Control;
> >> +  UINT32 Length;
> >> +  UINT64 Address;
> >> +} FW_CFG_DMA_ACCESS;
> >> +#pragma pack ()
> >> +
> >> +//
> >> +// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
> >> +//
> >> +#define FW_CFG_DMA_CTL_ERROR  BIT0
> >> +#define FW_CFG_DMA_CTL_READ   BIT1
> >> +#define FW_CFG_DMA_CTL_SKIP   BIT2
> >> +#define FW_CFG_DMA_CTL_SELECT BIT3
> >> +#define FW_CFG_DMA_CTL_WRITE  BIT4
> >> +
> >> +#endif
> >> -- 
> >> 2.9.2
> >>
> >>
> 

Re: [PATCH 3/5] OvmfPkg/IndustryStandard: add QemuFwCfgDma.h

[Laszlo Ersek]

On 12/02/16 02:01, Jordan Justen wrote:
> On 2016-12-01 12:48:51, Laszlo Ersek wrote:
>> On 12/01/16 20:34, Jordan Justen wrote:
>>> On 2016-12-01 09:56:31, Laszlo Ersek wrote:
>>>> Add the type and macro definitions related to QEMU's DMA-like fw_cfg
>>>> access method in a dedicated header file.
>>>>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>> ---
>>>>  OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h | 50 ++++++++++++++++++++
>>>
>>> What do you think about just
>>> OvmfPkg/Include/IndustryStandard/QemuFwCfg.h?
>>>
>>> Arguably, the FIRMWARE_CONFIG_ITEM enums could be moved there as
>>> well...
>>>
>>> Then again, I think we could also just put this content into
>>> OvmfPkg/Include/Library/QemuFwCfgLib.h.
>>
>> Adding this stuff to "OvmfPkg/Include/Library/QemuFwCfgLib.h" sounds
>> good to me; I'll do that in v2.
>>
> 
> Ok. By the way, I looked over the series, and with that change you can
> add Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

Thank you!

> I'll let you decide if you want to send out a v2.

Yes, I will; first I'd like you to at least skim v2, second, I need
Ard's R-b for the ArmVirtPkg patches.

Hm, Ard seems to be on vacation (good choice :)), so maybe I'll CC Leif
as well. Then I'll use your R-b and hopefully Leif's in Ard's review's
stead.

Thanks!
Laszlo

> -Jordan
> 
>>
>>> -Jordan
>>>
>>>>  1 file changed, 50 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
>>>> new file mode 100644
>>>> index 000000000000..37a5804adb05
>>>> --- /dev/null
>>>> +++ b/OvmfPkg/Include/IndustryStandard/QemuFwCfgDma.h
>>>> @@ -0,0 +1,50 @@
>>>> +/** @file
>>>> +  Macro and type definitions related to QEMU's DMA-like fw_cfg access method,
>>>> +  based on "docs/specs/fw_cfg.txt" in the QEMU tree.
>>>> +
>>>> +  Copyright (C) 2016, 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 __FW_CFG_DMA__
>>>> +#define __FW_CFG_DMA__
>>>> +
>>>> +#include <Base.h>
>>>> +
>>>> +//
>>>> +// If the following bit is set in the UINT32 fw_cfg revision / feature bitmap
>>>> +// -- read from key 0x0001 with the basic IO Port or MMIO method --, then the
>>>> +// DMA interface is available.
>>>> +//
>>>> +#define FW_CFG_F_DMA BIT1
>>>> +
>>>> +//
>>>> +// Communication structure for the DMA access method. All fields are encoded in
>>>> +// big endian.
>>>> +//
>>>> +#pragma pack (1)
>>>> +typedef struct {
>>>> +  UINT32 Control;
>>>> +  UINT32 Length;
>>>> +  UINT64 Address;
>>>> +} FW_CFG_DMA_ACCESS;
>>>> +#pragma pack ()
>>>> +
>>>> +//
>>>> +// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
>>>> +//
>>>> +#define FW_CFG_DMA_CTL_ERROR  BIT0
>>>> +#define FW_CFG_DMA_CTL_READ   BIT1
>>>> +#define FW_CFG_DMA_CTL_SKIP   BIT2
>>>> +#define FW_CFG_DMA_CTL_SELECT BIT3
>>>> +#define FW_CFG_DMA_CTL_WRITE  BIT4
>>>> +
>>>> +#endif
>>>> -- 
>>>> 2.9.2
>>>>
>>>>
>>

[PATCH 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

[Laszlo Ersek]

where "QemuFwCfgDma.h" was added in the previous patch.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-----------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 2fd8d9050566..62a85dff328e 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -25,6 +25,8 @@
 
 #include <Protocol/FdtClient.h>
 
+#include <IndustryStandard/QemuFwCfgDma.h>
+
 STATIC UINTN mFwCfgSelectorAddress;
 STATIC UINTN mFwCfgDataAddress;
 STATIC UINTN mFwCfgDmaAddress;
@@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;
 //
 STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
 
-//
-// Communication structure for DmaReadBytes(). All fields are encoded in big
-// endian.
-//
-#pragma pack (1)
-typedef struct {
-  UINT32 Control;
-  UINT32 Length;
-  UINT64 Address;
-} FW_CFG_DMA_ACCESS;
-#pragma pack ()
-
-//
-// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
-//
-#define FW_CFG_DMA_CTL_ERROR  BIT0
-#define FW_CFG_DMA_CTL_READ   BIT1
-#define FW_CFG_DMA_CTL_SKIP   BIT2
-#define FW_CFG_DMA_CTL_SELECT BIT3
-
 
 /**
   Returns a boolean indicating if the firmware configuration interface
@@ -183,7 +165,7 @@ QemuFwCfgInitialize (
 
         QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
         Features = QemuFwCfgRead32 ();
-        if ((Features & BIT1) != 0) {
+        if ((Features & FW_CFG_F_DMA) != 0) {
           mFwCfgDmaAddress = FwCfgDmaAddress;
           InternalQemuFwCfgReadBytes = DmaReadBytes;
         }
-- 
2.9.2

Re: [PATCH 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

[Leif Lindholm]

On Thu, Dec 01, 2016 at 06:56:32PM +0100, Laszlo Ersek wrote:
> where "QemuFwCfgDma.h" was added in the previous patch.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

This is nice cleanup.
One bit of bikeshedding below, address or ignore - regardless:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-----------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index 2fd8d9050566..62a85dff328e 100644
> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> @@ -25,6 +25,8 @@
>  
>  #include <Protocol/FdtClient.h>
>  
> +#include <IndustryStandard/QemuFwCfgDma.h>
> +

So, I forget if we have official guidelines on this, but instinctively
I would put IndustryStandard before Library (alphabetically).

Regards,

Leif

>  STATIC UINTN mFwCfgSelectorAddress;
>  STATIC UINTN mFwCfgDataAddress;
>  STATIC UINTN mFwCfgDmaAddress;
> @@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;
>  //
>  STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
>  
> -//
> -// Communication structure for DmaReadBytes(). All fields are encoded in big
> -// endian.
> -//
> -#pragma pack (1)
> -typedef struct {
> -  UINT32 Control;
> -  UINT32 Length;
> -  UINT64 Address;
> -} FW_CFG_DMA_ACCESS;
> -#pragma pack ()
> -
> -//
> -// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
> -//
> -#define FW_CFG_DMA_CTL_ERROR  BIT0
> -#define FW_CFG_DMA_CTL_READ   BIT1
> -#define FW_CFG_DMA_CTL_SKIP   BIT2
> -#define FW_CFG_DMA_CTL_SELECT BIT3
> -
>  
>  /**
>    Returns a boolean indicating if the firmware configuration interface
> @@ -183,7 +165,7 @@ QemuFwCfgInitialize (
>  
>          QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
>          Features = QemuFwCfgRead32 ();
> -        if ((Features & BIT1) != 0) {
> +        if ((Features & FW_CFG_F_DMA) != 0) {
>            mFwCfgDmaAddress = FwCfgDmaAddress;
>            InternalQemuFwCfgReadBytes = DmaReadBytes;
>          }
> -- 
> 2.9.2
> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 4/5] ArmVirtPkg/QemuFwCfgLib: rebase lib instance to OvmfPkg/IndustryStandard

[Laszlo Ersek]

On 12/02/16 12:03, Leif Lindholm wrote:
> On Thu, Dec 01, 2016 at 06:56:32PM +0100, Laszlo Ersek wrote:
>> where "QemuFwCfgDma.h" was added in the previous patch.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> This is nice cleanup.
> One bit of bikeshedding below, address or ignore - regardless:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
>> ---
>>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 24 +++-----------------
>>  1 file changed, 3 insertions(+), 21 deletions(-)
>>
>> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> index 2fd8d9050566..62a85dff328e 100644
>> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>> @@ -25,6 +25,8 @@
>>  
>>  #include <Protocol/FdtClient.h>
>>  
>> +#include <IndustryStandard/QemuFwCfgDma.h>
>> +
> 
> So, I forget if we have official guidelines on this, but instinctively
> I would put IndustryStandard before Library (alphabetically).

Good point. But, in the next version, this #include directive will go
away anyway, because this file already includes
<Library/QemuFwCfgLib.h>, and that header file will get the new macros
in v2 (based on Jordan's feedback).

Thanks!
Laszlo

> 
> Regards,
> 
> Leif
> 
>>  STATIC UINTN mFwCfgSelectorAddress;
>>  STATIC UINTN mFwCfgDataAddress;
>>  STATIC UINTN mFwCfgDmaAddress;
>> @@ -53,26 +55,6 @@ STATIC READ_BYTES_FUNCTION DmaReadBytes;
>>  //
>>  STATIC READ_BYTES_FUNCTION *InternalQemuFwCfgReadBytes = MmioReadBytes;
>>  
>> -//
>> -// Communication structure for DmaReadBytes(). All fields are encoded in big
>> -// endian.
>> -//
>> -#pragma pack (1)
>> -typedef struct {
>> -  UINT32 Control;
>> -  UINT32 Length;
>> -  UINT64 Address;
>> -} FW_CFG_DMA_ACCESS;
>> -#pragma pack ()
>> -
>> -//
>> -// Macros for the FW_CFG_DMA_ACCESS.Control bitmap (in native encoding).
>> -//
>> -#define FW_CFG_DMA_CTL_ERROR  BIT0
>> -#define FW_CFG_DMA_CTL_READ   BIT1
>> -#define FW_CFG_DMA_CTL_SKIP   BIT2
>> -#define FW_CFG_DMA_CTL_SELECT BIT3
>> -
>>  
>>  /**
>>    Returns a boolean indicating if the firmware configuration interface
>> @@ -183,7 +165,7 @@ QemuFwCfgInitialize (
>>  
>>          QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
>>          Features = QemuFwCfgRead32 ();
>> -        if ((Features & BIT1) != 0) {
>> +        if ((Features & FW_CFG_F_DMA) != 0) {
>>            mFwCfgDmaAddress = FwCfgDmaAddress;
>>            InternalQemuFwCfgReadBytes = DmaReadBytes;
>>          }
>> -- 
>> 2.9.2
>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

[PATCH 5/5] OvmfPkg/QemuFwCfgLib: support QEMU's DMA-like fw_cfg access method

[Laszlo Ersek]

The benefits of the DMA-like access method are (a) speed, (b) write
support in QEMU 2.9+.

(IOPort-based write support was discontinued in QEMU 2.4, and the
DMA-based one is being added to QEMU 2.9. Write support needs no separate
feature detection because writeability is governed on the level of
individual fw_cfg files -- if a file meant to be written by the firmware
exists in the directory, then it is writeable with the DMA method.)

We don't enable this feature for the SEC library instance, because:
- the SEC instance remains without clients (I've checked that it builds
  though),
- in SEC, any possible fw_cfg use is expected to be small and read-only.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 13 ++++
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c         | 73 ++++++++++++++++++++
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c      | 26 ++++++-
 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c         | 15 ++++
 4 files changed, 126 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
index 5b162bf98739..6e87c625102e 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h
@@ -30,4 +30,17 @@ InternalQemuFwCfgIsAvailable (
   VOID
   );
 
+
+/**
+  Returns a boolean indicating whether QEMU provides the DMA-like access method
+  for fw_cfg.
+
+  @retval    TRUE   The DMA-like access method is available.
+  @retval    FALSE  The DMA-like access method is unavailable.
+**/
+BOOLEAN
+InternalQemuFwCfgDmaIsAvailable (
+  VOID
+  );
+
 #endif
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 804d5b0e42be..f609f6422878 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -21,6 +21,7 @@
 #include <Library/QemuFwCfgLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <IndustryStandard/QemuFwCfgDma.h>
 
 #include "QemuFwCfgLibInternal.h"
 
@@ -99,6 +100,70 @@ QemuFwCfgSelectItem (
 
 
 /**
+  Transfer an array of bytes using the DMA interface.
+
+  @param[in]     Size    Size in bytes to transfer.
+  @param[in,out] Buffer  Buffer to read data into or write data from. May be
+                         NULL if Size is zero.
+  @param[in]     Write   TRUE if writing to fw_cfg from Buffer, FALSE if
+                         reading from fw_cfg into Buffer.
+**/
+VOID
+InternalQemuFwCfgDmaBytes (
+  IN     UINT32   Size,
+  IN OUT VOID     *Buffer OPTIONAL,
+  IN     BOOLEAN  Write
+  )
+{
+  volatile FW_CFG_DMA_ACCESS Access;
+  UINT32                     AccessHigh, AccessLow;
+  UINT32                     Status;
+
+  if (Size == 0) {
+    return;
+  }
+
+  Access.Control = SwapBytes32 (
+                    Write ? FW_CFG_DMA_CTL_WRITE : FW_CFG_DMA_CTL_READ
+                    );
+  Access.Length  = SwapBytes32 (Size);
+  Access.Address = SwapBytes64 ((UINTN)Buffer);
+
+  //
+  // Delimit the transfer from (a) modifications to Access, (b) in case of a
+  // write, from writes to Buffer by the caller.
+  //
+  MemoryFence ();
+
+  //
+  // Start the transfer.
+  //
+  AccessHigh = (UINT32)RShiftU64 ((UINTN)&Access, 32);
+  AccessLow  = (UINT32)(UINTN)&Access;
+  IoWrite32 (0x514, SwapBytes32 (AccessHigh));
+  IoWrite32 (0x518, SwapBytes32 (AccessLow));
+
+  //
+  // Don't look at Access.Control before starting the transfer.
+  //
+  MemoryFence ();
+
+  //
+  // Wait for the transfer to complete.
+  //
+  do {
+    Status = SwapBytes32 (Access.Control);
+    ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0);
+  } while (Status != 0);
+
+  //
+  // After a read, the caller will want to use Buffer.
+  //
+  MemoryFence ();
+}
+
+
+/**
   Reads firmware configuration bytes into a buffer
 
   @param[in] Size - Size in bytes to read
@@ -112,6 +177,10 @@ InternalQemuFwCfgReadBytes (
   IN VOID                   *Buffer  OPTIONAL
   )
 {
+  if (InternalQemuFwCfgDmaIsAvailable () && Size <= MAX_UINT32) {
+    InternalQemuFwCfgDmaBytes ((UINT32)Size, Buffer, FALSE);
+    return;
+  }
   IoReadFifo8 (0x511, Size, Buffer);
 }
 
@@ -160,6 +229,10 @@ QemuFwCfgWriteBytes (
   )
 {
   if (InternalQemuFwCfgIsAvailable ()) {
+    if (InternalQemuFwCfgDmaIsAvailable () && Size <= MAX_UINT32) {
+      InternalQemuFwCfgDmaBytes ((UINT32)Size, Buffer, TRUE);
+      return;
+    }
     IoWriteFifo8 (0x511, Size, Buffer);
   }
 }
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
index 88d88c0edf69..fde4a11b4e0d 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPeiDxe.c
@@ -14,12 +14,14 @@
   WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 **/
 
+#include <IndustryStandard/QemuFwCfgDma.h>
 #include <Library/DebugLib.h>
 #include <Library/QemuFwCfgLib.h>
 
 #include "QemuFwCfgLibInternal.h"
 
 STATIC BOOLEAN mQemuFwCfgSupported = FALSE;
+STATIC BOOLEAN mQemuFwCfgDmaSupported;
 
 
 /**
@@ -53,8 +55,10 @@ QemuFwCfgInitialize (
 
   //
   // Enable the access routines while probing to see if it is supported.
+  // For probing we always use the IO Port (IoReadFifo8()) access method.
   //
   mQemuFwCfgSupported = TRUE;
+  mQemuFwCfgDmaSupported = FALSE;
 
   QemuFwCfgSelectItem (QemuFwCfgItemSignature);
   Signature = QemuFwCfgRead32 ();
@@ -70,7 +74,12 @@ QemuFwCfgInitialize (
     return RETURN_SUCCESS;
   }
 
-  DEBUG ((EFI_D_INFO, "QemuFwCfg interface is supported.\n"));
+  if ((Revision & FW_CFG_F_DMA) == 0) {
+    DEBUG ((DEBUG_INFO, "QemuFwCfg interface (IO Port) is supported.\n"));
+  } else {
+    mQemuFwCfgDmaSupported = TRUE;
+    DEBUG ((DEBUG_INFO, "QemuFwCfg interface (DMA) is supported.\n"));
+  }
   return RETURN_SUCCESS;
 }
 
@@ -91,3 +100,18 @@ InternalQemuFwCfgIsAvailable (
 {
   return mQemuFwCfgSupported;
 }
+
+/**
+  Returns a boolean indicating whether QEMU provides the DMA-like access method
+  for fw_cfg.
+
+  @retval    TRUE   The DMA-like access method is available.
+  @retval    FALSE  The DMA-like access method is unavailable.
+**/
+BOOLEAN
+InternalQemuFwCfgDmaIsAvailable (
+  VOID
+  )
+{
+  return mQemuFwCfgDmaSupported;
+}
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
index 56c59ca3f01d..465ccbe90dad 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c
@@ -79,3 +79,18 @@ InternalQemuFwCfgIsAvailable (
   //
   return TRUE;
 }
+
+/**
+  Returns a boolean indicating whether QEMU provides the DMA-like access method
+  for fw_cfg.
+
+  @retval    TRUE   The DMA-like access method is available.
+  @retval    FALSE  The DMA-like access method is unavailable.
+**/
+BOOLEAN
+InternalQemuFwCfgDmaIsAvailable (
+  VOID
+  )
+{
+  return FALSE;
+}
-- 
2.9.2