* [PATCH 01/12] OvmfPkg: introduce QemuFwCfgS3Lib class
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 1:48 ` [PATCH 02/12] OvmfPkg/QemuFwCfgS3Lib: add initial Base Null library instance Laszlo Ersek
` (12 subsequent siblings)
13 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
This library class will enable driver modules (a) to query whether S3
support was enabled on the QEMU command line, (b) to produce fw_cfg DMA
operations that are to be replayed at S3 resume time.
Declare the library class in OvmfPkg/OvmfPkg.dec, and add the library
class header under OvmfPkg/Include/Library/. At the moment, the only API
we expose is QemuFwCfgS3Enabled(), which we'll first migrate from
QemuFwCfgLib. Further interfaces will be added in later patches.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/OvmfPkg.dec | 4 ++
OvmfPkg/Include/Library/QemuFwCfgS3Lib.h | 39 ++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index a0c76a5bb448..3490f7ca7c07 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -35,6 +35,10 @@ [LibraryClasses]
#
QemuFwCfgLib|Include/Library/QemuFwCfgLib.h
+ ## @libraryclass S3 support for QEMU fw_cfg
+ #
+ QemuFwCfgS3Lib|Include/Library/QemuFwCfgS3Lib.h
+
## @libraryclass Rewrite the BootOrder NvVar based on QEMU's "bootorder"
# fw_cfg file.
#
diff --git a/OvmfPkg/Include/Library/QemuFwCfgS3Lib.h b/OvmfPkg/Include/Library/QemuFwCfgS3Lib.h
new file mode 100644
index 000000000000..1c473610d11c
--- /dev/null
+++ b/OvmfPkg/Include/Library/QemuFwCfgS3Lib.h
@@ -0,0 +1,39 @@
+/** @file
+ S3 support for QEMU fw_cfg
+
+ This library class enables driver modules (a) to query whether S3 support was
+ enabled on the QEMU command line, (b) to produce fw_cfg DMA operations that
+ are to be replayed at S3 resume time.
+
+ Copyright (C) 2017, 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_S3_LIB__
+#define __FW_CFG_S3_LIB__
+
+/**
+ Determine if S3 support is explicitly enabled.
+
+ @retval TRUE If S3 support is explicitly enabled. Other functions in this
+ library may be called (subject to their individual
+ restrictions).
+
+ FALSE Otherwise. This includes unavailability of the firmware
+ configuration interface. No other function in this library
+ must be called.
+**/
+BOOLEAN
+EFIAPI
+QemuFwCfgS3Enabled (
+ VOID
+ );
+
+#endif
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 02/12] OvmfPkg/QemuFwCfgS3Lib: add initial Base Null library instance
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
2017-02-23 1:48 ` [PATCH 01/12] OvmfPkg: introduce QemuFwCfgS3Lib class Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 1:48 ` [PATCH 03/12] OvmfPkg/QemuFwCfgS3Lib: add initial PEI and DXE fw_cfg library instances Laszlo Ersek
` (11 subsequent siblings)
13 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
This library instance returns constant FALSE from QemuFwCfgS3Enabled(),
and all other library functions trigger assertion failures. It is suitable
for QEMU targets and machine types that never enable S3.
The QemuFwCfgS3Enabled() implementation is copied from
"ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c". Stubs for further
QemuFwCfgS3Lib APIs (with assertion failures, see above) will be added
later.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf | 39 ++++++++++++++++++++
OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c | 39 ++++++++++++++++++++
2 files changed, 78 insertions(+)
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf b/OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
new file mode 100644
index 000000000000..ba24a0b9d434
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
@@ -0,0 +1,39 @@
+## @file
+# Base Null library instance of the QemuFwCfgS3Lib class.
+#
+# This library instance returns constant FALSE from QemuFwCfgS3Enabled(), and
+# all other library functions trigger assertion failures. It is suitable for
+# QEMU targets and machine types that never enable S3.
+#
+# Copyright (C) 2017, 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.
+##
+
+[Defines]
+ INF_VERSION = 1.25
+ BASE_NAME = BaseQemuFwCfgS3LibNull
+ FILE_GUID = EA7D2B69-D221-4950-9C2C-C38A65BCC96E
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = QemuFwCfgS3Lib
+
+#
+# The following information is for reference only and not required by the build
+# tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64 IPF EBC
+#
+
+[Sources]
+ QemuFwCfgS3Base.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c
new file mode 100644
index 000000000000..bed9bf3dfb57
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c
@@ -0,0 +1,39 @@
+/** @file
+ Base Null library instance of the QemuFwCfgS3Lib class.
+
+ This library instance returns constant FALSE from QemuFwCfgS3Enabled(), and
+ all other library functions trigger assertion failures. It is suitable for
+ QEMU targets and machine types that never enable S3.
+
+ Copyright (C) 2017, 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.
+**/
+
+#include <Library/QemuFwCfgS3Lib.h>
+
+/**
+ Determine if S3 support is explicitly enabled.
+
+ @retval TRUE If S3 support is explicitly enabled. Other functions in this
+ library may be called (subject to their individual
+ restrictions).
+
+ FALSE Otherwise. This includes unavailability of the firmware
+ configuration interface. No other function in this library
+ must be called.
+**/
+BOOLEAN
+EFIAPI
+QemuFwCfgS3Enabled (
+ VOID
+ )
+{
+ return FALSE;
+}
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 03/12] OvmfPkg/QemuFwCfgS3Lib: add initial PEI and DXE fw_cfg library instances
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
2017-02-23 1:48 ` [PATCH 01/12] OvmfPkg: introduce QemuFwCfgS3Lib class Laszlo Ersek
2017-02-23 1:48 ` [PATCH 02/12] OvmfPkg/QemuFwCfgS3Lib: add initial Base Null library instance Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 1:48 ` [PATCH 04/12] ArmVirtPkg: resolve QemuFwCfgS3Lib Laszlo Ersek
` (10 subsequent siblings)
13 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
This patch introduces PeiQemuFwCfgS3LibFwCfg, a limited functionality
QemuFwCfgS3Lib instance, for PEI phase modules.
The patch also introduces DxeQemuFwCfgS3LibFwCfg, a full functionality
QemuFwCfgS3Lib instance, for DXE_DRIVER and DXE_RUNTIME_DRIVER modules.
These library instances share the QemuFwCfgS3Enabled() function. The
function actually uses fw_cfg; the implementation is copied from
"OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c".
The library instances will diverge in the following patches.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf | 38 ++++++++++++++++
OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf | 41 +++++++++++++++++
OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c | 48 ++++++++++++++++++++
3 files changed, 127 insertions(+)
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf b/OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
new file mode 100644
index 000000000000..7016575f3dab
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
@@ -0,0 +1,38 @@
+## @file
+# Full functionality QemuFwCfgS3Lib instance, for DXE phase modules.
+#
+# Copyright (C) 2017, 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.
+##
+
+[Defines]
+ INF_VERSION = 1.25
+ BASE_NAME = DxeQemuFwCfgS3LibFwCfg
+ FILE_GUID = C5DE76EB-E8DE-4057-A487-C5A09AB039AB
+ MODULE_TYPE = DXE_DRIVER
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = QemuFwCfgS3Lib|DXE_DRIVER DXE_RUNTIME_DRIVER
+
+#
+# The following information is for reference only and not required by the build
+# tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64 IPF EBC
+#
+
+[Sources]
+ QemuFwCfgS3PeiDxe.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ QemuFwCfgLib
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf b/OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
new file mode 100644
index 000000000000..2593af8e5b4c
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
@@ -0,0 +1,41 @@
+## @file
+# Limited functionality QemuFwCfgS3Lib instance, for PEI phase modules.
+#
+# QemuFwCfgS3Enabled() queries S3 enablement via fw_cfg. Other library APIs
+# will report lack of support.
+#
+# Copyright (C) 2017, 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.
+##
+
+[Defines]
+ INF_VERSION = 1.25
+ BASE_NAME = PeiQemuFwCfgS3LibFwCfg
+ FILE_GUID = DD8D28B4-C1DC-4CAF-BB93-074BE80DAE6D
+ MODULE_TYPE = PEIM
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = QemuFwCfgS3Lib|PEIM
+
+#
+# The following information is for reference only and not required by the build
+# tools.
+#
+# VALID_ARCHITECTURES = IA32 X64 ARM AARCH64 IPF EBC
+#
+
+[Sources]
+ QemuFwCfgS3PeiDxe.c
+
+[Packages]
+ MdePkg/MdePkg.dec
+ OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ QemuFwCfgLib
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c
new file mode 100644
index 000000000000..f87d6b8227cf
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c
@@ -0,0 +1,48 @@
+/** @file
+ Shared code for the PEI fw_cfg and DXE fw_cfg instances of the QemuFwCfgS3Lib
+ class.
+
+ Copyright (C) 2017, 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.
+**/
+
+#include <Library/QemuFwCfgLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
+
+/**
+ Determine if S3 support is explicitly enabled.
+
+ @retval TRUE If S3 support is explicitly enabled. Other functions in this
+ library may be called (subject to their individual
+ restrictions).
+
+ FALSE Otherwise. This includes unavailability of the firmware
+ configuration interface. No other function in this library
+ must be called.
+**/
+BOOLEAN
+EFIAPI
+QemuFwCfgS3Enabled (
+ VOID
+ )
+{
+ RETURN_STATUS Status;
+ FIRMWARE_CONFIG_ITEM FwCfgItem;
+ UINTN FwCfgSize;
+ UINT8 SystemStates[6];
+
+ Status = QemuFwCfgFindFile ("etc/system-states", &FwCfgItem, &FwCfgSize);
+ if (Status != RETURN_SUCCESS || FwCfgSize != sizeof SystemStates) {
+ return FALSE;
+ }
+ QemuFwCfgSelectItem (FwCfgItem);
+ QemuFwCfgReadBytes (sizeof SystemStates, SystemStates);
+ return (BOOLEAN) (SystemStates[3] & BIT7);
+}
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 04/12] ArmVirtPkg: resolve QemuFwCfgS3Lib
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (2 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 03/12] OvmfPkg/QemuFwCfgS3Lib: add initial PEI and DXE fw_cfg library instances Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 11:18 ` Ard Biesheuvel
2017-02-23 1:48 ` [PATCH 05/12] OvmfPkg: " Laszlo Ersek
` (9 subsequent siblings)
13 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel
QemuFwCfgS3Enabled() in "ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c"
returns constant FALSE.
The same implementation is now available factored-out in
"OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c".
Resolve QemuFwCfgS3Lib to BaseQemuFwCfgS3LibNull.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/ArmVirtQemu.dsc | 1 +
ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
2 files changed, 2 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 8fe3c3816961..9c8a2d977a8a 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -52,6 +52,7 @@ [LibraryClasses.common]
VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
QemuFwCfgLib|ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
ArmPlatformLib|ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
ArmPlatformSysConfigLib|ArmPlatformPkg/Library/ArmPlatformSysConfigLibNull/ArmPlatformSysConfigLibNull.inf
diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index aa40374745af..6afc10e69ef5 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -51,6 +51,7 @@ [LibraryClasses.common]
VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
QemuFwCfgLib|ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
ArmPlatformLib|ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/ArmQemuRelocatablePlatformLib.inf
ArmPlatformSysConfigLib|ArmPlatformPkg/Library/ArmPlatformSysConfigLibNull/ArmPlatformSysConfigLibNull.inf
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 04/12] ArmVirtPkg: resolve QemuFwCfgS3Lib
2017-02-23 1:48 ` [PATCH 04/12] ArmVirtPkg: resolve QemuFwCfgS3Lib Laszlo Ersek
@ 2017-02-23 11:18 ` Ard Biesheuvel
0 siblings, 0 replies; 32+ messages in thread
From: Ard Biesheuvel @ 2017-02-23 11:18 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01
On 23 February 2017 at 01:48, Laszlo Ersek <lersek@redhat.com> wrote:
> QemuFwCfgS3Enabled() in "ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c"
> returns constant FALSE.
>
> The same implementation is now available factored-out in
> "OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c".
>
> Resolve QemuFwCfgS3Lib to BaseQemuFwCfgS3LibNull.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
> index 8fe3c3816961..9c8a2d977a8a 100644
> --- a/ArmVirtPkg/ArmVirtQemu.dsc
> +++ b/ArmVirtPkg/ArmVirtQemu.dsc
> @@ -52,6 +52,7 @@ [LibraryClasses.common]
> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> QemuFwCfgLib|ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> + QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
>
> ArmPlatformLib|ArmVirtPkg/Library/ArmVirtPlatformLib/ArmVirtPlatformLib.inf
> ArmPlatformSysConfigLib|ArmPlatformPkg/Library/ArmPlatformSysConfigLibNull/ArmPlatformSysConfigLibNull.inf
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index aa40374745af..6afc10e69ef5 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -51,6 +51,7 @@ [LibraryClasses.common]
> VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf
> VirtioMmioDeviceLib|OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceLib.inf
> QemuFwCfgLib|ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf
> + QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
>
> ArmPlatformLib|ArmVirtPkg/Library/ArmQemuRelocatablePlatformLib/ArmQemuRelocatablePlatformLib.inf
> ArmPlatformSysConfigLib|ArmPlatformPkg/Library/ArmPlatformSysConfigLibNull/ArmPlatformSysConfigLibNull.inf
> --
> 2.9.3
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 05/12] OvmfPkg: resolve QemuFwCfgS3Lib
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (3 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 04/12] ArmVirtPkg: resolve QemuFwCfgS3Lib Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 1:48 ` [PATCH 06/12] ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib Laszlo Ersek
` (8 subsequent siblings)
13 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
QemuFwCfgS3Enabled() in "OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c"
queries the "etc/system-states" fw_cfg file.
The same implementation is now available factored-out in
"OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c". It is available to
PEIMs through the PeiQemuFwCfgS3LibFwCfg instance, and to DXE_DRIVER and
DXE_RUNTIME_DRIVER modules through the DxeQemuFwCfgS3LibFwCfg instance.
Resolve QemuFwCfgS3Lib accordingly.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 3 +++
OvmfPkg/OvmfPkgIa32X64.dsc | 3 +++
OvmfPkg/OvmfPkgX64.dsc | 3 +++
3 files changed, 9 insertions(+)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 993547d4859e..68ac1ba22cae 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -232,6 +232,7 @@ [LibraryClasses.common.PEIM]
!endif
CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.DXE_CORE]
HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -265,6 +266,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.UEFI_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -310,6 +312,7 @@ [LibraryClasses.common.DXE_DRIVER]
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.UEFI_APPLICATION]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index f36604ecb4d8..ee3ca38ba243 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -237,6 +237,7 @@ [LibraryClasses.common.PEIM]
!endif
CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.DXE_CORE]
HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -270,6 +271,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.UEFI_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -315,6 +317,7 @@ [LibraryClasses.common.DXE_DRIVER]
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.UEFI_APPLICATION]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index c5bf1a672b1e..2ae6b5ad4488 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -237,6 +237,7 @@ [LibraryClasses.common.PEIM]
!endif
CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.DXE_CORE]
HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -270,6 +271,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.UEFI_DRIVER]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -315,6 +317,7 @@ [LibraryClasses.common.DXE_DRIVER]
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+ QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.UEFI_APPLICATION]
PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 06/12] ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (4 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 05/12] OvmfPkg: " Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 11:18 ` Ard Biesheuvel
2017-02-23 1:48 ` [PATCH 07/12] OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to libclass Laszlo Ersek
` (7 subsequent siblings)
13 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Ard Biesheuvel, Jordan Justen
At this point we're ready to retire QemuFwCfgS3Enabled() from the
QemuFwCfgLib class, together with its implementations in:
- ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
- OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
Extend all modules that call the function with a new QemuFwCfgS3Lib class
dependency. Thanks to the previously added library class, instances, and
class resolutions, we can do this switch now as tightly as possible.
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 1 +
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 1 +
OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 1 +
OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 1 +
OvmfPkg/Include/Library/QemuFwCfgLib.h | 14 ----------
OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 1 +
ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 17 ------------
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 1 +
OvmfPkg/Library/LockBoxLib/LockBoxDxe.c | 1 +
OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 28 --------------------
OvmfPkg/PlatformPei/Platform.c | 1 +
OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 1 +
14 files changed, 11 insertions(+), 59 deletions(-)
diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
index bb5f14e0fc7a..42edc97b3da2 100644
--- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
+++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
@@ -52,6 +52,7 @@ [LibraryClasses]
UefiDriverEntryPoint
HobLib
QemuFwCfgLib
+ QemuFwCfgS3Lib
MemoryAllocationLib
BaseLib
DxeServicesTableLib
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
index e550ff5a4714..a9350540215d 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
@@ -44,6 +44,7 @@ [LibraryClasses]
MemoryAllocationLib
OrderedCollectionLib
QemuFwCfgLib
+ QemuFwCfgS3Lib
UefiBootServicesTableLib
UefiDriverEntryPoint
diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
index bedf1811e0b2..eb03f4f546bc 100644
--- a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
+++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
@@ -40,6 +40,7 @@ [LibraryClasses]
DebugLib
UefiBootServicesTableLib
QemuFwCfgLib
+ QemuFwCfgS3Lib
[Protocols]
gEfiLockBoxProtocolGuid ## SOMETIMES_PRODUCES
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
index f9e35c955d4d..27789b7377bc 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
+++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
@@ -52,6 +52,7 @@ [LibraryClasses]
PciLib
NvVarsFileLib
QemuFwCfgLib
+ QemuFwCfgS3Lib
LoadLinuxLib
QemuBootOrderLib
UefiLib
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index fbaed3182dcf..53c6dd445a0e 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -58,6 +58,7 @@ [LibraryClasses]
PeiServicesTablePointerLib
PeimEntryPoint
QemuFwCfgLib
+ QemuFwCfgS3Lib
MtrrLib
PcdLib
diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
index 31c80bd4448c..04b1ed0e4eb3 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
@@ -56,6 +56,7 @@ [LibraryClasses]
PcdLib
PciLib
QemuFwCfgLib
+ QemuFwCfgS3Lib
UefiBootServicesTableLib
UefiDriverEntryPoint
diff --git a/OvmfPkg/Include/Library/QemuFwCfgLib.h b/OvmfPkg/Include/Library/QemuFwCfgLib.h
index 2a1261327b01..596e3f25d5fe 100644
--- a/OvmfPkg/Include/Library/QemuFwCfgLib.h
+++ b/OvmfPkg/Include/Library/QemuFwCfgLib.h
@@ -179,19 +179,5 @@ QemuFwCfgFindFile (
OUT UINTN *Size
);
-
-/**
- Determine if S3 support is explicitly enabled.
-
- @retval TRUE if S3 support is explicitly enabled.
- FALSE otherwise. This includes unavailability of the firmware
- configuration interface.
-**/
-BOOLEAN
-EFIAPI
-QemuFwCfgS3Enabled (
- VOID
- );
-
#endif
diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
index ec58efa5ef4a..97ffbb514825 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
@@ -48,6 +48,7 @@ Abstract:
#include <Library/IoLib.h>
#include <Library/NvVarsFileLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
#include <Library/QemuBootOrderLib.h>
#include <Protocol/Decompress.h>
diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 9dd5c911fc5c..fba1684af2b2 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -637,20 +637,3 @@ QemuFwCfgFindFile (
return RETURN_NOT_FOUND;
}
-
-
-/**
- Determine if S3 support is explicitly enabled.
-
- @retval TRUE if S3 support is explicitly enabled.
- FALSE otherwise. This includes unavailability of the firmware
- configuration interface.
-**/
-BOOLEAN
-EFIAPI
-QemuFwCfgS3Enabled (
- VOID
- )
-{
- return FALSE;
-}
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 6a0ecd1ad962..76512534f5e0 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -19,6 +19,7 @@
#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
#include <Library/DxeServicesTableLib.h>
#include <Library/PcdLib.h>
#include <Library/OrderedCollectionLib.h>
diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c b/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c
index 818646a275a9..3da9cd21e5c9 100644
--- a/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c
+++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c
@@ -19,6 +19,7 @@
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
#include <Protocol/LockBox.h>
#include <LockBoxLib.h>
diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 3dd55ba5042e..1bf725d8b7ae 100644
--- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -368,31 +368,3 @@ QemuFwCfgFindFile (
return RETURN_NOT_FOUND;
}
-
-
-/**
- Determine if S3 support is explicitly enabled.
-
- @retval TRUE if S3 support is explicitly enabled.
- FALSE otherwise. This includes unavailability of the firmware
- configuration interface.
-**/
-BOOLEAN
-EFIAPI
-QemuFwCfgS3Enabled (
- VOID
- )
-{
- RETURN_STATUS Status;
- FIRMWARE_CONFIG_ITEM FwCfgItem;
- UINTN FwCfgSize;
- UINT8 SystemStates[6];
-
- Status = QemuFwCfgFindFile ("etc/system-states", &FwCfgItem, &FwCfgSize);
- if (Status != RETURN_SUCCESS || FwCfgSize != sizeof SystemStates) {
- return FALSE;
- }
- QemuFwCfgSelectItem (FwCfgItem);
- QemuFwCfgReadBytes (sizeof SystemStates, SystemStates);
- return (BOOLEAN) (SystemStates[3] & BIT7);
-}
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 0be86722e548..77a8a16c15b8 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -32,6 +32,7 @@
#include <Library/PeimEntryPoint.h>
#include <Library/PeiServicesLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
#include <Library/ResourcePublicationLib.h>
#include <Guid/MemoryTypeInformation.h>
#include <Ppi/MasterBootMode.h>
diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
index f31646d73461..bb79fce0855b 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
@@ -32,6 +32,7 @@
#include <Library/PcdLib.h>
#include <Library/PciLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Protocol/S3SaveState.h>
#include <Protocol/SmmControl2.h>
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 06/12] ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib
2017-02-23 1:48 ` [PATCH 06/12] ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib Laszlo Ersek
@ 2017-02-23 11:18 ` Ard Biesheuvel
0 siblings, 0 replies; 32+ messages in thread
From: Ard Biesheuvel @ 2017-02-23 11:18 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen
On 23 February 2017 at 01:48, Laszlo Ersek <lersek@redhat.com> wrote:
> At this point we're ready to retire QemuFwCfgS3Enabled() from the
> QemuFwCfgLib class, together with its implementations in:
>
> - ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> - OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
>
> Extend all modules that call the function with a new QemuFwCfgS3Lib class
> dependency. Thanks to the previously added library class, instances, and
> class resolutions, we can do this switch now as tightly as possible.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 1 +
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 1 +
> OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 1 +
> OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 1 +
> OvmfPkg/Include/Library/QemuFwCfgLib.h | 14 ----------
> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 1 +
> ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 17 ------------
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 1 +
> OvmfPkg/Library/LockBoxLib/LockBoxDxe.c | 1 +
> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 28 --------------------
> OvmfPkg/PlatformPei/Platform.c | 1 +
> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 1 +
> 14 files changed, 11 insertions(+), 59 deletions(-)
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> index bb5f14e0fc7a..42edc97b3da2 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
> @@ -52,6 +52,7 @@ [LibraryClasses]
> UefiDriverEntryPoint
> HobLib
> QemuFwCfgLib
> + QemuFwCfgS3Lib
> MemoryAllocationLib
> BaseLib
> DxeServicesTableLib
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
> index e550ff5a4714..a9350540215d 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
> @@ -44,6 +44,7 @@ [LibraryClasses]
> MemoryAllocationLib
> OrderedCollectionLib
> QemuFwCfgLib
> + QemuFwCfgS3Lib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
>
> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
> index bedf1811e0b2..eb03f4f546bc 100644
> --- a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
> @@ -40,6 +40,7 @@ [LibraryClasses]
> DebugLib
> UefiBootServicesTableLib
> QemuFwCfgLib
> + QemuFwCfgS3Lib
>
> [Protocols]
> gEfiLockBoxProtocolGuid ## SOMETIMES_PRODUCES
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> index f9e35c955d4d..27789b7377bc 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> @@ -52,6 +52,7 @@ [LibraryClasses]
> PciLib
> NvVarsFileLib
> QemuFwCfgLib
> + QemuFwCfgS3Lib
> LoadLinuxLib
> QemuBootOrderLib
> UefiLib
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index fbaed3182dcf..53c6dd445a0e 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -58,6 +58,7 @@ [LibraryClasses]
> PeiServicesTablePointerLib
> PeimEntryPoint
> QemuFwCfgLib
> + QemuFwCfgS3Lib
> MtrrLib
> PcdLib
>
> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> index 31c80bd4448c..04b1ed0e4eb3 100644
> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf
> @@ -56,6 +56,7 @@ [LibraryClasses]
> PcdLib
> PciLib
> QemuFwCfgLib
> + QemuFwCfgS3Lib
> UefiBootServicesTableLib
> UefiDriverEntryPoint
>
> diff --git a/OvmfPkg/Include/Library/QemuFwCfgLib.h b/OvmfPkg/Include/Library/QemuFwCfgLib.h
> index 2a1261327b01..596e3f25d5fe 100644
> --- a/OvmfPkg/Include/Library/QemuFwCfgLib.h
> +++ b/OvmfPkg/Include/Library/QemuFwCfgLib.h
> @@ -179,19 +179,5 @@ QemuFwCfgFindFile (
> OUT UINTN *Size
> );
>
> -
> -/**
> - Determine if S3 support is explicitly enabled.
> -
> - @retval TRUE if S3 support is explicitly enabled.
> - FALSE otherwise. This includes unavailability of the firmware
> - configuration interface.
> -**/
> -BOOLEAN
> -EFIAPI
> -QemuFwCfgS3Enabled (
> - VOID
> - );
> -
> #endif
>
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> index ec58efa5ef4a..97ffbb514825 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h
> @@ -48,6 +48,7 @@ Abstract:
> #include <Library/IoLib.h>
> #include <Library/NvVarsFileLib.h>
> #include <Library/QemuFwCfgLib.h>
> +#include <Library/QemuFwCfgS3Lib.h>
> #include <Library/QemuBootOrderLib.h>
>
> #include <Protocol/Decompress.h>
> diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index 9dd5c911fc5c..fba1684af2b2 100644
> --- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> @@ -637,20 +637,3 @@ QemuFwCfgFindFile (
>
> return RETURN_NOT_FOUND;
> }
> -
> -
> -/**
> - Determine if S3 support is explicitly enabled.
> -
> - @retval TRUE if S3 support is explicitly enabled.
> - FALSE otherwise. This includes unavailability of the firmware
> - configuration interface.
> -**/
> -BOOLEAN
> -EFIAPI
> -QemuFwCfgS3Enabled (
> - VOID
> - )
> -{
> - return FALSE;
> -}
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index 6a0ecd1ad962..76512534f5e0 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -19,6 +19,7 @@
> #include <Library/BaseMemoryLib.h>
> #include <Library/MemoryAllocationLib.h>
> #include <Library/QemuFwCfgLib.h>
> +#include <Library/QemuFwCfgS3Lib.h>
> #include <Library/DxeServicesTableLib.h>
> #include <Library/PcdLib.h>
> #include <Library/OrderedCollectionLib.h>
> diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c b/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c
> index 818646a275a9..3da9cd21e5c9 100644
> --- a/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c
> +++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c
> @@ -19,6 +19,7 @@
> #include <Library/BaseMemoryLib.h>
> #include <Library/DebugLib.h>
> #include <Library/QemuFwCfgLib.h>
> +#include <Library/QemuFwCfgS3Lib.h>
> #include <Protocol/LockBox.h>
> #include <LockBoxLib.h>
>
> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> index 3dd55ba5042e..1bf725d8b7ae 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
> @@ -368,31 +368,3 @@ QemuFwCfgFindFile (
>
> return RETURN_NOT_FOUND;
> }
> -
> -
> -/**
> - Determine if S3 support is explicitly enabled.
> -
> - @retval TRUE if S3 support is explicitly enabled.
> - FALSE otherwise. This includes unavailability of the firmware
> - configuration interface.
> -**/
> -BOOLEAN
> -EFIAPI
> -QemuFwCfgS3Enabled (
> - VOID
> - )
> -{
> - RETURN_STATUS Status;
> - FIRMWARE_CONFIG_ITEM FwCfgItem;
> - UINTN FwCfgSize;
> - UINT8 SystemStates[6];
> -
> - Status = QemuFwCfgFindFile ("etc/system-states", &FwCfgItem, &FwCfgSize);
> - if (Status != RETURN_SUCCESS || FwCfgSize != sizeof SystemStates) {
> - return FALSE;
> - }
> - QemuFwCfgSelectItem (FwCfgItem);
> - QemuFwCfgReadBytes (sizeof SystemStates, SystemStates);
> - return (BOOLEAN) (SystemStates[3] & BIT7);
> -}
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 0be86722e548..77a8a16c15b8 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -32,6 +32,7 @@
> #include <Library/PeimEntryPoint.h>
> #include <Library/PeiServicesLib.h>
> #include <Library/QemuFwCfgLib.h>
> +#include <Library/QemuFwCfgS3Lib.h>
> #include <Library/ResourcePublicationLib.h>
> #include <Guid/MemoryTypeInformation.h>
> #include <Ppi/MasterBootMode.h>
> diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> index f31646d73461..bb79fce0855b 100644
> --- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> +++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
> @@ -32,6 +32,7 @@
> #include <Library/PcdLib.h>
> #include <Library/PciLib.h>
> #include <Library/QemuFwCfgLib.h>
> +#include <Library/QemuFwCfgS3Lib.h>
> #include <Library/UefiBootServicesTableLib.h>
> #include <Protocol/S3SaveState.h>
> #include <Protocol/SmmControl2.h>
> --
> 2.9.3
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 07/12] OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to libclass
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (5 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 06/12] ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-03-10 9:58 ` Jordan Justen
2017-02-23 1:48 ` [PATCH 08/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for Base Null instance Laszlo Ersek
` (6 subsequent siblings)
13 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
Introduce the following APIs:
- QemuFwCfgS3TransferOwnership(): central function that registers a
callback function, with a context parameter, for when ACPI S3 Boot
Script opcodes can be produced. This function also allocates reserved
memory for the opcodes to operate upon.
The client module is supposed to produce the boot script fragment in the
callback function.
- QemuFwCfgS3WriteBytes(), QemuFwCfgS3ReadBytes(), QemuFwCfgS3SkipBytes(),
QemuFwCfgS3CheckValue(): helper functions, available only to the above
callback function, for composing the boot script fragment.
QemuFwCfgS3SkipBytes() can double as a plain "select" whenever
necessary.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Include/Library/QemuFwCfgS3Lib.h | 318 ++++++++++++++++++++
1 file changed, 318 insertions(+)
diff --git a/OvmfPkg/Include/Library/QemuFwCfgS3Lib.h b/OvmfPkg/Include/Library/QemuFwCfgS3Lib.h
index 1c473610d11c..3499de3c74cc 100644
--- a/OvmfPkg/Include/Library/QemuFwCfgS3Lib.h
+++ b/OvmfPkg/Include/Library/QemuFwCfgS3Lib.h
@@ -19,6 +19,8 @@
#ifndef __FW_CFG_S3_LIB__
#define __FW_CFG_S3_LIB__
+#include <Base.h>
+
/**
Determine if S3 support is explicitly enabled.
@@ -36,4 +38,320 @@ QemuFwCfgS3Enabled (
VOID
);
+
+/**
+ Prototype for the callback function that the client module provides.
+
+ In the callback function, the client module calls the
+ QemuFwCfgS3WriteBytes(), QemuFwCfgS3ReadBytes(), QemuFwCfgS3SkipBytes(), and
+ QemuFwCfgS3CheckValue() functions. Those functions produce ACPI S3 Boot
+ Script opcodes that will perform fw_cfg DMA operations, and will check any
+ desired values that were read, during S3 resume.
+
+ The callback function is invoked when the production of ACPI S3 Boot Script
+ opcodes becomes possible. This may occur directly on the call stack of
+ QemuFwCfgS3TransferOwnership() (see below), or after
+ QemuFwCfgS3TransferOwnership() has successfully returned.
+
+ The callback function must not return if it fails -- in the general case,
+ there is noone to propagate any errors to. Therefore, on error, an error
+ message should be logged, and CpuDeadLoop() must be called.
+
+ @param[in,out] Context Carries information from the client module
+ itself (i.e., from the invocation of
+ QemuFwCfgS3TransferOwnership()) to the callback
+ function.
+
+ If Context points to dynamically allocated
+ storage, then the callback function must
+ release it.
+
+ @param[in,out] ScratchBuffer Points to reserved memory, allocated by
+ QemuFwCfgS3TransferOwnership() internally.
+
+ ScratchBuffer is typed and sized by the client
+ module when it calls
+ QemuFwCfgS3TransferOwnership(). The client
+ module defines a union type of structures for
+ ScratchBuffer such that the union can hold
+ client data for any desired fw_cfg DMA read and
+ write operations, and value checking.
+
+ The callback function casts ScratchBuffer to
+ the union type described above. It passes union
+ member sizes as NumberOfBytes to
+ QemuFwCfgS3ReadBytes() and
+ QemuFwCfgS3WriteBytes(). It passes field
+ addresses and sizes in structures in the union
+ as ScratchData and ValueSize to
+ QemuFwCfgS3CheckValue().
+
+ ScratchBuffer is aligned at 8 bytes.
+**/
+typedef
+VOID (EFIAPI FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION) (
+ IN OUT VOID *Context, OPTIONAL
+ IN OUT VOID *ScratchBuffer
+ );
+
+
+/**
+ Install the client module's FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION callback for
+ when the production of ACPI S3 Boot Script opcodes becomes possible.
+
+ Take ownership of the client-provided Context, and pass it to the callback
+ function, when the latter is invoked.
+
+ Allocate scratch space for those ACPI S3 Boot Script opcodes to work upon
+ that the client will produce in the callback function.
+
+ @param[in] Append FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION to invoke
+ when the production of ACPI S3 Boot Script
+ opcodes becomes possible. Append() may be
+ called immediately from
+ QemuFwCfgS3TransferOwnership().
+
+ @param[in,out] Context Client-provided data structure for the Append()
+ callback function to consume.
+
+ If Context points to dynamically allocated
+ memory, then Append() must release it.
+
+ If Context points to dynamically allocated
+ memory, and QemuFwCfgS3TransferOwnership()
+ returns successfully, then the caller of
+ QemuFwCfgS3TransferOwnership() must neither
+ dereference nor even evaluate Context any
+ longer, as ownership of the referenced area has
+ been transferred to Append().
+
+ @param[in] ScratchBufferSize The size of the scratch buffer that will hold,
+ in reserved memory, all client data read,
+ written, and checked by the ACPI S3 Boot Script
+ opcodes produced by Append().
+
+ @retval RETURN_UNSUPPORTED The library instance does not support this
+ function.
+
+ @retval RETURN_NOT_FOUND The fw_cfg DMA interface to QEMU is
+ unavailable.
+
+ @retval RETURN_BAD_BUFFER_SIZE ScratchBufferSize is too large.
+
+ @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
+
+ @retval RETURN_SUCCESS Append() has been installed, and the
+ ownership of Context has been transferred.
+ Reserved memory has been allocated for the
+ scratch buffer.
+
+ A successful invocation of
+ QemuFwCfgS3TransferOwnership() cannot be
+ rolled back.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3TransferOwnership (
+ IN FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION *Append,
+ IN OUT VOID *Context, OPTIONAL
+ IN UINTN ScratchBufferSize
+ );
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that (optionally) select an fw_cfg item,
+ and transfer data to it.
+
+ The opcodes produced by QemuFwCfgS3WriteBytes() will first restore
+ NumberOfBytes bytes in ScratchBuffer in-place, in reserved memory, then write
+ them to fw_cfg using DMA.
+
+ If the operation fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] FirmwareConfigItem The UINT16 selector key of the firmware config
+ item to write, expressed as INT32. If
+ FirmwareConfigItem is -1, no selection is
+ made, the write will occur to the currently
+ selected item, at its currently selected
+ offset. Otherwise, the specified item will be
+ selected, and the write will occur at offset
+ 0.
+
+ @param[in] NumberOfBytes Size of the data to restore in ScratchBuffer,
+ and to write from ScratchBuffer, during S3
+ resume. NumberOfBytes must not exceed
+ ScratchBufferSize, which was passed to
+ QemuFwCfgS3TransferOwnership().
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER FirmwareConfigItem is invalid.
+
+ @retval RETURN_BAD_BUFFER_SIZE NumberOfBytes is larger than
+ ScratchBufferSize.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3WriteBytes (
+ IN INT32 FirmwareConfigItem,
+ IN UINTN NumberOfBytes
+ );
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that (optionally) select an fw_cfg item,
+ and transfer data from it.
+
+ The opcodes produced by QemuFwCfgS3ReadBytes() will read NumberOfBytes bytes
+ from fw_cfg using DMA, storing the result in ScratchBuffer, in reserved
+ memory.
+
+ If the operation fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] FirmwareConfigItem The UINT16 selector key of the firmware config
+ item to read, expressed as INT32. If
+ FirmwareConfigItem is -1, no selection is
+ made, the read will occur from the currently
+ selected item, from its currently selected
+ offset. Otherwise, the specified item will be
+ selected, and the read will occur from offset
+ 0.
+
+ @param[in] NumberOfBytes Size of the data to read during S3 resume.
+ NumberOfBytes must not exceed
+ ScratchBufferSize, which was passed to
+ QemuFwCfgS3TransferOwnership().
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER FirmwareConfigItem is invalid.
+
+ @retval RETURN_BAD_BUFFER_SIZE NumberOfBytes is larger than
+ ScratchBufferSize.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3ReadBytes (
+ IN INT32 FirmwareConfigItem,
+ IN UINTN NumberOfBytes
+ );
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that (optionally) select an fw_cfg item,
+ and increase its offset.
+
+ If the operation fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] FirmwareConfigItem The UINT16 selector key of the firmware config
+ item to advance the offset of, expressed as
+ INT32. If FirmwareConfigItem is -1, no
+ selection is made, and the offset for the
+ currently selected item is increased.
+ Otherwise, the specified item will be
+ selected, and the offset increment will occur
+ from offset 0.
+
+ @param[in] NumberOfBytes The number of bytes to skip in the subject
+ fw_cfg item.
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER FirmwareConfigItem is invalid.
+
+ @retval RETURN_BAD_BUFFER_SIZE NumberOfBytes is too large.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3SkipBytes (
+ IN INT32 FirmwareConfigItem,
+ IN UINTN NumberOfBytes
+ );
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that check a value in ScratchBuffer.
+
+ If the check fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] ScratchData Pointer to the UINT8, UINT16, UINT32 or UINT64 field
+ in ScratchBuffer that should be checked. The caller
+ is responsible for populating the field during S3
+ resume, by calling QemuFwCfgS3ReadBytes() ahead of
+ QemuFwCfgS3CheckValue().
+
+ ScratchData must point into ScratchBuffer, which was
+ allocated, and passed to Append(), by
+ QemuFwCfgS3TransferOwnership().
+
+ ScratchData must be aligned at ValueSize bytes.
+
+ @param[in] ValueSize One of 1, 2, 4 or 8, specifying the size of the field
+ to check.
+
+ @param[in] ValueMask The value read from ScratchData is binarily AND-ed
+ with ValueMask, and the result is compared against
+ Value. If the masked data equals Value, the check
+ passes, and the boot script can proceed. Otherwise,
+ the check fails, and the boot script hangs.
+
+ @param[in] Value Refer to ValueMask.
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER ValueSize is invalid.
+
+ @retval RETURN_INVALID_PARAMETER ValueMask or Value cannot be represented in
+ ValueSize bytes.
+
+ @retval RETURN_INVALID_PARAMETER ScratchData is not aligned at ValueSize
+ bytes.
+
+ @retval RETURN_BAD_BUFFER_SIZE The ValueSize bytes at ScratchData aren't
+ wholly contained in the ScratchBufferSize
+ bytes at ScratchBuffer.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3CheckValue (
+ IN VOID *ScratchData,
+ IN UINT8 ValueSize,
+ IN UINT64 ValueMask,
+ IN UINT64 Value
+ );
+
#endif
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 07/12] OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to libclass
2017-02-23 1:48 ` [PATCH 07/12] OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to libclass Laszlo Ersek
@ 2017-03-10 9:58 ` Jordan Justen
2017-03-10 19:14 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Jordan Justen @ 2017-03-10 9:58 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
On 2017-02-22 17:48:09, Laszlo Ersek wrote:
> +EFIAPI
> +RETURN_STATUS
> +QemuFwCfgS3TransferOwnership (
> + IN FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION *Append,
> + IN OUT VOID *Context, OPTIONAL
> + IN UINTN ScratchBufferSize
> + );
I think this might be better named something like
QemuFwCfgS3CallWhenBootScriptReady. (Wow, that is verbose. Maybe you
have a better idea.)
-Jordan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 07/12] OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to libclass
2017-03-10 9:58 ` Jordan Justen
@ 2017-03-10 19:14 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-03-10 19:14 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01
On 03/10/17 10:58, Jordan Justen wrote:
> On 2017-02-22 17:48:09, Laszlo Ersek wrote:
>> +EFIAPI
>> +RETURN_STATUS
>> +QemuFwCfgS3TransferOwnership (
>> + IN FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION *Append,
>> + IN OUT VOID *Context, OPTIONAL
>> + IN UINTN ScratchBufferSize
>> + );
>
> I think this might be better named something like
> QemuFwCfgS3CallWhenBootScriptReady. (Wow, that is verbose. Maybe you
> have a better idea.)
No, I like your suggestion.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 08/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for Base Null instance
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (6 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 07/12] OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to libclass Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 1:48 ` [PATCH 09/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for PEI fw_cfg instance Laszlo Ersek
` (5 subsequent siblings)
13 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
In the Base Null instance:
- QemuFwCfgS3Enabled() returns constant FALSE. This is unique to the Base
Null instance, and the function is already present in
"QemuFwCfgS3Base.c".
- The QemuFwCfgS3TransferOwnership() function must never be called
(according to the documentation, given the above). This is also unique
to the Base Null instance, so implement the function in
"QemuFwCfgS3Base.c".
- Consequently, the QemuFwCfgS3WriteBytes(), QemuFwCfgS3ReadBytes(),
QemuFwCfgS3SkipBytes(), and QemuFwCfgS3CheckValue() functions must never
be called either. This behavior is not unique to the Base Null instance
(it will be shared with the PEI fw_cfg instance), so add these functions
to "QemuFwCfgS3BasePei.c".
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf | 4 +
OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c | 70 ++++++
OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c | 227 ++++++++++++++++++++
3 files changed, 301 insertions(+)
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf b/OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
index ba24a0b9d434..837fd70db6e5 100644
--- a/OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
@@ -33,7 +33,11 @@ [Defines]
[Sources]
QemuFwCfgS3Base.c
+ QemuFwCfgS3BasePei.c
[Packages]
MdePkg/MdePkg.dec
OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+ DebugLib
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c
index bed9bf3dfb57..834ac8e523c7 100644
--- a/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c
@@ -16,6 +16,7 @@
WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
**/
+#include <Library/DebugLib.h>
#include <Library/QemuFwCfgS3Lib.h>
/**
@@ -37,3 +38,72 @@ QemuFwCfgS3Enabled (
{
return FALSE;
}
+
+
+/**
+ Install the client module's FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION callback for
+ when the production of ACPI S3 Boot Script opcodes becomes possible.
+
+ Take ownership of the client-provided Context, and pass it to the callback
+ function, when the latter is invoked.
+
+ Allocate scratch space for those ACPI S3 Boot Script opcodes to work upon
+ that the client will produce in the callback function.
+
+ @param[in] Append FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION to invoke
+ when the production of ACPI S3 Boot Script
+ opcodes becomes possible. Append() may be
+ called immediately from
+ QemuFwCfgS3TransferOwnership().
+
+ @param[in,out] Context Client-provided data structure for the Append()
+ callback function to consume.
+
+ If Context points to dynamically allocated
+ memory, then Append() must release it.
+
+ If Context points to dynamically allocated
+ memory, and QemuFwCfgS3TransferOwnership()
+ returns successfully, then the caller of
+ QemuFwCfgS3TransferOwnership() must neither
+ dereference nor even evaluate Context any
+ longer, as ownership of the referenced area has
+ been transferred to Append().
+
+ @param[in] ScratchBufferSize The size of the scratch buffer that will hold,
+ in reserved memory, all client data read,
+ written, and checked by the ACPI S3 Boot Script
+ opcodes produced by Append().
+
+ @retval RETURN_UNSUPPORTED The library instance does not support this
+ function.
+
+ @retval RETURN_NOT_FOUND The fw_cfg DMA interface to QEMU is
+ unavailable.
+
+ @retval RETURN_BAD_BUFFER_SIZE ScratchBufferSize is too large.
+
+ @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
+
+ @retval RETURN_SUCCESS Append() has been installed, and the
+ ownership of Context has been transferred.
+ Reserved memory has been allocated for the
+ scratch buffer.
+
+ A successful invocation of
+ QemuFwCfgS3TransferOwnership() cannot be
+ rolled back.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3TransferOwnership (
+ IN FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION *Append,
+ IN OUT VOID *Context, OPTIONAL
+ IN UINTN ScratchBufferSize
+ )
+{
+ ASSERT (FALSE);
+ return RETURN_UNSUPPORTED;
+}
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c
new file mode 100644
index 000000000000..537678999c5f
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c
@@ -0,0 +1,227 @@
+/** @file
+ Shared code for the Base Null and PEI fw_cfg instances of the QemuFwCfgS3Lib
+ class.
+
+ Copyright (C) 2017, 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.
+**/
+
+#include <Library/DebugLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
+
+/**
+ Produce ACPI S3 Boot Script opcodes that (optionally) select an fw_cfg item,
+ and transfer data to it.
+
+ The opcodes produced by QemuFwCfgS3WriteBytes() will first restore
+ NumberOfBytes bytes in ScratchBuffer in-place, in reserved memory, then write
+ them to fw_cfg using DMA.
+
+ If the operation fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] FirmwareConfigItem The UINT16 selector key of the firmware config
+ item to write, expressed as INT32. If
+ FirmwareConfigItem is -1, no selection is
+ made, the write will occur to the currently
+ selected item, at its currently selected
+ offset. Otherwise, the specified item will be
+ selected, and the write will occur at offset
+ 0.
+
+ @param[in] NumberOfBytes Size of the data to restore in ScratchBuffer,
+ and to write from ScratchBuffer, during S3
+ resume. NumberOfBytes must not exceed
+ ScratchBufferSize, which was passed to
+ QemuFwCfgS3TransferOwnership().
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER FirmwareConfigItem is invalid.
+
+ @retval RETURN_BAD_BUFFER_SIZE NumberOfBytes is larger than
+ ScratchBufferSize.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3WriteBytes (
+ IN INT32 FirmwareConfigItem,
+ IN UINTN NumberOfBytes
+ )
+{
+ ASSERT (FALSE);
+ return RETURN_UNSUPPORTED;
+}
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that (optionally) select an fw_cfg item,
+ and transfer data from it.
+
+ The opcodes produced by QemuFwCfgS3ReadBytes() will read NumberOfBytes bytes
+ from fw_cfg using DMA, storing the result in ScratchBuffer, in reserved
+ memory.
+
+ If the operation fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] FirmwareConfigItem The UINT16 selector key of the firmware config
+ item to read, expressed as INT32. If
+ FirmwareConfigItem is -1, no selection is
+ made, the read will occur from the currently
+ selected item, from its currently selected
+ offset. Otherwise, the specified item will be
+ selected, and the read will occur from offset
+ 0.
+
+ @param[in] NumberOfBytes Size of the data to read during S3 resume.
+ NumberOfBytes must not exceed
+ ScratchBufferSize, which was passed to
+ QemuFwCfgS3TransferOwnership().
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER FirmwareConfigItem is invalid.
+
+ @retval RETURN_BAD_BUFFER_SIZE NumberOfBytes is larger than
+ ScratchBufferSize.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3ReadBytes (
+ IN INT32 FirmwareConfigItem,
+ IN UINTN NumberOfBytes
+ )
+{
+ ASSERT (FALSE);
+ return RETURN_UNSUPPORTED;
+}
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that (optionally) select an fw_cfg item,
+ and increase its offset.
+
+ If the operation fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] FirmwareConfigItem The UINT16 selector key of the firmware config
+ item to advance the offset of, expressed as
+ INT32. If FirmwareConfigItem is -1, no
+ selection is made, and the offset for the
+ currently selected item is increased.
+ Otherwise, the specified item will be
+ selected, and the offset increment will occur
+ from offset 0.
+
+ @param[in] NumberOfBytes The number of bytes to skip in the subject
+ fw_cfg item.
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER FirmwareConfigItem is invalid.
+
+ @retval RETURN_BAD_BUFFER_SIZE NumberOfBytes is too large.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3SkipBytes (
+ IN INT32 FirmwareConfigItem,
+ IN UINTN NumberOfBytes
+ )
+{
+ ASSERT (FALSE);
+ return RETURN_UNSUPPORTED;
+}
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that check a value in ScratchBuffer.
+
+ If the check fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] ScratchData Pointer to the UINT8, UINT16, UINT32 or UINT64 field
+ in ScratchBuffer that should be checked. The caller
+ is responsible for populating the field during S3
+ resume, by calling QemuFwCfgS3ReadBytes() ahead of
+ QemuFwCfgS3CheckValue().
+
+ ScratchData must point into ScratchBuffer, which was
+ allocated, and passed to Append(), by
+ QemuFwCfgS3TransferOwnership().
+
+ ScratchData must be aligned at ValueSize bytes.
+
+ @param[in] ValueSize One of 1, 2, 4 or 8, specifying the size of the field
+ to check.
+
+ @param[in] ValueMask The value read from ScratchData is binarily AND-ed
+ with ValueMask, and the result is compared against
+ Value. If the masked data equals Value, the check
+ passes, and the boot script can proceed. Otherwise,
+ the check fails, and the boot script hangs.
+
+ @param[in] Value Refer to ValueMask.
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER ValueSize is invalid.
+
+ @retval RETURN_INVALID_PARAMETER ValueMask or Value cannot be represented in
+ ValueSize bytes.
+
+ @retval RETURN_INVALID_PARAMETER ScratchData is not aligned at ValueSize
+ bytes.
+
+ @retval RETURN_BAD_BUFFER_SIZE The ValueSize bytes at ScratchData aren't
+ wholly contained in the ScratchBufferSize
+ bytes at ScratchBuffer.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3CheckValue (
+ IN VOID *ScratchData,
+ IN UINT8 ValueSize,
+ IN UINT64 ValueMask,
+ IN UINT64 Value
+ )
+{
+ ASSERT (FALSE);
+ return RETURN_UNSUPPORTED;
+}
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 09/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for PEI fw_cfg instance
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (7 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 08/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for Base Null instance Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 1:48 ` [PATCH 10/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for DXE " Laszlo Ersek
` (4 subsequent siblings)
13 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
In the PEI fw_cfg instance:
- QemuFwCfgS3Enabled() queries S3 enablement via fw_cfg. This behavior is
shared with the DXE fw_cfg instance, and the PEI fw_cfg instance already
pulls in the function from "QemuFwCfgS3PeiDxe.c".
- If QemuFwCfgS3Enabled() returns TRUE, the client module is permitted to
call QemuFwCfgS3TransferOwnership(). However, in the PEI phase we have
no support for capturing ACPI S3 Boot Script opcodes, hence we return
RETURN_UNSUPPORTED unconditionally. This behavior is unique to the PEI
fw_cfg instance, so add the function to "QemuFwCfgS3Pei.c".
- Consequently, the QemuFwCfgS3WriteBytes(), QemuFwCfgS3ReadBytes(),
QemuFwCfgS3SkipBytes(), and QemuFwCfgS3CheckValue() functions must never
be called. (They could only be called from the client module's callback,
but QemuFwCfgS3TransferOwnership() will never install such callback in
the PEI fw_cfg instance -- see above.)
This behavior is not unique to the PEI fw_cfg instance (it is shared
with the Base Null instance), so pull in these functions from
"QemuFwCfgS3BasePei.c".
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf | 3 +
OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c | 85 ++++++++++++++++++++
2 files changed, 88 insertions(+)
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf b/OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
index 2593af8e5b4c..890862076e81 100644
--- a/OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
@@ -31,6 +31,8 @@ [Defines]
#
[Sources]
+ QemuFwCfgS3BasePei.c
+ QemuFwCfgS3Pei.c
QemuFwCfgS3PeiDxe.c
[Packages]
@@ -38,4 +40,5 @@ [Packages]
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ DebugLib
QemuFwCfgLib
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c
new file mode 100644
index 000000000000..77ed058884e1
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c
@@ -0,0 +1,85 @@
+/** @file
+ Limited functionality QemuFwCfgS3Lib instance, for PEI phase modules.
+
+ QemuFwCfgS3Enabled() queries S3 enablement via fw_cfg. Other library APIs
+ will report lack of support.
+
+ Copyright (C) 2017, 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.
+**/
+
+#include <Library/QemuFwCfgS3Lib.h>
+
+/**
+ Install the client module's FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION callback for
+ when the production of ACPI S3 Boot Script opcodes becomes possible.
+
+ Take ownership of the client-provided Context, and pass it to the callback
+ function, when the latter is invoked.
+
+ Allocate scratch space for those ACPI S3 Boot Script opcodes to work upon
+ that the client will produce in the callback function.
+
+ @param[in] Append FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION to invoke
+ when the production of ACPI S3 Boot Script
+ opcodes becomes possible. Append() may be
+ called immediately from
+ QemuFwCfgS3TransferOwnership().
+
+ @param[in,out] Context Client-provided data structure for the Append()
+ callback function to consume.
+
+ If Context points to dynamically allocated
+ memory, then Append() must release it.
+
+ If Context points to dynamically allocated
+ memory, and QemuFwCfgS3TransferOwnership()
+ returns successfully, then the caller of
+ QemuFwCfgS3TransferOwnership() must neither
+ dereference nor even evaluate Context any
+ longer, as ownership of the referenced area has
+ been transferred to Append().
+
+ @param[in] ScratchBufferSize The size of the scratch buffer that will hold,
+ in reserved memory, all client data read,
+ written, and checked by the ACPI S3 Boot Script
+ opcodes produced by Append().
+
+ @retval RETURN_UNSUPPORTED The library instance does not support this
+ function.
+
+ @retval RETURN_NOT_FOUND The fw_cfg DMA interface to QEMU is
+ unavailable.
+
+ @retval RETURN_BAD_BUFFER_SIZE ScratchBufferSize is too large.
+
+ @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
+
+ @retval RETURN_SUCCESS Append() has been installed, and the
+ ownership of Context has been transferred.
+ Reserved memory has been allocated for the
+ scratch buffer.
+
+ A successful invocation of
+ QemuFwCfgS3TransferOwnership() cannot be
+ rolled back.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3TransferOwnership (
+ IN FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION *Append,
+ IN OUT VOID *Context, OPTIONAL
+ IN UINTN ScratchBufferSize
+ )
+{
+ return RETURN_UNSUPPORTED;
+}
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 10/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for DXE fw_cfg instance
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (8 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 09/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for PEI fw_cfg instance Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 1:48 ` [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib Laszlo Ersek
` (3 subsequent siblings)
13 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
In the DXE fw_cfg instance:
- QemuFwCfgS3Enabled() queries S3 enablement via fw_cfg. This behavior is
shared with the PEI fw_cfg instance, and the DXE fw_cfg instance already
pulls in the function from "QemuFwCfgS3PeiDxe.c".
- If QemuFwCfgS3Enabled() returns TRUE, the client module is permitted to
call QemuFwCfgS3TransferOwnership().
We provide a fully functional implementation for
QemuFwCfgS3TransferOwnership(). A protocol notify is installed at
TPL_CALLBACK for EFI_S3_SAVE_STATE_PROTOCOL. If / once the protocol is
available, the client module's Append() function is called, which is
expected to produce ACPI S3 Boot Script opcodes using the helper
functions listed below. In QemuFwCfgS3TransferOwnership(), we also
allocate a reserved memory buffer, sized & typed by the client module,
for the opcodes and (internally) the fw_cfg DMA operations to work upon,
during S3 resume.
This behavior is unique to the DXE fw_cfg instance. Thus, add the
function to "QemuFwCfgS3Dxe.c".
- The QemuFwCfgS3WriteBytes(), QemuFwCfgS3ReadBytes(),
QemuFwCfgS3SkipBytes(), and QemuFwCfgS3CheckValue() functions are also
implemented usefully, since the client module's Append() function is
expected to invoke them.
Each of the first three functions produces MEM_WRITE, IO_WRITE, and
MEM_POLL opcodes, to set up the DMA command in reserved memory, to start
the DMA transfer, and to check the DMA result, respectively.
The QemuFwCfgS3CheckValue() function produces a MEM_POLL opcode to
validate an unsigned integer field in data that was read via
QemuFwCfgS3ReadBytes().
This behavior is again unique to the DXE fw_cfg instance, so add the
functions to "QemuFwCfgS3Dxe.c".
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf | 8 +
OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c | 791 ++++++++++++++++++++
2 files changed, 799 insertions(+)
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf b/OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
index 7016575f3dab..a0e4275cb8a5 100644
--- a/OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
@@ -28,6 +28,7 @@ [Defines]
#
[Sources]
+ QemuFwCfgS3Dxe.c
QemuFwCfgS3PeiDxe.c
[Packages]
@@ -35,4 +36,11 @@ [Packages]
OvmfPkg/OvmfPkg.dec
[LibraryClasses]
+ BaseLib
+ DebugLib
+ MemoryAllocationLib
QemuFwCfgLib
+ UefiBootServicesTableLib
+
+[Protocols]
+ gEfiS3SaveStateProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c
new file mode 100644
index 000000000000..51d99d66a98b
--- /dev/null
+++ b/OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c
@@ -0,0 +1,791 @@
+/** @file
+ Full functionality QemuFwCfgS3Lib instance, for DXE phase modules.
+
+ Copyright (C) 2017, 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.
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/QemuFwCfgLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/S3SaveState.h>
+
+
+//
+// Event to signal when the S3SaveState protocol interface is installed.
+//
+STATIC EFI_EVENT mS3SaveStateInstalledEvent;
+
+//
+// Reference to the S3SaveState protocol interface, after it is installed.
+//
+STATIC EFI_S3_SAVE_STATE_PROTOCOL *mS3SaveState;
+
+//
+// The control structure is allocated in reserved memory, aligned at 8 bytes.
+// The client-requested ScratchBuffer will be allocated adjacently, also
+// aligned at 8 bytes.
+//
+#define RESERVED_MEM_ALIGNMENT 8
+
+STATIC FW_CFG_DMA_ACCESS *mDmaAccess;
+STATIC VOID *mScratchBuffer;
+STATIC UINTN mScratchBufferSize;
+
+//
+// Callback provided by the client, for appending ACPI S3 Boot Script opcodes.
+// To be called from S3SaveStateInstalledNotify().
+//
+STATIC FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION *mAppend;
+
+
+/**
+ Event notification function for mS3SaveStateInstalledEvent.
+**/
+STATIC
+VOID
+EFIAPI
+S3SaveStateInstalledNotify (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ EFI_STATUS Status;
+
+ ASSERT (Event == mS3SaveStateInstalledEvent);
+
+ Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid,
+ NULL /* Registration */, (VOID **)&mS3SaveState);
+ if (EFI_ERROR (Status)) {
+ return;
+ }
+
+ ASSERT (mAppend != NULL);
+
+ DEBUG ((DEBUG_INFO, "%a: %a: DmaAccess@0x%Lx ScratchBuffer@[0x%Lx+0x%Lx]\n",
+ gEfiCallerBaseName, __FUNCTION__, (UINT64)(UINTN)mDmaAccess,
+ (UINT64)(UINTN)mScratchBuffer, (UINT64)mScratchBufferSize));
+ mAppend (Context, mScratchBuffer);
+
+ gBS->CloseEvent (mS3SaveStateInstalledEvent);
+ mS3SaveStateInstalledEvent = NULL;
+}
+
+
+/**
+ Install the client module's FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION callback for
+ when the production of ACPI S3 Boot Script opcodes becomes possible.
+
+ Take ownership of the client-provided Context, and pass it to the callback
+ function, when the latter is invoked.
+
+ Allocate scratch space for those ACPI S3 Boot Script opcodes to work upon
+ that the client will produce in the callback function.
+
+ @param[in] Append FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION to invoke
+ when the production of ACPI S3 Boot Script
+ opcodes becomes possible. Append() may be
+ called immediately from
+ QemuFwCfgS3TransferOwnership().
+
+ @param[in,out] Context Client-provided data structure for the Append()
+ callback function to consume.
+
+ If Context points to dynamically allocated
+ memory, then Append() must release it.
+
+ If Context points to dynamically allocated
+ memory, and QemuFwCfgS3TransferOwnership()
+ returns successfully, then the caller of
+ QemuFwCfgS3TransferOwnership() must neither
+ dereference nor even evaluate Context any
+ longer, as ownership of the referenced area has
+ been transferred to Append().
+
+ @param[in] ScratchBufferSize The size of the scratch buffer that will hold,
+ in reserved memory, all client data read,
+ written, and checked by the ACPI S3 Boot Script
+ opcodes produced by Append().
+
+ @retval RETURN_UNSUPPORTED The library instance does not support this
+ function.
+
+ @retval RETURN_NOT_FOUND The fw_cfg DMA interface to QEMU is
+ unavailable.
+
+ @retval RETURN_BAD_BUFFER_SIZE ScratchBufferSize is too large.
+
+ @retval RETURN_OUT_OF_RESOURCES Memory allocation failed.
+
+ @retval RETURN_SUCCESS Append() has been installed, and the
+ ownership of Context has been transferred.
+ Reserved memory has been allocated for the
+ scratch buffer.
+
+ A successful invocation of
+ QemuFwCfgS3TransferOwnership() cannot be
+ rolled back.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3TransferOwnership (
+ IN FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION *Append,
+ IN OUT VOID *Context, OPTIONAL
+ IN UINTN ScratchBufferSize
+ )
+{
+ EFI_STATUS Status;
+ VOID *Registration;
+
+ //
+ // Basic fw_cfg is certainly available, as we can only be here after a
+ // successful call to QemuFwCfgS3Enabled(). Check fw_cfg DMA availability.
+ //
+ ASSERT (QemuFwCfgIsAvailable ());
+ QemuFwCfgSelectItem (QemuFwCfgItemInterfaceVersion);
+ if ((QemuFwCfgRead32 () & FW_CFG_F_DMA) == 0) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: fw_cfg DMA unavailable\n",
+ gEfiCallerBaseName, __FUNCTION__));
+ return RETURN_NOT_FOUND;
+ }
+
+ //
+ // Allocate a reserved buffer for the DMA access control structure and the
+ // client data together.
+ //
+ if (ScratchBufferSize >
+ MAX_UINT32 - (RESERVED_MEM_ALIGNMENT - 1) - sizeof *mDmaAccess) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: ScratchBufferSize too big: %Lu\n",
+ gEfiCallerBaseName, __FUNCTION__, (UINT64)ScratchBufferSize));
+ return RETURN_BAD_BUFFER_SIZE;
+ }
+ mDmaAccess = AllocateReservedPool ((RESERVED_MEM_ALIGNMENT - 1) +
+ sizeof *mDmaAccess + ScratchBufferSize);
+ if (mDmaAccess == NULL) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: AllocateReservedPool(): out of resources\n",
+ gEfiCallerBaseName, __FUNCTION__));
+ return RETURN_OUT_OF_RESOURCES;
+ }
+ mDmaAccess = ALIGN_POINTER (mDmaAccess, RESERVED_MEM_ALIGNMENT);
+
+ //
+ // Set up a protocol notify for EFI_S3_SAVE_STATE_PROTOCOL. Forward the
+ // client's Context to the callback.
+ //
+ Status = gBS->CreateEvent (EVT_NOTIFY_SIGNAL, TPL_CALLBACK,
+ S3SaveStateInstalledNotify, Context,
+ &mS3SaveStateInstalledEvent);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: CreateEvent(): %r\n", gEfiCallerBaseName,
+ __FUNCTION__, Status));
+ goto FreeDmaAccess;
+ }
+ Status = gBS->RegisterProtocolNotify (&gEfiS3SaveStateProtocolGuid,
+ mS3SaveStateInstalledEvent, &Registration);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: RegisterProtocolNotify(): %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ goto CloseEvent;
+ }
+
+ //
+ // Set the remaining global variables. For the alignment guarantee on
+ // mScratchBuffer, we rely on the fact that *mDmaAccess has a size that is an
+ // integral multiple of RESERVED_MEM_ALIGNMENT.
+ //
+ ASSERT (sizeof *mDmaAccess % RESERVED_MEM_ALIGNMENT == 0);
+ mScratchBuffer = mDmaAccess + 1;
+ mScratchBufferSize = ScratchBufferSize;
+ mAppend = Append;
+
+ //
+ // Kick the event; EFI_S3_SAVE_STATE_PROTOCOL could be available already.
+ //
+ Status = gBS->SignalEvent (mS3SaveStateInstalledEvent);
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: SignalEvent(): %r\n", gEfiCallerBaseName,
+ __FUNCTION__, Status));
+ goto NullGlobals;
+ }
+
+ return RETURN_SUCCESS;
+
+NullGlobals:
+ mScratchBuffer = NULL;
+ mScratchBufferSize = 0;
+ mAppend = NULL;
+
+CloseEvent:
+ gBS->CloseEvent (mS3SaveStateInstalledEvent);
+ mS3SaveStateInstalledEvent = NULL;
+
+FreeDmaAccess:
+ FreePool (mDmaAccess);
+ mDmaAccess = NULL;
+
+ return (RETURN_STATUS)Status;
+}
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that (optionally) select an fw_cfg item,
+ and transfer data to it.
+
+ The opcodes produced by QemuFwCfgS3WriteBytes() will first restore
+ NumberOfBytes bytes in ScratchBuffer in-place, in reserved memory, then write
+ them to fw_cfg using DMA.
+
+ If the operation fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] FirmwareConfigItem The UINT16 selector key of the firmware config
+ item to write, expressed as INT32. If
+ FirmwareConfigItem is -1, no selection is
+ made, the write will occur to the currently
+ selected item, at its currently selected
+ offset. Otherwise, the specified item will be
+ selected, and the write will occur at offset
+ 0.
+
+ @param[in] NumberOfBytes Size of the data to restore in ScratchBuffer,
+ and to write from ScratchBuffer, during S3
+ resume. NumberOfBytes must not exceed
+ ScratchBufferSize, which was passed to
+ QemuFwCfgS3TransferOwnership().
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER FirmwareConfigItem is invalid.
+
+ @retval RETURN_BAD_BUFFER_SIZE NumberOfBytes is larger than
+ ScratchBufferSize.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3WriteBytes (
+ IN INT32 FirmwareConfigItem,
+ IN UINTN NumberOfBytes
+ )
+{
+ UINTN Count;
+ EFI_STATUS Status;
+ UINT64 AccessAddress;
+ UINT32 ControlPollData;
+ UINT32 ControlPollMask;
+
+ ASSERT (mDmaAccess != NULL);
+ ASSERT (mS3SaveState != NULL);
+
+ if (FirmwareConfigItem < -1 || FirmwareConfigItem > MAX_UINT16) {
+ return RETURN_INVALID_PARAMETER;
+ }
+ if (NumberOfBytes > mScratchBufferSize) {
+ return RETURN_BAD_BUFFER_SIZE;
+ }
+
+ //
+ // Set up a write[+select] fw_cfg DMA command.
+ //
+ mDmaAccess->Control = FW_CFG_DMA_CTL_WRITE;
+ if (FirmwareConfigItem != -1) {
+ mDmaAccess->Control |= FW_CFG_DMA_CTL_SELECT;
+ mDmaAccess->Control |= (UINT32)FirmwareConfigItem << 16;
+ }
+ mDmaAccess->Control = SwapBytes32 (mDmaAccess->Control);
+
+ //
+ // We ensured the following constraint via mScratchBufferSize in
+ // QemuFwCfgS3TransferOwnership().
+ //
+ ASSERT (NumberOfBytes <= MAX_UINT32);
+ mDmaAccess->Length = SwapBytes32 ((UINT32)NumberOfBytes);
+
+ mDmaAccess->Address = SwapBytes64 ((UINTN)mScratchBuffer);
+
+ //
+ // Copy mDmaAccess and NumberOfBytes bytes from mScratchBuffer into the boot
+ // script. When executed at S3 resume, this opcode will restore all of them
+ // in-place.
+ //
+ Count = (UINTN)mScratchBuffer + NumberOfBytes - (UINTN)mDmaAccess;
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
+ EfiBootScriptWidthUint8, // Width
+ (UINT64)(UINTN)mDmaAccess, // Address
+ Count, // Count
+ (VOID *)mDmaAccess // Buffer
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ //
+ // Append an opcode that will write the address of the fw_cfg DMA command to
+ // the fw_cfg DMA address register, which consists of two 32-bit IO ports.
+ // The second (highest address, least significant) write will start the
+ // transfer.
+ //
+ AccessAddress = SwapBytes64 ((UINTN)mDmaAccess);
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
+ EfiBootScriptWidthUint32, // Width
+ (UINT64)FW_CFG_IO_DMA_ADDRESS, // Address
+ (UINTN)2, // Count
+ (VOID *)&AccessAddress // Buffer
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ //
+ // The following opcode will wait until the Control word reads as zero
+ // (transfer complete). As timeout we use MAX_UINT64 * 100ns, which is
+ // approximately 58494 years.
+ //
+ ControlPollData = 0;
+ ControlPollMask = MAX_UINT32;
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
+ EfiBootScriptWidthUint32, // Width
+ (UINT64)(UINTN)&mDmaAccess->Control, // Address
+ (VOID *)&ControlPollData, // Data
+ (VOID *)&ControlPollMask, // DataMask
+ MAX_UINT64 // Delay
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_MEM_POLL_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ return RETURN_SUCCESS;
+}
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that (optionally) select an fw_cfg item,
+ and transfer data from it.
+
+ The opcodes produced by QemuFwCfgS3ReadBytes() will read NumberOfBytes bytes
+ from fw_cfg using DMA, storing the result in ScratchBuffer, in reserved
+ memory.
+
+ If the operation fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] FirmwareConfigItem The UINT16 selector key of the firmware config
+ item to read, expressed as INT32. If
+ FirmwareConfigItem is -1, no selection is
+ made, the read will occur from the currently
+ selected item, from its currently selected
+ offset. Otherwise, the specified item will be
+ selected, and the read will occur from offset
+ 0.
+
+ @param[in] NumberOfBytes Size of the data to read during S3 resume.
+ NumberOfBytes must not exceed
+ ScratchBufferSize, which was passed to
+ QemuFwCfgS3TransferOwnership().
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER FirmwareConfigItem is invalid.
+
+ @retval RETURN_BAD_BUFFER_SIZE NumberOfBytes is larger than
+ ScratchBufferSize.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3ReadBytes (
+ IN INT32 FirmwareConfigItem,
+ IN UINTN NumberOfBytes
+ )
+{
+ EFI_STATUS Status;
+ UINT64 AccessAddress;
+ UINT32 ControlPollData;
+ UINT32 ControlPollMask;
+
+ ASSERT (mDmaAccess != NULL);
+ ASSERT (mS3SaveState != NULL);
+
+ if (FirmwareConfigItem < -1 || FirmwareConfigItem > MAX_UINT16) {
+ return RETURN_INVALID_PARAMETER;
+ }
+ if (NumberOfBytes > mScratchBufferSize) {
+ return RETURN_BAD_BUFFER_SIZE;
+ }
+
+ //
+ // Set up a read[+select] fw_cfg DMA command.
+ //
+ mDmaAccess->Control = FW_CFG_DMA_CTL_READ;
+ if (FirmwareConfigItem != -1) {
+ mDmaAccess->Control |= FW_CFG_DMA_CTL_SELECT;
+ mDmaAccess->Control |= (UINT32)FirmwareConfigItem << 16;
+ }
+ mDmaAccess->Control = SwapBytes32 (mDmaAccess->Control);
+
+ //
+ // We ensured the following constraint via mScratchBufferSize in
+ // QemuFwCfgS3TransferOwnership().
+ //
+ ASSERT (NumberOfBytes <= MAX_UINT32);
+ mDmaAccess->Length = SwapBytes32 ((UINT32)NumberOfBytes);
+
+ mDmaAccess->Address = SwapBytes64 ((UINTN)mScratchBuffer);
+
+ //
+ // Copy mDmaAccess into the boot script. When executed at S3 resume, this
+ // opcode will restore it in-place.
+ //
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
+ EfiBootScriptWidthUint8, // Width
+ (UINT64)(UINTN)mDmaAccess, // Address
+ sizeof *mDmaAccess, // Count
+ (VOID *)mDmaAccess // Buffer
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ //
+ // Append an opcode that will write the address of the fw_cfg DMA command to
+ // the fw_cfg DMA address register, which consists of two 32-bit IO ports.
+ // The second (highest address, least significant) write will start the
+ // transfer.
+ //
+ AccessAddress = SwapBytes64 ((UINTN)mDmaAccess);
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
+ EfiBootScriptWidthUint32, // Width
+ (UINT64)FW_CFG_IO_DMA_ADDRESS, // Address
+ (UINTN)2, // Count
+ (VOID *)&AccessAddress // Buffer
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ //
+ // The following opcode will wait until the Control word reads as zero
+ // (transfer complete). As timeout we use MAX_UINT64 * 100ns, which is
+ // approximately 58494 years.
+ //
+ ControlPollData = 0;
+ ControlPollMask = MAX_UINT32;
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
+ EfiBootScriptWidthUint32, // Width
+ (UINT64)(UINTN)&mDmaAccess->Control, // Address
+ (VOID *)&ControlPollData, // Data
+ (VOID *)&ControlPollMask, // DataMask
+ MAX_UINT64 // Delay
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_MEM_POLL_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ return RETURN_SUCCESS;
+}
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that (optionally) select an fw_cfg item,
+ and increase its offset.
+
+ If the operation fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] FirmwareConfigItem The UINT16 selector key of the firmware config
+ item to advance the offset of, expressed as
+ INT32. If FirmwareConfigItem is -1, no
+ selection is made, and the offset for the
+ currently selected item is increased.
+ Otherwise, the specified item will be
+ selected, and the offset increment will occur
+ from offset 0.
+
+ @param[in] NumberOfBytes The number of bytes to skip in the subject
+ fw_cfg item.
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER FirmwareConfigItem is invalid.
+
+ @retval RETURN_BAD_BUFFER_SIZE NumberOfBytes is too large.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3SkipBytes (
+ IN INT32 FirmwareConfigItem,
+ IN UINTN NumberOfBytes
+ )
+{
+ EFI_STATUS Status;
+ UINT64 AccessAddress;
+ UINT32 ControlPollData;
+ UINT32 ControlPollMask;
+
+ ASSERT (mDmaAccess != NULL);
+ ASSERT (mS3SaveState != NULL);
+
+ if (FirmwareConfigItem < -1 || FirmwareConfigItem > MAX_UINT16) {
+ return RETURN_INVALID_PARAMETER;
+ }
+ if (NumberOfBytes > MAX_UINT32) {
+ return RETURN_BAD_BUFFER_SIZE;
+ }
+
+ //
+ // Set up a skip[+select] fw_cfg DMA command.
+ //
+ mDmaAccess->Control = FW_CFG_DMA_CTL_SKIP;
+ if (FirmwareConfigItem != -1) {
+ mDmaAccess->Control |= FW_CFG_DMA_CTL_SELECT;
+ mDmaAccess->Control |= (UINT32)FirmwareConfigItem << 16;
+ }
+ mDmaAccess->Control = SwapBytes32 (mDmaAccess->Control);
+
+ mDmaAccess->Length = SwapBytes32 ((UINT32)NumberOfBytes);
+ mDmaAccess->Address = 0;
+
+ //
+ // Copy mDmaAccess into the boot script. When executed at S3 resume, this
+ // opcode will restore it in-place.
+ //
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
+ EfiBootScriptWidthUint8, // Width
+ (UINT64)(UINTN)mDmaAccess, // Address
+ sizeof *mDmaAccess, // Count
+ (VOID *)mDmaAccess // Buffer
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ //
+ // Append an opcode that will write the address of the fw_cfg DMA command to
+ // the fw_cfg DMA address register, which consists of two 32-bit IO ports.
+ // The second (highest address, least significant) write will start the
+ // transfer.
+ //
+ AccessAddress = SwapBytes64 ((UINTN)mDmaAccess);
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
+ EfiBootScriptWidthUint32, // Width
+ (UINT64)FW_CFG_IO_DMA_ADDRESS, // Address
+ (UINTN)2, // Count
+ (VOID *)&AccessAddress // Buffer
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ //
+ // The following opcode will wait until the Control word reads as zero
+ // (transfer complete). As timeout we use MAX_UINT64 * 100ns, which is
+ // approximately 58494 years.
+ //
+ ControlPollData = 0;
+ ControlPollMask = MAX_UINT32;
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
+ EfiBootScriptWidthUint32, // Width
+ (UINT64)(UINTN)&mDmaAccess->Control, // Address
+ (VOID *)&ControlPollData, // Data
+ (VOID *)&ControlPollMask, // DataMask
+ MAX_UINT64 // Delay
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_MEM_POLL_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ return RETURN_SUCCESS;
+}
+
+
+/**
+ Produce ACPI S3 Boot Script opcodes that check a value in ScratchBuffer.
+
+ If the check fails during S3 resume, the boot script will hang.
+
+ This function may only be called from the client module's
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION, which was passed to
+ QemuFwCfgS3TransferOwnership() as Append.
+
+ @param[in] ScratchData Pointer to the UINT8, UINT16, UINT32 or UINT64 field
+ in ScratchBuffer that should be checked. The caller
+ is responsible for populating the field during S3
+ resume, by calling QemuFwCfgS3ReadBytes() ahead of
+ QemuFwCfgS3CheckValue().
+
+ ScratchData must point into ScratchBuffer, which was
+ allocated, and passed to Append(), by
+ QemuFwCfgS3TransferOwnership().
+
+ ScratchData must be aligned at ValueSize bytes.
+
+ @param[in] ValueSize One of 1, 2, 4 or 8, specifying the size of the field
+ to check.
+
+ @param[in] ValueMask The value read from ScratchData is binarily AND-ed
+ with ValueMask, and the result is compared against
+ Value. If the masked data equals Value, the check
+ passes, and the boot script can proceed. Otherwise,
+ the check fails, and the boot script hangs.
+
+ @param[in] Value Refer to ValueMask.
+
+ @retval RETURN_SUCCESS The opcodes were appended to the ACPI S3
+ Boot Script successfully. There is no way
+ to undo this action.
+
+ @retval RETURN_INVALID_PARAMETER ValueSize is invalid.
+
+ @retval RETURN_INVALID_PARAMETER ValueMask or Value cannot be represented in
+ ValueSize bytes.
+
+ @retval RETURN_INVALID_PARAMETER ScratchData is not aligned at ValueSize
+ bytes.
+
+ @retval RETURN_BAD_BUFFER_SIZE The ValueSize bytes at ScratchData aren't
+ wholly contained in the ScratchBufferSize
+ bytes at ScratchBuffer.
+
+ @return Error codes from underlying functions.
+**/
+EFIAPI
+RETURN_STATUS
+QemuFwCfgS3CheckValue (
+ IN VOID *ScratchData,
+ IN UINT8 ValueSize,
+ IN UINT64 ValueMask,
+ IN UINT64 Value
+ )
+{
+ EFI_BOOT_SCRIPT_WIDTH Width;
+ EFI_STATUS Status;
+
+ ASSERT (mS3SaveState != NULL);
+
+ switch (ValueSize) {
+ case 1:
+ Width = EfiBootScriptWidthUint8;
+ break;
+
+ case 2:
+ Width = EfiBootScriptWidthUint16;
+ break;
+
+ case 4:
+ Width = EfiBootScriptWidthUint32;
+ break;
+
+ case 8:
+ Width = EfiBootScriptWidthUint64;
+ break;
+
+ default:
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ if (ValueSize < 8 &&
+ (RShiftU64 (ValueMask, ValueSize * 8) > 0 ||
+ RShiftU64 (Value, ValueSize * 8) > 0)) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ if ((UINTN)ScratchData % ValueSize > 0) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ if (((UINTN)ScratchData < (UINTN)mScratchBuffer) ||
+ ((UINTN)ScratchData > MAX_UINTN - ValueSize) ||
+ ((UINTN)ScratchData + ValueSize >
+ (UINTN)mScratchBuffer + mScratchBufferSize)) {
+ return RETURN_BAD_BUFFER_SIZE;
+ }
+
+ //
+ // The following opcode will wait "until" (*ScratchData & ValueMask) reads as
+ // Value, considering the least significant ValueSize bytes. As timeout we
+ // use MAX_UINT64 * 100ns, which is approximately 58494 years.
+ //
+ Status = mS3SaveState->Write (
+ mS3SaveState, // This
+ EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
+ Width, // Width
+ (UINT64)(UINTN)ScratchData, // Address
+ (VOID *)&Value, // Data
+ (VOID *)&ValueMask, // DataMask
+ MAX_UINT64 // Delay
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "%a: %a: EFI_BOOT_SCRIPT_MEM_POLL_OPCODE: %r\n",
+ gEfiCallerBaseName, __FUNCTION__, Status));
+ return (RETURN_STATUS)Status;
+ }
+
+ return RETURN_SUCCESS;
+}
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (9 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 10/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for DXE " Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-03-10 10:14 ` Jordan Justen
2017-02-23 1:48 ` [PATCH 12/12] OvmfPkg/AcpiPlatformDxe: " Laszlo Ersek
` (2 subsequent siblings)
13 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
We cannot entirely eliminate the manual boot script building in this
driver, as it also programs lower-level chipset registers (SMI_EN,
GEN_PMCON_1) at S3 resume, not just registers exposed via fw_cfg.
We can nonetheless replace the manually built opcodes for the latter class
of registers with QemuFwCfgS3Lib function calls. We preserve the ordering
between the two sets of registers (low-level chipset first, fw_cfg
second).
This patch demonstrates that manual handling of S3SaveState protocol
installation can be combined with QemuFwCfgS3Lib, even without upsetting
the original order between boot script fragments. An S3SaveState notify
function running at TPL_CALLBACK can safely queue another S3SaveState
notify function at TPL_CALLBACK with QemuFwCfgS3TransferOwnership().
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/SmmControl2Dxe/SmiFeatures.h | 5 +-
OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 224 +++++++-------------
OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 5 +-
3 files changed, 77 insertions(+), 157 deletions(-)
diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.h b/OvmfPkg/SmmControl2Dxe/SmiFeatures.h
index 9d5f1dbcb57e..3f3a5d3ea9b1 100644
--- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.h
+++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.h
@@ -37,13 +37,10 @@ NegotiateSmiFeatures (
/**
Append a boot script fragment that will re-select the previously negotiated
SMI features during S3 resume.
-
- @param[in] S3SaveState The EFI_S3_SAVE_STATE_PROTOCOL instance to append to
- the S3 boot script with.
**/
VOID
SaveSmiFeatures (
- IN EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState
+ VOID
);
#endif
diff --git a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
index bd257f15d955..ecdab2fd9a86 100644
--- a/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
+++ b/OvmfPkg/SmmControl2Dxe/SmiFeatures.c
@@ -18,6 +18,7 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/PcdLib.h>
#include <Library/QemuFwCfgLib.h>
+#include <Library/QemuFwCfgS3Lib.h>
#include "SmiFeatures.h"
@@ -29,29 +30,26 @@
//
// Provides a scratch buffer (allocated in EfiReservedMemoryType type memory)
-// for the S3 boot script fragment to write to and read from. The buffer
-// captures a combined fw_cfg item selection + write command using the DMA
-// access method. Note that we don't trust the runtime OS to preserve the
-// contents of the buffer, the boot script will first rewrite it.
+// for the S3 boot script fragment to write to and read from.
//
#pragma pack (1)
-typedef struct {
- FW_CFG_DMA_ACCESS Access;
- UINT64 Features;
+typedef union {
+ UINT64 Features;
+ UINT8 FeaturesOk;
} SCRATCH_BUFFER;
#pragma pack ()
//
// These carry the selector keys of the "etc/smi/requested-features" and
// "etc/smi/features-ok" fw_cfg files from NegotiateSmiFeatures() to
-// SaveSmiFeatures().
+// AppendFwCfgBootScript().
//
STATIC FIRMWARE_CONFIG_ITEM mRequestedFeaturesItem;
STATIC FIRMWARE_CONFIG_ITEM mFeaturesOkItem;
//
// Carries the negotiated SMI features from NegotiateSmiFeatures() to
-// SaveSmiFeatures().
+// AppendFwCfgBootScript().
//
STATIC UINT64 mSmiFeatures;
@@ -168,157 +166,81 @@ FatalError:
}
/**
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION provided to QemuFwCfgS3Lib.
+**/
+STATIC
+VOID
+EFIAPI
+AppendFwCfgBootScript (
+ IN OUT VOID *Context, OPTIONAL
+ IN OUT VOID *ExternalScratchBuffer
+ )
+{
+ SCRATCH_BUFFER *ScratchBuffer;
+ RETURN_STATUS Status;
+
+ ScratchBuffer = ExternalScratchBuffer;
+
+ //
+ // Write the negotiated feature bitmap into "etc/smi/requested-features".
+ //
+ ScratchBuffer->Features = mSmiFeatures;
+ Status = QemuFwCfgS3WriteBytes (mRequestedFeaturesItem,
+ sizeof ScratchBuffer->Features);
+ if (RETURN_ERROR (Status)) {
+ goto FatalError;
+ }
+
+ //
+ // Read back "etc/smi/features-ok". This invokes the feature validation &
+ // lockdown. (The validation succeeded at first boot.)
+ //
+ Status = QemuFwCfgS3ReadBytes (mFeaturesOkItem,
+ sizeof ScratchBuffer->FeaturesOk);
+ if (RETURN_ERROR (Status)) {
+ goto FatalError;
+ }
+
+ //
+ // If "etc/smi/features-ok" read as 1, we're good. Otherwise, hang the S3
+ // resume process.
+ //
+ Status = QemuFwCfgS3CheckValue (&ScratchBuffer->FeaturesOk,
+ sizeof ScratchBuffer->FeaturesOk, MAX_UINT8, 1);
+ if (RETURN_ERROR (Status)) {
+ goto FatalError;
+ }
+
+ DEBUG ((DEBUG_VERBOSE, "%a: SMI feature negotiation boot script saved\n",
+ __FUNCTION__));
+ return;
+
+FatalError:
+ ASSERT (FALSE);
+ CpuDeadLoop ();
+}
+
+
+/**
Append a boot script fragment that will re-select the previously negotiated
SMI features during S3 resume.
-
- @param[in] S3SaveState The EFI_S3_SAVE_STATE_PROTOCOL instance to append to
- the S3 boot script with.
**/
VOID
SaveSmiFeatures (
- IN EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState
+ VOID
)
{
- SCRATCH_BUFFER *ScratchBuffer;
- EFI_STATUS Status;
- UINT64 AccessAddress;
- UINT32 ControlPollData;
- UINT32 ControlPollMask;
- UINT16 FeaturesOkItemAsUint16;
- UINT8 FeaturesOkData;
- UINT8 FeaturesOkMask;
+ RETURN_STATUS Status;
- ScratchBuffer = AllocateReservedPool (sizeof *ScratchBuffer);
- if (ScratchBuffer == NULL) {
- DEBUG ((DEBUG_ERROR, "%a: scratch buffer allocation failed\n",
- __FUNCTION__));
- goto FatalError;
- }
-
- //
- // Populate the scratch buffer with a select + write fw_cfg DMA command that
- // will write the negotiated feature bitmap into
- // "etc/smi/requested-features".
- //
- ScratchBuffer->Access.Control = SwapBytes32 (
- (UINT32)mRequestedFeaturesItem << 16 |
- FW_CFG_DMA_CTL_SELECT |
- FW_CFG_DMA_CTL_WRITE
- );
- ScratchBuffer->Access.Length = SwapBytes32 (
- (UINT32)sizeof ScratchBuffer->Features);
- ScratchBuffer->Access.Address = SwapBytes64 (
- (UINTN)&ScratchBuffer->Features);
- ScratchBuffer->Features = mSmiFeatures;
-
- //
- // Copy the scratch buffer into the boot script. When replayed, this
- // EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE will restore the current contents of the
- // scratch buffer, in-place.
//
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint8, // Width
- (UINT64)(UINTN)ScratchBuffer, // Address
- sizeof *ScratchBuffer, // Count
- (VOID*)ScratchBuffer // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
- }
-
- //
- // Append an opcode that will write the address of the scratch buffer to the
- // fw_cfg DMA address register, which consists of two 32-bit IO ports. The
- // second (highest address, least significant) write will start the transfer.
+ // We are already running at TPL_CALLBACK, on the stack of
+ // OnS3SaveStateInstalled(). But that's okay, we can easily queue more
+ // notification functions while executing a notification function.
//
- AccessAddress = SwapBytes64 ((UINTN)&ScratchBuffer->Access);
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint32, // Width
- (UINT64)FW_CFG_IO_DMA_ADDRESS, // Address
- (UINTN)2, // Count
- &AccessAddress // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
+ Status = QemuFwCfgS3TransferOwnership (AppendFwCfgBootScript, NULL,
+ sizeof (SCRATCH_BUFFER));
+ if (RETURN_ERROR (Status)) {
+ ASSERT (FALSE);
+ CpuDeadLoop ();
}
-
- //
- // The EFI_BOOT_SCRIPT_MEM_POLL_OPCODE will wait until the Control word reads
- // as zero (transfer complete). As timeout we use MAX_UINT64 * 100ns, which
- // is approximately 58494 years.
- //
- ControlPollData = 0;
- ControlPollMask = MAX_UINT32;
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
- EfiBootScriptWidthUint32, // Width
- (UINT64)(UINTN)&ScratchBuffer->Access.Control, // Address
- &ControlPollData, // Data
- &ControlPollMask, // DataMask
- MAX_UINT64 // Delay
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_MEM_POLL_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
- }
-
- //
- // Select the "etc/smi/features-ok" fw_cfg file, which invokes the feature
- // validation & lockdown. (The validation succeeded at first boot.)
- //
- FeaturesOkItemAsUint16 = (UINT16)mFeaturesOkItem;
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint16, // Width
- (UINT64)FW_CFG_IO_SELECTOR, // Address
- (UINTN)1, // Count
- &FeaturesOkItemAsUint16 // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_WRITE_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
- }
-
- //
- // Read the contents (one byte) of "etc/smi/features-ok". If the value is
- // one, we're good. Otherwise, continue reading the data port: QEMU returns 0
- // past the end of the fw_cfg item, so this will hang the resume process,
- // which matches our intent.
- //
- FeaturesOkData = 1;
- FeaturesOkMask = MAX_UINT8;
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_IO_POLL_OPCODE, // OpCode
- EfiBootScriptWidthUint8, // Width
- (UINT64)(UINTN)FW_CFG_IO_DATA, // Address
- &FeaturesOkData, // Data
- &FeaturesOkMask, // DataMask
- MAX_UINT64 // Delay
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a:%d: EFI_BOOT_SCRIPT_IO_POLL_OPCODE: %r\n",
- __FUNCTION__, __LINE__, Status));
- goto FatalError;
- }
-
- DEBUG ((DEBUG_VERBOSE, "%a: ScratchBuffer@%p\n", __FUNCTION__,
- (VOID *)ScratchBuffer));
- return;
-
-FatalError:
- ASSERT (FALSE);
- CpuDeadLoop ();
}
diff --git a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
index bb79fce0855b..e1cd3d02ac36 100644
--- a/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
+++ b/OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c
@@ -378,14 +378,15 @@ OnS3SaveStateInstalled (
CpuDeadLoop ();
}
+ DEBUG ((DEBUG_VERBOSE, "%a: chipset boot script saved\n", __FUNCTION__));
+
//
// Append a boot script fragment that re-selects the negotiated SMI features.
//
if (mSmiFeatureNegotiation) {
- SaveSmiFeatures (S3SaveState);
+ SaveSmiFeatures ();
}
- DEBUG ((EFI_D_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__));
gBS->CloseEvent (Event);
mS3SaveStateInstalled = NULL;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-02-23 1:48 ` [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib Laszlo Ersek
@ 2017-03-10 10:14 ` Jordan Justen
2017-03-10 19:36 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Jordan Justen @ 2017-03-10 10:14 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
On 2017-02-22 17:48:13, Laszlo Ersek wrote:
> + //
> + ScratchBuffer->Features = mSmiFeatures;
> + Status = QemuFwCfgS3WriteBytes (mRequestedFeaturesItem,
I noticed a fair amount of function calls where the first args are
appearing on the first line, and then the rest appear on subsequent
lines indented two spaces.
> + sizeof ScratchBuffer->Features);
> + if (RETURN_ERROR (Status)) {
> + goto FatalError;
> + }
> +
I can't say that the coding style prohibits this, but it does seem odd
looking. I would expect to see:
[1]
Status = QemuFwCfgS3WriteBytes (
mRequestedFeaturesItem,
sizeof ScratchBuffer->Features
);
The coding style examples look like this:
Status = QemuFwCfgS3WriteBytes (
mRequestedFeaturesItem,
sizeof ScratchBuffer->Features
);
But this looks less like what I'm used to actually seeing in EDK II.
(Look at DXE Core, for example. It looks like [1].)
I personally think this is okay to save a line, but it doesn't seem to
follow the coding style doc:
Status = QemuFwCfgS3WriteBytes (
mRequestedFeaturesItem,
sizeof ScratchBuffer->Features);
Now, we all know EDK II has a style of it's own, but outide EDK II, I
might expect something like:
Status = QemuFwCfgS3WriteBytes(mRequestedFeaturesItem,
sizeof ScratchBuffer->Features);
Which once again shows arguements on subsequent lines are lined up.
Based on that, I think [1] is the best style for EDK II code.
-Jordan
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-03-10 10:14 ` Jordan Justen
@ 2017-03-10 19:36 ` Laszlo Ersek
2017-03-13 21:32 ` Jordan Justen
0 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2017-03-10 19:36 UTC (permalink / raw)
To: Jordan Justen; +Cc: edk2-devel-01
On 03/10/17 11:14, Jordan Justen wrote:
> On 2017-02-22 17:48:13, Laszlo Ersek wrote:
>> + //
>> + ScratchBuffer->Features = mSmiFeatures;
>> + Status = QemuFwCfgS3WriteBytes (mRequestedFeaturesItem,
>
> I noticed a fair amount of function calls where the first args are
> appearing on the first line, and then the rest appear on subsequent
> lines indented two spaces.
Yes, that's intentional. The coding style nominally requires each argument on a separate line (if they don't all fit on a single line), but that wastes a huge amount of vertical space (which is bad because less code fits in a screenful). So I frequently follow a semi-compressed style where I place the arguments continuously, but wherever I have to break the line (because of the 80 char limit) I stick with the indentation required by the coding style. Also, when I apply this style, I don't break the final closing paren off to a separate line (because this style is primarily contiguous).
I mostly employ this style when the arguments are otherwise simple (i.e., when the "one arg per line" style does not improve clarity, just wastes a lot of space).
This is not new, I've been coding like this in edk2 virtually forever.
>
>> + sizeof ScratchBuffer->Features);
>> + if (RETURN_ERROR (Status)) {
>> + goto FatalError;
>> + }
>> +
>
> I can't say that the coding style prohibits this, but it does seem odd
> looking. I would expect to see:
>
> [1]
>
> Status = QemuFwCfgS3WriteBytes (
> mRequestedFeaturesItem,
> sizeof ScratchBuffer->Features
> );
>
> The coding style examples look like this:
>
> Status = QemuFwCfgS3WriteBytes (
> mRequestedFeaturesItem,
> sizeof ScratchBuffer->Features
> );
>
> But this looks less like what I'm used to actually seeing in EDK II.
> (Look at DXE Core, for example. It looks like [1].)
>
> I personally think this is okay to save a line, but it doesn't seem to
> follow the coding style doc:
>
> Status = QemuFwCfgS3WriteBytes (
> mRequestedFeaturesItem,
> sizeof ScratchBuffer->Features);
>
> Now, we all know EDK II has a style of it's own, but outide EDK II, I
> might expect something like:
>
> Status = QemuFwCfgS3WriteBytes(mRequestedFeaturesItem,
> sizeof ScratchBuffer->Features);
>
> Which once again shows arguements on subsequent lines are lined up.
> Based on that, I think [1] is the best style for EDK II code.
Yes, [1] is the official style, and I stick with that when the arguments are complex or long, or require separate comments.
However, as I said above, when the arguments are simple, it makes sense to place them contiguously (together with the final paren), while preserving the edk2 indentation.
Again, I've been doing this for years, consistently, and you've never seemed to take issue with it. For example, some snippets from the virtio block driver:
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 290) //
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 291) // virtio-blk header in first desc
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 292) //
7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 293) VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 294) VRING_DESC_F_NEXT, &Indices);
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 310) //
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 311) // VRING_DESC_F_WRITE is interpreted from the host's point of view.
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 312) //
7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 313) VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize,
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 314) VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE),
e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 315) &Indices);
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 316) }
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 318) //
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 319) // host status in last (second or third) desc
fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 320) //
7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 321) VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus,
e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 322) VRING_DESC_F_WRITE, &Indices);
6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 674) Status = VIRTIO_CFG_READ (Dev, Topology.PhysicalBlockExp,
6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 675) &PhysicalBlockExp);
More recent code from e.g. "SmmAccessPei.c":
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 186) return SmramAccessGetCapabilities (This->LockState, This->OpenState,
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 187) SmramMapSize, SmramMap);
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 267) DEBUG ((EFI_D_ERROR, "%a: no SMRAM with host bridge DID=0x%04x; only "
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 268) "DID=0x%04x (Q35) is supported\n", __FUNCTION__, HostBridgeDevId,
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 269) INTEL_Q35_MCH_DEVICE_ID));
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 309) //
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 310) // Given the zero graphics memory sizes configured above, set the
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 311) // graphics-related stolen memory bases to the same as TOLUD.
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 312) //
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 313) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_GBSM),
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 314) TopOfLowRamMb << MCH_GBSM_MB_SHIFT);
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 315) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_BGSM),
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 316) TopOfLowRamMb << MCH_BGSM_MB_SHIFT);
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 348) Status = SmramAccessGetCapabilities (mAccess.LockState, mAccess.OpenState,
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 349) &SmramMapSize, SmramMap);
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 376) CopyMem (GuidHob, &SmramMap[DescIdxSmmS3ResumeState],
9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 377) sizeof SmramMap[DescIdxSmmS3ResumeState]);
Brand new code from "OvmfPkg/AcpiPlatformDxe/BootScript.c":
df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 107) Context->WritePointers = AllocatePool (WritePointerCount *
df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 108) sizeof *Context->WritePointers);
df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 191) DEBUG ((DEBUG_VERBOSE, "%a: 0x%04x/[0x%08x+%d] := 0x%Lx (%Lu)\n",
df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 192) __FUNCTION__, PointerItem, PointerOffset, PointerSize, PointerValue,
df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 193) (UINT64)S3Context->Used));
df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 247) Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid,
df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 248) NULL /* Registration */, (VOID **)&S3SaveState);
I've been entirely consistent about this.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-03-10 19:36 ` Laszlo Ersek
@ 2017-03-13 21:32 ` Jordan Justen
2017-03-13 22:12 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Jordan Justen @ 2017-03-13 21:32 UTC (permalink / raw)
To: Laszlo Ersek, Kinney, Michael D; +Cc: edk2-devel-01
On 2017-03-10 11:36:36, Laszlo Ersek wrote:
> On 03/10/17 11:14, Jordan Justen wrote:
> > On 2017-02-22 17:48:13, Laszlo Ersek wrote:
> >> + //
> >> + ScratchBuffer->Features = mSmiFeatures;
> >> + Status = QemuFwCfgS3WriteBytes (mRequestedFeaturesItem,
> >
> > I noticed a fair amount of function calls where the first args are
> > appearing on the first line, and then the rest appear on subsequent
> > lines indented two spaces.
>
> Yes, that's intentional. The coding style nominally requires each
> argument on a separate line (if they don't all fit on a single
> line), but that wastes a huge amount of vertical space (which is bad
> because less code fits in a screenful). So I frequently follow a
> semi-compressed style where I place the arguments continuously, but
> wherever I have to break the line (because of the 80 char limit) I
> stick with the indentation required by the coding style. Also, when
> I apply this style, I don't break the final closing paren off to a
> separate line (because this style is primarily contiguous).
>
> I mostly employ this style when the arguments are otherwise simple
> (i.e., when the "one arg per line" style does not improve clarity,
> just wastes a lot of space).
>
> This is not new, I've been coding like this in edk2 virtually
> forever.
>
I understand that I've given an r-b in the past for code like this. I
have noticed it previously, but in some cases it doesn't look as
strange. For example, with the DEBUG macro, the error level seems less
important to move to the next line. I guess it is probably bad to make
an exception there.
I do think that lining up the arguments should be prioritized over
saving vertical whitespace. I think we just have to accept that EDK II
is not efficient in terms of vertical space. In fact, EDK II is just
plain verbose in general. :)
At least we don't follow the kernel's strict 8 char indent and 80
columns limit. (Although, there are some good arguments in favor of
this in terms of forcing code to be broken down into smaller
functions.)
I would like to know if Mike has any opinion on whether this is a bug
in the coding style spec to not address this more definitively.
-Jordan
> >
> >> + sizeof ScratchBuffer->Features);
> >> + if (RETURN_ERROR (Status)) {
> >> + goto FatalError;
> >> + }
> >> +
> >
> > I can't say that the coding style prohibits this, but it does seem odd
> > looking. I would expect to see:
> >
> > [1]
> >
> > Status = QemuFwCfgS3WriteBytes (
> > mRequestedFeaturesItem,
> > sizeof ScratchBuffer->Features
> > );
> >
> > The coding style examples look like this:
> >
> > Status = QemuFwCfgS3WriteBytes (
> > mRequestedFeaturesItem,
> > sizeof ScratchBuffer->Features
> > );
> >
> > But this looks less like what I'm used to actually seeing in EDK II.
> > (Look at DXE Core, for example. It looks like [1].)
> >
> > I personally think this is okay to save a line, but it doesn't seem to
> > follow the coding style doc:
> >
> > Status = QemuFwCfgS3WriteBytes (
> > mRequestedFeaturesItem,
> > sizeof ScratchBuffer->Features);
> >
> > Now, we all know EDK II has a style of it's own, but outide EDK II, I
> > might expect something like:
> >
> > Status = QemuFwCfgS3WriteBytes(mRequestedFeaturesItem,
> > sizeof ScratchBuffer->Features);
> >
> > Which once again shows arguements on subsequent lines are lined up.
> > Based on that, I think [1] is the best style for EDK II code.
>
> Yes, [1] is the official style, and I stick with that when the arguments are complex or long, or require separate comments.
>
> However, as I said above, when the arguments are simple, it makes sense to place them contiguously (together with the final paren), while preserving the edk2 indentation.
>
> Again, I've been doing this for years, consistently, and you've never seemed to take issue with it. For example, some snippets from the virtio block driver:
>
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 290) //
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 291) // virtio-blk header in first desc
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 292) //
> 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 293) VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
> e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 294) VRING_DESC_F_NEXT, &Indices);
>
>
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 310) //
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 311) // VRING_DESC_F_WRITE is interpreted from the host's point of view.
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 312) //
> 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 313) VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize,
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 314) VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE),
> e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 315) &Indices);
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 316) }
>
>
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 318) //
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 319) // host status in last (second or third) desc
> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 320) //
> 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 321) VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus,
> e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 322) VRING_DESC_F_WRITE, &Indices);
>
>
> 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 674) Status = VIRTIO_CFG_READ (Dev, Topology.PhysicalBlockExp,
> 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 675) &PhysicalBlockExp);
>
>
> More recent code from e.g. "SmmAccessPei.c":
>
>
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 186) return SmramAccessGetCapabilities (This->LockState, This->OpenState,
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 187) SmramMapSize, SmramMap);
>
>
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 267) DEBUG ((EFI_D_ERROR, "%a: no SMRAM with host bridge DID=0x%04x; only "
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 268) "DID=0x%04x (Q35) is supported\n", __FUNCTION__, HostBridgeDevId,
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 269) INTEL_Q35_MCH_DEVICE_ID));
>
>
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 309) //
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 310) // Given the zero graphics memory sizes configured above, set the
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 311) // graphics-related stolen memory bases to the same as TOLUD.
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 312) //
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 313) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_GBSM),
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 314) TopOfLowRamMb << MCH_GBSM_MB_SHIFT);
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 315) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_BGSM),
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 316) TopOfLowRamMb << MCH_BGSM_MB_SHIFT);
>
>
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 348) Status = SmramAccessGetCapabilities (mAccess.LockState, mAccess.OpenState,
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 349) &SmramMapSize, SmramMap);
>
>
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 376) CopyMem (GuidHob, &SmramMap[DescIdxSmmS3ResumeState],
> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 377) sizeof SmramMap[DescIdxSmmS3ResumeState]);
>
>
> Brand new code from "OvmfPkg/AcpiPlatformDxe/BootScript.c":
>
>
> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 107) Context->WritePointers = AllocatePool (WritePointerCount *
> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 108) sizeof *Context->WritePointers);
>
>
> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 191) DEBUG ((DEBUG_VERBOSE, "%a: 0x%04x/[0x%08x+%d] := 0x%Lx (%Lu)\n",
> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 192) __FUNCTION__, PointerItem, PointerOffset, PointerSize, PointerValue,
> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 193) (UINT64)S3Context->Used));
>
>
> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 247) Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid,
> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 248) NULL /* Registration */, (VOID **)&S3SaveState);
>
> I've been entirely consistent about this.
>
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-03-13 21:32 ` Jordan Justen
@ 2017-03-13 22:12 ` Laszlo Ersek
2017-03-13 22:43 ` Jordan Justen
0 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2017-03-13 22:12 UTC (permalink / raw)
To: Jordan Justen, Kinney, Michael D; +Cc: edk2-devel-01
On 03/13/17 22:32, Jordan Justen wrote:
> On 2017-03-10 11:36:36, Laszlo Ersek wrote:
>> On 03/10/17 11:14, Jordan Justen wrote:
>>> On 2017-02-22 17:48:13, Laszlo Ersek wrote:
>>>> + //
>>>> + ScratchBuffer->Features = mSmiFeatures;
>>>> + Status = QemuFwCfgS3WriteBytes (mRequestedFeaturesItem,
>>>
>>> I noticed a fair amount of function calls where the first args are
>>> appearing on the first line, and then the rest appear on subsequent
>>> lines indented two spaces.
>>
>> Yes, that's intentional. The coding style nominally requires each
>> argument on a separate line (if they don't all fit on a single
>> line), but that wastes a huge amount of vertical space (which is bad
>> because less code fits in a screenful). So I frequently follow a
>> semi-compressed style where I place the arguments continuously, but
>> wherever I have to break the line (because of the 80 char limit) I
>> stick with the indentation required by the coding style. Also, when
>> I apply this style, I don't break the final closing paren off to a
>> separate line (because this style is primarily contiguous).
>>
>> I mostly employ this style when the arguments are otherwise simple
>> (i.e., when the "one arg per line" style does not improve clarity,
>> just wastes a lot of space).
>>
>> This is not new, I've been coding like this in edk2 virtually
>> forever.
>>
>
> I understand that I've given an r-b in the past for code like this. I
> have noticed it previously, but in some cases it doesn't look as
> strange. For example, with the DEBUG macro, the error level seems less
> important to move to the next line. I guess it is probably bad to make
> an exception there.
>
> I do think that lining up the arguments should be prioritized over
> saving vertical whitespace. I think we just have to accept that EDK II
> is not efficient in terms of vertical space. In fact, EDK II is just
> plain verbose in general. :)
>
> At least we don't follow the kernel's strict 8 char indent and 80
> columns limit. (Although, there are some good arguments in favor of
> this in terms of forcing code to be broken down into smaller
> functions.)
>
> I would like to know if Mike has any opinion on whether this is a bug
> in the coding style spec to not address this more definitively.
"CCS_2_1_Draft.pdf" seems to be the most recent version; in it, I find:
(1)
5.2.2.4 Subsequent lines of multi-line function calls should line up
one or two tab-stops from the beginning of the function name
Use either one or two tab stops to ensure that each parameter
is indented at least two spaces after the function name. Either
of the below examples is acceptable:
Status = gBS->AllocatePool (
EfiBootServicesData,
sizeof (DRIVER_NAME_INSTANCE),
&PrivateData
);
Status = gBS->AllocatePool (
EfiBootServicesData,
sizeof (DRIVER_NAME_INSTANCE),
&PrivateData
);
My notes on this:
- my code complies with the indentation requirement
- the passage is silent on whether each argument should be broken off
to a new line. The examples are called "acceptable", not "required".
I guess this is what your question to Mike is about.
- I don't see what the difference is between the two examples. To me
they look identical.
- In the examples, the closing parens line up with "AllocatePool", not
with "EfiBootServicesData". I think I have never ever seen this style
in edk2.
(2)
6.5.2.4 Comments are allowed on the parameters of a function call.
These comments provide supplemental information about the
parameters for that specific function call. The information in
parameter comments should not repeat the information in the
descriptive text for the @param entries in the special
documentation block describing the function. Function call
parameter comments are never Doxygen comments.
Status = TestString (
String, // Comment for first parameter
Index + 3, // Comment for second parameter
&Value // Comment for third parameter
);
My notes on this:
- the closing paren's indentation is inconsistent with the examples
given under 5.2.2.4 (but consistent with edk2 practice).
If the agreement is to break every single argument in a multi-line
function call (or function-like macro call) off to a separate line, then
please let us file a BZ for that (EDK2/Documentation I guess?), and then
I'll comply with that *new* requirement in my patches that I post
*after* the BZ is filed.
I see a number of open items for FDF/INF/etc specs:
https://bugzilla.tianocore.org/buglist.cgi?component=Documentation&product=EDK2&query_format=advanced
so I think the TianoCore bugzilla is the right place to track coding
style spec reports as well.
Thanks
Laszlo
>
> -Jordan
>
>>>
>>>> + sizeof ScratchBuffer->Features);
>>>> + if (RETURN_ERROR (Status)) {
>>>> + goto FatalError;
>>>> + }
>>>> +
>>>
>>> I can't say that the coding style prohibits this, but it does seem odd
>>> looking. I would expect to see:
>>>
>>> [1]
>>>
>>> Status = QemuFwCfgS3WriteBytes (
>>> mRequestedFeaturesItem,
>>> sizeof ScratchBuffer->Features
>>> );
>>>
>>> The coding style examples look like this:
>>>
>>> Status = QemuFwCfgS3WriteBytes (
>>> mRequestedFeaturesItem,
>>> sizeof ScratchBuffer->Features
>>> );
>>>
>>> But this looks less like what I'm used to actually seeing in EDK II.
>>> (Look at DXE Core, for example. It looks like [1].)
>>>
>>> I personally think this is okay to save a line, but it doesn't seem to
>>> follow the coding style doc:
>>>
>>> Status = QemuFwCfgS3WriteBytes (
>>> mRequestedFeaturesItem,
>>> sizeof ScratchBuffer->Features);
>>>
>>> Now, we all know EDK II has a style of it's own, but outide EDK II, I
>>> might expect something like:
>>>
>>> Status = QemuFwCfgS3WriteBytes(mRequestedFeaturesItem,
>>> sizeof ScratchBuffer->Features);
>>>
>>> Which once again shows arguements on subsequent lines are lined up.
>>> Based on that, I think [1] is the best style for EDK II code.
>>
>> Yes, [1] is the official style, and I stick with that when the arguments are complex or long, or require separate comments.
>>
>> However, as I said above, when the arguments are simple, it makes sense to place them contiguously (together with the final paren), while preserving the edk2 indentation.
>>
>> Again, I've been doing this for years, consistently, and you've never seemed to take issue with it. For example, some snippets from the virtio block driver:
>>
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 290) //
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 291) // virtio-blk header in first desc
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 292) //
>> 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 293) VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
>> e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 294) VRING_DESC_F_NEXT, &Indices);
>>
>>
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 310) //
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 311) // VRING_DESC_F_WRITE is interpreted from the host's point of view.
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 312) //
>> 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 313) VirtioAppendDesc (&Dev->Ring, (UINTN) Buffer, (UINT32) BufferSize,
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 314) VRING_DESC_F_NEXT | (RequestIsWrite ? 0 : VRING_DESC_F_WRITE),
>> e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 315) &Indices);
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 316) }
>>
>>
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 318) //
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 319) // host status in last (second or third) desc
>> fd51d75991731 (jljusten 2012-10-08 07:32:59 +0000 320) //
>> 7fcacd6c92616 (jljusten 2012-10-12 18:54:17 +0000 321) VirtioAppendDesc (&Dev->Ring, (UINTN) &HostStatus, sizeof HostStatus,
>> e371e7e545b26 (jljusten 2012-10-12 18:54:35 +0000 322) VRING_DESC_F_WRITE, &Indices);
>>
>>
>> 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 674) Status = VIRTIO_CFG_READ (Dev, Topology.PhysicalBlockExp,
>> 6476804e3cd2e (Laszlo Ersek 2013-12-18 19:57:46 +0000 675) &PhysicalBlockExp);
>>
>>
>> More recent code from e.g. "SmmAccessPei.c":
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 186) return SmramAccessGetCapabilities (This->LockState, This->OpenState,
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 187) SmramMapSize, SmramMap);
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 267) DEBUG ((EFI_D_ERROR, "%a: no SMRAM with host bridge DID=0x%04x; only "
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 268) "DID=0x%04x (Q35) is supported\n", __FUNCTION__, HostBridgeDevId,
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 269) INTEL_Q35_MCH_DEVICE_ID));
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 309) //
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 310) // Given the zero graphics memory sizes configured above, set the
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 311) // graphics-related stolen memory bases to the same as TOLUD.
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 312) //
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 313) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_GBSM),
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 314) TopOfLowRamMb << MCH_GBSM_MB_SHIFT);
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 315) PciWrite32 (DRAMC_REGISTER_Q35 (MCH_BGSM),
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 316) TopOfLowRamMb << MCH_BGSM_MB_SHIFT);
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 348) Status = SmramAccessGetCapabilities (mAccess.LockState, mAccess.OpenState,
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 349) &SmramMapSize, SmramMap);
>>
>>
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 376) CopyMem (GuidHob, &SmramMap[DescIdxSmmS3ResumeState],
>> 9d560947f6d35 (Laszlo Ersek 2015-11-30 18:41:38 +0000 377) sizeof SmramMap[DescIdxSmmS3ResumeState]);
>>
>>
>> Brand new code from "OvmfPkg/AcpiPlatformDxe/BootScript.c":
>>
>>
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 107) Context->WritePointers = AllocatePool (WritePointerCount *
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 108) sizeof *Context->WritePointers);
>>
>>
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 191) DEBUG ((DEBUG_VERBOSE, "%a: 0x%04x/[0x%08x+%d] := 0x%Lx (%Lu)\n",
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 192) __FUNCTION__, PointerItem, PointerOffset, PointerSize, PointerValue,
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 193) (UINT64)S3Context->Used));
>>
>>
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 247) Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid,
>> df73df138d9d5 (Laszlo Ersek 2017-02-09 17:32:40 +0100 248) NULL /* Registration */, (VOID **)&S3SaveState);
>>
>> I've been entirely consistent about this.
>>
>> Thanks
>> Laszlo
>> _______________________________________________
>> 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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-03-13 22:12 ` Laszlo Ersek
@ 2017-03-13 22:43 ` Jordan Justen
2017-03-14 1:58 ` Kinney, Michael D
0 siblings, 1 reply; 32+ messages in thread
From: Jordan Justen @ 2017-03-13 22:43 UTC (permalink / raw)
To: Kinney, Michael D, Laszlo Ersek; +Cc: edk2-devel-01
On 2017-03-13 15:12:08, Laszlo Ersek wrote:
>
> "CCS_2_1_Draft.pdf" seems to be the most recent version; in it, I find:
>
> (1)
>
> 5.2.2.4 Subsequent lines of multi-line function calls should line up
> one or two tab-stops from the beginning of the function name
>
> Use either one or two tab stops to ensure that each parameter
> is indented at least two spaces after the function name. Either
> of the below examples is acceptable:
>
> Status = gBS->AllocatePool (
> EfiBootServicesData,
> sizeof (DRIVER_NAME_INSTANCE),
> &PrivateData
> );
>
> Status = gBS->AllocatePool (
> EfiBootServicesData,
> sizeof (DRIVER_NAME_INSTANCE),
> &PrivateData
> );
>
> My notes on this:
>
> - my code complies with the indentation requirement
>
> - the passage is silent on whether each argument should be broken off
> to a new line. The examples are called "acceptable", not "required".
> I guess this is what your question to Mike is about.
>
Right. It seems like this is a case where the text perhaps could be
more clear. Just looking at the precedence in the tree, it seems
likely that we might want to say that if the call requires multiple
lines, then the first parameter should be indented on the next line.
> - I don't see what the difference is between the two examples. To me
> they look identical.
I think this example formatting got messed up. I saw a previous
version that varied very subtlely.
Status = gBS->AllocatePool (
EfiBootServicesData,
sizeof (DRIVER_NAME_INSTANCE),
&PrivateData
);
Status = gBS->AllocatePool (
EfiBootServicesData,
sizeof (DRIVER_NAME_INSTANCE),
&PrivateData
);
I always thought the rule was 2 spaces indenting beyond the function
name.
These example make me think that the intension is that your tab key
might only want to align to columns that are even numbered, so you
might have the result be that the indent from the function name would
be 3 spaces.
I don't know... I prefer the rule that you indent 2 spaces from the
function name. :)
> - In the examples, the closing parens line up with "AllocatePool", not
> with "EfiBootServicesData". I think I have never ever seen this style
> in edk2.
>
Yeah. Me either. This is confusing.
-Jordan
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-03-13 22:43 ` Jordan Justen
@ 2017-03-14 1:58 ` Kinney, Michael D
2017-03-14 13:48 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Kinney, Michael D @ 2017-03-14 1:58 UTC (permalink / raw)
To: Justen, Jordan L, Laszlo Ersek, Kinney, Michael D; +Cc: edk2-devel-01
Laszlo,
I agree that there are some inconsistencies in the edk2 C source files.
For this specific example, I prefer what Jordan recommended:
> > Status = QemuFwCfgS3WriteBytes (
> > mRequestedFeaturesItem,
> > sizeof ScratchBuffer->Features
> > );
When multi-line:
* One argument per line
* Indent each argument 2 spaces from start of function name
* Close parenthesis aligned with start of arguments
I am actively working on adding the EDK II C Coding standard
and other EDK II docs to gitbooks. That will be a good place
to discuss and clarify or correct the EDK II C coding standard.
Given inconsistencies in some of the edk2 C source files and
some clarification needed in EDK II C Coding Standard, I would
not require any changes here. I would prefer everyone follow
this specific recommendation if possible.
Hopefully we can expand the PatchCheck tool over time to help
check code against the EDK II C Coding Standard.
Mike
> -----Original Message-----
> From: Justen, Jordan L
> Sent: Monday, March 13, 2017 3:44 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>
> Subject: Re: [edk2] [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with
> QemuFwCfgS3Lib
>
> On 2017-03-13 15:12:08, Laszlo Ersek wrote:
> >
> > "CCS_2_1_Draft.pdf" seems to be the most recent version; in it, I find:
> >
> > (1)
> >
> > 5.2.2.4 Subsequent lines of multi-line function calls should line up
> > one or two tab-stops from the beginning of the function name
> >
> > Use either one or two tab stops to ensure that each parameter
> > is indented at least two spaces after the function name. Either
> > of the below examples is acceptable:
> >
> > Status = gBS->AllocatePool (
> > EfiBootServicesData,
> > sizeof (DRIVER_NAME_INSTANCE),
> > &PrivateData
> > );
> >
> > Status = gBS->AllocatePool (
> > EfiBootServicesData,
> > sizeof (DRIVER_NAME_INSTANCE),
> > &PrivateData
> > );
> >
> > My notes on this:
> >
> > - my code complies with the indentation requirement
> >
> > - the passage is silent on whether each argument should be broken off
> > to a new line. The examples are called "acceptable", not "required".
> > I guess this is what your question to Mike is about.
> >
>
> Right. It seems like this is a case where the text perhaps could be
> more clear. Just looking at the precedence in the tree, it seems
> likely that we might want to say that if the call requires multiple
> lines, then the first parameter should be indented on the next line.
>
> > - I don't see what the difference is between the two examples. To me
> > they look identical.
>
> I think this example formatting got messed up. I saw a previous
> version that varied very subtlely.
>
> Status = gBS->AllocatePool (
> EfiBootServicesData,
> sizeof (DRIVER_NAME_INSTANCE),
> &PrivateData
> );
>
> Status = gBS->AllocatePool (
> EfiBootServicesData,
> sizeof (DRIVER_NAME_INSTANCE),
> &PrivateData
> );
>
> I always thought the rule was 2 spaces indenting beyond the function
> name.
>
> These example make me think that the intension is that your tab key
> might only want to align to columns that are even numbered, so you
> might have the result be that the indent from the function name would
> be 3 spaces.
>
> I don't know... I prefer the rule that you indent 2 spaces from the
> function name. :)
>
> > - In the examples, the closing parens line up with "AllocatePool", not
> > with "EfiBootServicesData". I think I have never ever seen this style
> > in edk2.
> >
>
> Yeah. Me either. This is confusing.
>
> -Jordan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-03-14 1:58 ` Kinney, Michael D
@ 2017-03-14 13:48 ` Laszlo Ersek
2017-03-14 20:38 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2017-03-14 13:48 UTC (permalink / raw)
To: Kinney, Michael D, Justen, Jordan L; +Cc: edk2-devel-01
On 03/14/17 02:58, Kinney, Michael D wrote:
> Laszlo,
>
> I agree that there are some inconsistencies in the edk2 C source files.
>
> For this specific example, I prefer what Jordan recommended:
>
>>> Status = QemuFwCfgS3WriteBytes (
>>> mRequestedFeaturesItem,
>>> sizeof ScratchBuffer->Features
>>> );
>
> When multi-line:
> * One argument per line
> * Indent each argument 2 spaces from start of function name
> * Close parenthesis aligned with start of arguments
>
> I am actively working on adding the EDK II C Coding standard
> and other EDK II docs to gitbooks. That will be a good place
> to discuss and clarify or correct the EDK II C coding standard.
>
> Given inconsistencies in some of the edk2 C source files and
> some clarification needed in EDK II C Coding Standard, I would
> not require any changes here.
If I understand correctly, that means v2 of this set should be acceptable.
If that's right, can you please R-b this set, Jordan?
> I would prefer everyone follow
> this specific recommendation if possible.
I can do that, in the future, if we codify the preference in some
written form, even before the CCS is updated and/or imported to
gitbooks. Should we file a TianoCore BZ for it? I can do that, if you
prefer.
Thanks!
Laszlo
> Hopefully we can expand the PatchCheck tool over time to help
> check code against the EDK II C Coding Standard.
>
> Mike
>
>
>> -----Original Message-----
>> From: Justen, Jordan L
>> Sent: Monday, March 13, 2017 3:44 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>
>> Cc: edk2-devel-01 <edk2-devel@lists.01.org>
>> Subject: Re: [edk2] [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with
>> QemuFwCfgS3Lib
>>
>> On 2017-03-13 15:12:08, Laszlo Ersek wrote:
>>>
>>> "CCS_2_1_Draft.pdf" seems to be the most recent version; in it, I find:
>>>
>>> (1)
>>>
>>> 5.2.2.4 Subsequent lines of multi-line function calls should line up
>>> one or two tab-stops from the beginning of the function name
>>>
>>> Use either one or two tab stops to ensure that each parameter
>>> is indented at least two spaces after the function name. Either
>>> of the below examples is acceptable:
>>>
>>> Status = gBS->AllocatePool (
>>> EfiBootServicesData,
>>> sizeof (DRIVER_NAME_INSTANCE),
>>> &PrivateData
>>> );
>>>
>>> Status = gBS->AllocatePool (
>>> EfiBootServicesData,
>>> sizeof (DRIVER_NAME_INSTANCE),
>>> &PrivateData
>>> );
>>>
>>> My notes on this:
>>>
>>> - my code complies with the indentation requirement
>>>
>>> - the passage is silent on whether each argument should be broken off
>>> to a new line. The examples are called "acceptable", not "required".
>>> I guess this is what your question to Mike is about.
>>>
>>
>> Right. It seems like this is a case where the text perhaps could be
>> more clear. Just looking at the precedence in the tree, it seems
>> likely that we might want to say that if the call requires multiple
>> lines, then the first parameter should be indented on the next line.
>>
>>> - I don't see what the difference is between the two examples. To me
>>> they look identical.
>>
>> I think this example formatting got messed up. I saw a previous
>> version that varied very subtlely.
>>
>> Status = gBS->AllocatePool (
>> EfiBootServicesData,
>> sizeof (DRIVER_NAME_INSTANCE),
>> &PrivateData
>> );
>>
>> Status = gBS->AllocatePool (
>> EfiBootServicesData,
>> sizeof (DRIVER_NAME_INSTANCE),
>> &PrivateData
>> );
>>
>> I always thought the rule was 2 spaces indenting beyond the function
>> name.
>>
>> These example make me think that the intension is that your tab key
>> might only want to align to columns that are even numbered, so you
>> might have the result be that the indent from the function name would
>> be 3 spaces.
>>
>> I don't know... I prefer the rule that you indent 2 spaces from the
>> function name. :)
>>
>>> - In the examples, the closing parens line up with "AllocatePool", not
>>> with "EfiBootServicesData". I think I have never ever seen this style
>>> in edk2.
>>>
>>
>> Yeah. Me either. This is confusing.
>>
>> -Jordan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-03-14 13:48 ` Laszlo Ersek
@ 2017-03-14 20:38 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-03-14 20:38 UTC (permalink / raw)
To: Kinney, Michael D, Justen, Jordan L; +Cc: edk2-devel-01
On 03/14/17 14:48, Laszlo Ersek wrote:
> On 03/14/17 02:58, Kinney, Michael D wrote:
>> Laszlo,
>>
>> I agree that there are some inconsistencies in the edk2 C source files.
>>
>> For this specific example, I prefer what Jordan recommended:
>>
>>>> Status = QemuFwCfgS3WriteBytes (
>>>> mRequestedFeaturesItem,
>>>> sizeof ScratchBuffer->Features
>>>> );
>>
>> When multi-line:
>> * One argument per line
>> * Indent each argument 2 spaces from start of function name
>> * Close parenthesis aligned with start of arguments
>>
>> I am actively working on adding the EDK II C Coding standard
>> and other EDK II docs to gitbooks. That will be a good place
>> to discuss and clarify or correct the EDK II C coding standard.
>>
>> Given inconsistencies in some of the edk2 C source files and
>> some clarification needed in EDK II C Coding Standard, I would
>> not require any changes here.
>
> If I understand correctly, that means v2 of this set should be acceptable.
>
> If that's right, can you please R-b this set, Jordan?
>
>> I would prefer everyone follow
>> this specific recommendation if possible.
>
> I can do that, in the future, if we codify the preference in some
> written form, even before the CCS is updated and/or imported to
> gitbooks. Should we file a TianoCore BZ for it? I can do that, if you
> prefer.
I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=425>.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 12/12] OvmfPkg/AcpiPlatformDxe: save fw_cfg boot script with QemuFwCfgS3Lib
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (10 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 11/12] OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib Laszlo Ersek
@ 2017-02-23 1:48 ` Laszlo Ersek
2017-02-23 16:59 ` [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Jordan Justen
2017-03-06 9:19 ` Jordan Justen
13 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 1:48 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
Drop the explicit S3SaveState protocol and opcode management; instead,
create ACPI S3 Boot Script opcodes for the WRITE_POINTER commands with
QemuFwCfgS3Lib functions.
In this case, we have a dynamically allocated Context structure, hence the
patch demonstrates how the FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION takes
ownership of Context.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=394
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 1 -
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 1 -
OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 2 +-
OvmfPkg/AcpiPlatformDxe/BootScript.c | 262 +++++---------------
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 7 +
5 files changed, 64 insertions(+), 209 deletions(-)
diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
index 42edc97b3da2..9a9b2e6bb2e5 100644
--- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
+++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf
@@ -61,7 +61,6 @@ [LibraryClasses]
[Protocols]
gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEfiPciIoProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
- gEfiS3SaveStateProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
[Guids]
gEfiXenInfoGuid
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
index a9350540215d..adc50cfd9f76 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
@@ -51,7 +51,6 @@ [LibraryClasses]
[Protocols]
gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEfiPciIoProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
- gEfiS3SaveStateProtocolGuid # PROTOCOL SOMETIMES_CONSUMED
[Guids]
gRootBridgesConnectedEventGroupGuid
diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
index 0f035a0d5751..83b981ee005d 100644
--- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
+++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
@@ -115,7 +115,7 @@ SaveCondensedWritePointerToS3Context (
EFI_STATUS
TransferS3ContextToBootScript (
- IN CONST S3_CONTEXT *S3Context
+ IN S3_CONTEXT *S3Context
);
#endif
diff --git a/OvmfPkg/AcpiPlatformDxe/BootScript.c b/OvmfPkg/AcpiPlatformDxe/BootScript.c
index bff42ad8b9b0..424b4b3ee2bd 100644
--- a/OvmfPkg/AcpiPlatformDxe/BootScript.c
+++ b/OvmfPkg/AcpiPlatformDxe/BootScript.c
@@ -15,7 +15,7 @@
#include <Library/MemoryAllocationLib.h>
#include <Library/QemuFwCfgLib.h>
-#include <Protocol/S3SaveState.h>
+#include <Library/QemuFwCfgS3Lib.h>
#include "AcpiPlatform.h"
@@ -53,19 +53,11 @@ struct S3_CONTEXT {
//
// Scratch buffer, allocated in EfiReservedMemoryType type memory, for the ACPI
-// S3 Boot Script opcodes to work on. We use the buffer to compose and to
-// replay several fw_cfg select+skip and write operations, using the DMA access
-// method. The fw_cfg operations will implement the actions dictated by
-// CONDENSED_WRITE_POINTER objects.
+// S3 Boot Script opcodes to work on.
//
#pragma pack (1)
-typedef struct {
- FW_CFG_DMA_ACCESS Access; // filled in from
- // CONDENSED_WRITE_POINTER.PointerItem,
- // CONDENSED_WRITE_POINTER.PointerSize,
- // CONDENSED_WRITE_POINTER.PointerOffset
- UINT64 PointerValue; // filled in from
- // CONDENSED_WRITE_POINTER.PointerValue
+typedef union {
+ UINT64 PointerValue; // filled in from CONDENSED_WRITE_POINTER.PointerValue
} SCRATCH_BUFFER;
#pragma pack ()
@@ -197,220 +189,78 @@ SaveCondensedWritePointerToS3Context (
/**
- Translate and append the information from an S3_CONTEXT object to the ACPI S3
- Boot Script.
-
- The effects of a successful call to this function cannot be undone.
-
- @param[in] S3Context The S3_CONTEXT object to translate to ACPI S3 Boot
- Script opcodes.
-
- @retval EFI_OUT_OF_RESOURCES Out of memory.
-
- @retval EFI_SUCCESS The translation of S3Context to ACPI S3 Boot
- Script opcodes has been successful.
-
- @return Error codes from underlying functions.
+ FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION provided to QemuFwCfgS3Lib.
**/
-EFI_STATUS
-TransferS3ContextToBootScript (
- IN CONST S3_CONTEXT *S3Context
+STATIC
+VOID
+EFIAPI
+AppendFwCfgBootScript (
+ IN OUT VOID *Context, OPTIONAL
+ IN OUT VOID *ExternalScratchBuffer
)
{
- EFI_STATUS Status;
- EFI_S3_SAVE_STATE_PROTOCOL *S3SaveState;
- SCRATCH_BUFFER *ScratchBuffer;
- FW_CFG_DMA_ACCESS *Access;
- UINT64 BigEndianAddressOfAccess;
- UINT32 ControlPollData;
- UINT32 ControlPollMask;
- UINTN Index;
+ S3_CONTEXT *S3Context;
+ SCRATCH_BUFFER *ScratchBuffer;
+ UINTN Index;
- //
- // If the following protocol lookup fails, it shall not happen due to an
- // unexpected DXE driver dispatch order.
- //
- // Namely, this function is only invoked on QEMU. Therefore it is only
- // reached after Platform BDS signals gRootBridgesConnectedEventGroupGuid
- // (see OnRootBridgesConnected() in "EntryPoint.c"). Hence, because
- // TransferS3ContextToBootScript() is invoked in BDS, all DXE drivers,
- // including S3SaveStateDxe (producing EFI_S3_SAVE_STATE_PROTOCOL), have been
- // dispatched by the time we get here. (S3SaveStateDxe is not expected to
- // have any stricter-than-TRUE DEPEX -- not a DEPEX that gets unblocked only
- // within BDS anyway.)
- //
- // Reaching this function also depends on QemuFwCfgS3Enabled(). That implies
- // S3SaveStateDxe has not exited immediately due to S3 being disabled. Thus
- // EFI_S3_SAVE_STATE_PROTOCOL can only be missing for genuinely unforeseeable
- // reasons.
- //
- Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid,
- NULL /* Registration */, (VOID **)&S3SaveState);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a: LocateProtocol(): %r\n", __FUNCTION__, Status));
- return Status;
- }
+ S3Context = Context;
+ ScratchBuffer = ExternalScratchBuffer;
- ScratchBuffer = AllocateReservedPool (sizeof *ScratchBuffer);
- if (ScratchBuffer == NULL) {
- return EFI_OUT_OF_RESOURCES;
- }
-
- //
- // Set up helper variables that we'll use identically for all
- // CONDENSED_WRITE_POINTER elements.
- //
- Access = &ScratchBuffer->Access;
- BigEndianAddressOfAccess = SwapBytes64 ((UINTN)Access);
- ControlPollData = 0;
- ControlPollMask = MAX_UINT32;
-
- //
- // For each CONDENSED_WRITE_POINTER, we need six ACPI S3 Boot Script opcodes:
- // (1) restore an FW_CFG_DMA_ACCESS object in reserved memory that selects
- // the writeable fw_cfg file PointerFile (through PointerItem), and skips
- // to PointerOffset in it,
- // (2) call QEMU with the FW_CFG_DMA_ACCESS object,
- // (3) wait for the select+skip to finish,
- // (4) restore a SCRATCH_BUFFER object in reserved memory that writes
- // PointerValue (base address of the allocated / downloaded PointeeFile,
- // plus PointeeOffset), of size PointerSize, into the fw_cfg file
- // selected in (1), at the offset sought to in (1),
- // (5) call QEMU with the FW_CFG_DMA_ACCESS object,
- // (6) wait for the write to finish.
- //
- // EFI_S3_SAVE_STATE_PROTOCOL does not allow rolling back opcode additions,
- // therefore we treat any failure here as fatal.
- //
for (Index = 0; Index < S3Context->Used; ++Index) {
CONST CONDENSED_WRITE_POINTER *Condensed;
+ RETURN_STATUS Status;
Condensed = &S3Context->WritePointers[Index];
- //
- // (1) restore an FW_CFG_DMA_ACCESS object in reserved memory that selects
- // the writeable fw_cfg file PointerFile (through PointerItem), and
- // skips to PointerOffset in it,
- //
- Access->Control = SwapBytes32 ((UINT32)Condensed->PointerItem << 16 |
- FW_CFG_DMA_CTL_SELECT | FW_CFG_DMA_CTL_SKIP);
- Access->Length = SwapBytes32 (Condensed->PointerOffset);
- Access->Address = 0;
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint8, // Width
- (UINT64)(UINTN)Access, // Address
- sizeof *Access, // Count
- Access // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 1: %r\n", __FUNCTION__,
- (UINT64)Index, Status));
+ Status = QemuFwCfgS3SkipBytes (Condensed->PointerItem,
+ Condensed->PointerOffset);
+ if (RETURN_ERROR (Status)) {
goto FatalError;
}
- //
- // (2) call QEMU with the FW_CFG_DMA_ACCESS object,
- //
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint32, // Width
- (UINT64)FW_CFG_IO_DMA_ADDRESS, // Address
- (UINTN)2, // Count
- &BigEndianAddressOfAccess // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 2: %r\n", __FUNCTION__,
- (UINT64)Index, Status));
- goto FatalError;
- }
-
- //
- // (3) wait for the select+skip to finish,
- //
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
- EfiBootScriptWidthUint32, // Width
- (UINT64)(UINTN)&Access->Control, // Address
- &ControlPollData, // Data
- &ControlPollMask, // DataMask
- MAX_UINT64 // Delay
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 3: %r\n", __FUNCTION__,
- (UINT64)Index, Status));
- goto FatalError;
- }
-
- //
- // (4) restore a SCRATCH_BUFFER object in reserved memory that writes
- // PointerValue (base address of the allocated / downloaded
- // PointeeFile, plus PointeeOffset), of size PointerSize, into the
- // fw_cfg file selected in (1), at the offset sought to in (1),
- //
- Access->Control = SwapBytes32 (FW_CFG_DMA_CTL_WRITE);
- Access->Length = SwapBytes32 (Condensed->PointerSize);
- Access->Address = SwapBytes64 ((UINTN)&ScratchBuffer->PointerValue);
ScratchBuffer->PointerValue = Condensed->PointerValue;
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_MEM_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint8, // Width
- (UINT64)(UINTN)ScratchBuffer, // Address
- sizeof *ScratchBuffer, // Count
- ScratchBuffer // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 4: %r\n", __FUNCTION__,
- (UINT64)Index, Status));
- goto FatalError;
- }
-
- //
- // (5) call QEMU with the FW_CFG_DMA_ACCESS object,
- //
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_IO_WRITE_OPCODE, // OpCode
- EfiBootScriptWidthUint32, // Width
- (UINT64)FW_CFG_IO_DMA_ADDRESS, // Address
- (UINTN)2, // Count
- &BigEndianAddressOfAccess // Buffer
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 5: %r\n", __FUNCTION__,
- (UINT64)Index, Status));
- goto FatalError;
- }
-
- //
- // (6) wait for the write to finish.
- //
- Status = S3SaveState->Write (
- S3SaveState, // This
- EFI_BOOT_SCRIPT_MEM_POLL_OPCODE, // OpCode
- EfiBootScriptWidthUint32, // Width
- (UINT64)(UINTN)&Access->Control, // Address
- &ControlPollData, // Data
- &ControlPollMask, // DataMask
- MAX_UINT64 // Delay
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a: Index %Lu opcode 6: %r\n", __FUNCTION__,
- (UINT64)Index, Status));
+ Status = QemuFwCfgS3WriteBytes (-1, Condensed->PointerSize);
+ if (RETURN_ERROR (Status)) {
goto FatalError;
}
}
- DEBUG ((DEBUG_VERBOSE, "%a: boot script fragment saved, ScratchBuffer=%p\n",
- __FUNCTION__, (VOID *)ScratchBuffer));
- return EFI_SUCCESS;
+ DEBUG ((DEBUG_VERBOSE, "%a: boot script fragment saved\n", __FUNCTION__));
+
+ ReleaseS3Context (S3Context);
+ return;
FatalError:
ASSERT (FALSE);
CpuDeadLoop ();
- return Status;
+}
+
+
+/**
+ Translate and append the information from an S3_CONTEXT object to the ACPI S3
+ Boot Script.
+
+ The effects of a successful call to this function cannot be undone.
+
+ @param[in] S3Context The S3_CONTEXT object to translate to ACPI S3 Boot
+ Script opcodes. If the function returns successfully,
+ the caller must set the S3Context pointer -- originally
+ returned by AllocateS3Context() -- immediately to NULL,
+ because the ownership of S3Context has been transfered.
+
+ @retval EFI_SUCCESS The translation of S3Context to ACPI S3 Boot Script
+ opcodes has been successfully executed or queued.
+
+ @return Error codes from underlying functions.
+**/
+EFI_STATUS
+TransferS3ContextToBootScript (
+ IN S3_CONTEXT *S3Context
+ )
+{
+ RETURN_STATUS Status;
+
+ Status = QemuFwCfgS3TransferOwnership (AppendFwCfgBootScript, S3Context,
+ sizeof (SCRATCH_BUFFER));
+ return (EFI_STATUS)Status;
}
diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 76512534f5e0..7bb2e3f21821 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -848,6 +848,13 @@ InstallQemuFwCfgTables (
//
if (S3Context != NULL) {
Status = TransferS3ContextToBootScript (S3Context);
+ if (EFI_ERROR (Status)) {
+ goto UninstallAcpiTables;
+ }
+ //
+ // Ownership of S3Context has been transfered.
+ //
+ S3Context = NULL;
}
UninstallAcpiTables:
--
2.9.3
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (11 preceding siblings ...)
2017-02-23 1:48 ` [PATCH 12/12] OvmfPkg/AcpiPlatformDxe: " Laszlo Ersek
@ 2017-02-23 16:59 ` Jordan Justen
2017-02-23 17:22 ` Laszlo Ersek
2017-03-06 9:19 ` Jordan Justen
13 siblings, 1 reply; 32+ messages in thread
From: Jordan Justen @ 2017-02-23 16:59 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Ard Biesheuvel
On 2017-02-22 17:48:02, Laszlo Ersek wrote:
> The new QemuFwCfgS3Lib class has two goals:
>
> (a) to query whether S3 support was enabled on the QEMU command line,
>
> (b) to save fw_cfg DMA operations that are to be replayed at S3 resume
> time, and more easily for the programmer than hacking Boot Script
> opcodes manually.
>
> Patches #1 through #5 introduce the new library class, with Base Null,
> PEI fw_cfg, and DXE fw_cfg instances, covering goal (a) in both
> ArmVirtPkg and OvmfPkg.
>
> Patch #6 retires QemuFwCfgS3Enabled() from QemuFwCfgLib (including
> library class and all instances), and switches all client modules to
> QemuFwCfgS3Lib. This separates S3 concerns from QemuFwCfgLib.
1-6 Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> Patches #7 through #10 cover goal (b) for all three library instances
> (at levels of support that are appropriate for each, of course).
>
> Patches #11 and #12 put the new library class to use in
> OvmfPkg/SmmControl2Dxe and OvmfPkg/AcpiPlatformDxe, eliminating such
> ACPI S3 Boot Script opcode hacking that is related to fw_cfg. (For
> OvmfPkg/SmmControl2Dxe, that's "most of it", for
> OvmfPkg/AcpiPlatformDxe, it's "all of it".)
>
> I tested:
> - ArmVirtQemu boot,
> - OVMF boot without SMM, with S3 enabled and disabled, using a Linux
> guest (and when S3 was enabled, I exercised it),
> - OVMF boot with SMM, with S3 enabled and disabled, using Linux and
> Windows guests,
> - and whenever S3 was enabled, I exercised it
> - and in the Windows guest, I tested VMGENID / WRITE_POINTER too.
>
> The diffstat looks scary, but it's due to comments, I promise.
>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=394
> Repo: https://github.com/lersek/edk2.git
> Branch: fw_cfg_s3
>
> NOTE: if you want to fetch & test the branch, you'll have to revert
> recent commit dc4c770763d0 ("BaseTools: add error check for Macro usage
> in the INF file", 2017-02-20) on top. It causes BaseTools to mis-build
> OVMF. I reported the regression on the list already, in that patch's
> thread.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (12):
> OvmfPkg: introduce QemuFwCfgS3Lib class
> OvmfPkg/QemuFwCfgS3Lib: add initial Base Null library instance
> OvmfPkg/QemuFwCfgS3Lib: add initial PEI and DXE fw_cfg library
> instances
> ArmVirtPkg: resolve QemuFwCfgS3Lib
> OvmfPkg: resolve QemuFwCfgS3Lib
> ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib
> OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to
> libclass
> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for Base Null instance
> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for PEI fw_cfg instance
> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for DXE fw_cfg instance
> OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
> OvmfPkg/AcpiPlatformDxe: save fw_cfg boot script with QemuFwCfgS3Lib
>
> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
> ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 17 -
> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 2 +-
> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 2 +-
> OvmfPkg/AcpiPlatformDxe/BootScript.c | 262 ++-----
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 8 +
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 2 +-
> OvmfPkg/Include/Library/QemuFwCfgLib.h | 14 -
> OvmfPkg/Include/Library/QemuFwCfgS3Lib.h | 357 +++++++++
> OvmfPkg/Library/LockBoxLib/LockBoxDxe.c | 1 +
> OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 1 +
> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 1 +
> OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 28 -
> OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf | 43 ++
> OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf | 46 ++
> OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf | 44 ++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c | 109 +++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c | 227 ++++++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c | 791 ++++++++++++++++++++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c | 85 +++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c | 48 ++
> OvmfPkg/OvmfPkg.dec | 4 +
> OvmfPkg/OvmfPkgIa32.dsc | 3 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 3 +
> OvmfPkg/OvmfPkgX64.dsc | 3 +
> OvmfPkg/PlatformPei/Platform.c | 1 +
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 224 ++----
> OvmfPkg/SmmControl2Dxe/SmiFeatures.h | 5 +-
> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 6 +-
> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 1 +
> 33 files changed, 1917 insertions(+), 425 deletions(-)
> create mode 100644 OvmfPkg/Include/Library/QemuFwCfgS3Lib.h
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c
>
> --
> 2.9.3
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib
2017-02-23 16:59 ` [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Jordan Justen
@ 2017-02-23 17:22 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-02-23 17:22 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01; +Cc: Ard Biesheuvel
On 02/23/17 17:59, Jordan Justen wrote:
> On 2017-02-22 17:48:02, Laszlo Ersek wrote:
>> The new QemuFwCfgS3Lib class has two goals:
>>
>> (a) to query whether S3 support was enabled on the QEMU command line,
>>
>> (b) to save fw_cfg DMA operations that are to be replayed at S3 resume
>> time, and more easily for the programmer than hacking Boot Script
>> opcodes manually.
>>
>> Patches #1 through #5 introduce the new library class, with Base Null,
>> PEI fw_cfg, and DXE fw_cfg instances, covering goal (a) in both
>> ArmVirtPkg and OvmfPkg.
>>
>> Patch #6 retires QemuFwCfgS3Enabled() from QemuFwCfgLib (including
>> library class and all instances), and switches all client modules to
>> QemuFwCfgS3Lib. This separates S3 concerns from QemuFwCfgLib.
>
> 1-6 Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Thank you!
... Because goal (b) is mentioned in some of the commit messages and
source files of patches #1 through #6, I think it best if we only commit
these patches all together, once they are fully reviewed. I'm in no rush
with this :) (Unlike with WRITE_POINTER.)
Cheers!
Laszlo
>
>> Patches #7 through #10 cover goal (b) for all three library instances
>> (at levels of support that are appropriate for each, of course).
>>
>> Patches #11 and #12 put the new library class to use in
>> OvmfPkg/SmmControl2Dxe and OvmfPkg/AcpiPlatformDxe, eliminating such
>> ACPI S3 Boot Script opcode hacking that is related to fw_cfg. (For
>> OvmfPkg/SmmControl2Dxe, that's "most of it", for
>> OvmfPkg/AcpiPlatformDxe, it's "all of it".)
>>
>> I tested:
>> - ArmVirtQemu boot,
>> - OVMF boot without SMM, with S3 enabled and disabled, using a Linux
>> guest (and when S3 was enabled, I exercised it),
>> - OVMF boot with SMM, with S3 enabled and disabled, using Linux and
>> Windows guests,
>> - and whenever S3 was enabled, I exercised it
>> - and in the Windows guest, I tested VMGENID / WRITE_POINTER too.
>>
>> The diffstat looks scary, but it's due to comments, I promise.
>>
>> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=394
>> Repo: https://github.com/lersek/edk2.git
>> Branch: fw_cfg_s3
>>
>> NOTE: if you want to fetch & test the branch, you'll have to revert
>> recent commit dc4c770763d0 ("BaseTools: add error check for Macro usage
>> in the INF file", 2017-02-20) on top. It causes BaseTools to mis-build
>> OVMF. I reported the regression on the list already, in that patch's
>> thread.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (12):
>> OvmfPkg: introduce QemuFwCfgS3Lib class
>> OvmfPkg/QemuFwCfgS3Lib: add initial Base Null library instance
>> OvmfPkg/QemuFwCfgS3Lib: add initial PEI and DXE fw_cfg library
>> instances
>> ArmVirtPkg: resolve QemuFwCfgS3Lib
>> OvmfPkg: resolve QemuFwCfgS3Lib
>> ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib
>> OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to
>> libclass
>> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for Base Null instance
>> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for PEI fw_cfg instance
>> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for DXE fw_cfg instance
>> OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
>> OvmfPkg/AcpiPlatformDxe: save fw_cfg boot script with QemuFwCfgS3Lib
>>
>> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
>> ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
>> ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 17 -
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 2 +-
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 2 +-
>> OvmfPkg/AcpiPlatformDxe/BootScript.c | 262 ++-----
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 8 +
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 2 +-
>> OvmfPkg/Include/Library/QemuFwCfgLib.h | 14 -
>> OvmfPkg/Include/Library/QemuFwCfgS3Lib.h | 357 +++++++++
>> OvmfPkg/Library/LockBoxLib/LockBoxDxe.c | 1 +
>> OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 1 +
>> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 1 +
>> OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 28 -
>> OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf | 43 ++
>> OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf | 46 ++
>> OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf | 44 ++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c | 109 +++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c | 227 ++++++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c | 791 ++++++++++++++++++++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c | 85 +++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c | 48 ++
>> OvmfPkg/OvmfPkg.dec | 4 +
>> OvmfPkg/OvmfPkgIa32.dsc | 3 +
>> OvmfPkg/OvmfPkgIa32X64.dsc | 3 +
>> OvmfPkg/OvmfPkgX64.dsc | 3 +
>> OvmfPkg/PlatformPei/Platform.c | 1 +
>> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
>> OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 224 ++----
>> OvmfPkg/SmmControl2Dxe/SmiFeatures.h | 5 +-
>> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 6 +-
>> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 1 +
>> 33 files changed, 1917 insertions(+), 425 deletions(-)
>> create mode 100644 OvmfPkg/Include/Library/QemuFwCfgS3Lib.h
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c
>>
>> --
>> 2.9.3
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib
2017-02-23 1:48 [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Laszlo Ersek
` (12 preceding siblings ...)
2017-02-23 16:59 ` [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Jordan Justen
@ 2017-03-06 9:19 ` Jordan Justen
2017-03-06 18:41 ` Laszlo Ersek
13 siblings, 1 reply; 32+ messages in thread
From: Jordan Justen @ 2017-03-06 9:19 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Ard Biesheuvel
On 2017-02-22 17:48:02, Laszlo Ersek wrote:
> The new QemuFwCfgS3Lib class has two goals:
>
> (a) to query whether S3 support was enabled on the QEMU command line,
>
> (b) to save fw_cfg DMA operations that are to be replayed at S3 resume
> time, and more easily for the programmer than hacking Boot Script
> opcodes manually.
>
> Patches #1 through #5 introduce the new library class, with Base Null,
> PEI fw_cfg, and DXE fw_cfg instances, covering goal (a) in both
> ArmVirtPkg and OvmfPkg.
>
> Patch #6 retires QemuFwCfgS3Enabled() from QemuFwCfgLib (including
> library class and all instances), and switches all client modules to
> QemuFwCfgS3Lib. This separates S3 concerns from QemuFwCfgLib.
>
> Patches #7 through #10 cover goal (b) for all three library instances
> (at levels of support that are appropriate for each, of course).
>
> Patches #11 and #12 put the new library class to use in
> OvmfPkg/SmmControl2Dxe and OvmfPkg/AcpiPlatformDxe, eliminating such
> ACPI S3 Boot Script opcode hacking that is related to fw_cfg. (For
> OvmfPkg/SmmControl2Dxe, that's "most of it", for
> OvmfPkg/AcpiPlatformDxe, it's "all of it".)
The new functions added in 7 don't seem very generic.
My first thought (which I basically talked myself out of) was, should
we just make one simple platform specific function in the lib
interface. It would mean that the library is platform specific, but
maybe it could simplify things a lot. Like I mentioned, I don't feel
to sure about this being the best way to go, but I just mention it to
see if it seems compelling to you.
==
Next I started to think about what might make the interface feel
better as a more generic interface. So I had some ideas/questions:
1. Append sounded a bit odd for the callback function. What about
FW_CFG_BOOT_SCRIPT_CALLBACK_FUNCTION?
2. I think the read/write function only work from offset 0 of the
buffer. Is that correct? Is there any reason we couldn't add a
pointer within the scatch buffer for reading/writing to? It just
seems to make the interface a bit more generic/intuitive.
3. What do you think of adding 'Script'? QemuFwCfgS3ScriptWriteBytes
or something like that?
Thanks,
-Jordan
> I tested:
> - ArmVirtQemu boot,
> - OVMF boot without SMM, with S3 enabled and disabled, using a Linux
> guest (and when S3 was enabled, I exercised it),
> - OVMF boot with SMM, with S3 enabled and disabled, using Linux and
> Windows guests,
> - and whenever S3 was enabled, I exercised it
> - and in the Windows guest, I tested VMGENID / WRITE_POINTER too.
>
> The diffstat looks scary, but it's due to comments, I promise.
>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=394
> Repo: https://github.com/lersek/edk2.git
> Branch: fw_cfg_s3
>
> NOTE: if you want to fetch & test the branch, you'll have to revert
> recent commit dc4c770763d0 ("BaseTools: add error check for Macro usage
> in the INF file", 2017-02-20) on top. It causes BaseTools to mis-build
> OVMF. I reported the regression on the list already, in that patch's
> thread.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (12):
> OvmfPkg: introduce QemuFwCfgS3Lib class
> OvmfPkg/QemuFwCfgS3Lib: add initial Base Null library instance
> OvmfPkg/QemuFwCfgS3Lib: add initial PEI and DXE fw_cfg library
> instances
> ArmVirtPkg: resolve QemuFwCfgS3Lib
> OvmfPkg: resolve QemuFwCfgS3Lib
> ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib
> OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to
> libclass
> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for Base Null instance
> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for PEI fw_cfg instance
> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for DXE fw_cfg instance
> OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
> OvmfPkg/AcpiPlatformDxe: save fw_cfg boot script with QemuFwCfgS3Lib
>
> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
> ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
> ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 17 -
> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 2 +-
> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 2 +-
> OvmfPkg/AcpiPlatformDxe/BootScript.c | 262 ++-----
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 8 +
> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 2 +-
> OvmfPkg/Include/Library/QemuFwCfgLib.h | 14 -
> OvmfPkg/Include/Library/QemuFwCfgS3Lib.h | 357 +++++++++
> OvmfPkg/Library/LockBoxLib/LockBoxDxe.c | 1 +
> OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 1 +
> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 1 +
> OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 28 -
> OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf | 43 ++
> OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf | 46 ++
> OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf | 44 ++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c | 109 +++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c | 227 ++++++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c | 791 ++++++++++++++++++++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c | 85 +++
> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c | 48 ++
> OvmfPkg/OvmfPkg.dec | 4 +
> OvmfPkg/OvmfPkgIa32.dsc | 3 +
> OvmfPkg/OvmfPkgIa32X64.dsc | 3 +
> OvmfPkg/OvmfPkgX64.dsc | 3 +
> OvmfPkg/PlatformPei/Platform.c | 1 +
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 224 ++----
> OvmfPkg/SmmControl2Dxe/SmiFeatures.h | 5 +-
> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 6 +-
> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 1 +
> 33 files changed, 1917 insertions(+), 425 deletions(-)
> create mode 100644 OvmfPkg/Include/Library/QemuFwCfgS3Lib.h
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c
> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c
>
> --
> 2.9.3
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib
2017-03-06 9:19 ` Jordan Justen
@ 2017-03-06 18:41 ` Laszlo Ersek
2017-03-10 9:55 ` Jordan Justen
0 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2017-03-06 18:41 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01; +Cc: Ard Biesheuvel
On 03/06/17 10:19, Jordan Justen wrote:
> On 2017-02-22 17:48:02, Laszlo Ersek wrote:
>> The new QemuFwCfgS3Lib class has two goals:
>>
>> (a) to query whether S3 support was enabled on the QEMU command line,
>>
>> (b) to save fw_cfg DMA operations that are to be replayed at S3 resume
>> time, and more easily for the programmer than hacking Boot Script
>> opcodes manually.
>>
>> Patches #1 through #5 introduce the new library class, with Base Null,
>> PEI fw_cfg, and DXE fw_cfg instances, covering goal (a) in both
>> ArmVirtPkg and OvmfPkg.
>>
>> Patch #6 retires QemuFwCfgS3Enabled() from QemuFwCfgLib (including
>> library class and all instances), and switches all client modules to
>> QemuFwCfgS3Lib. This separates S3 concerns from QemuFwCfgLib.
>>
>> Patches #7 through #10 cover goal (b) for all three library instances
>> (at levels of support that are appropriate for each, of course).
>>
>> Patches #11 and #12 put the new library class to use in
>> OvmfPkg/SmmControl2Dxe and OvmfPkg/AcpiPlatformDxe, eliminating such
>> ACPI S3 Boot Script opcode hacking that is related to fw_cfg. (For
>> OvmfPkg/SmmControl2Dxe, that's "most of it", for
>> OvmfPkg/AcpiPlatformDxe, it's "all of it".)
>
> The new functions added in 7 don't seem very generic.
>
> My first thought (which I basically talked myself out of) was, should
> we just make one simple platform specific function in the lib
> interface. It would mean that the library is platform specific, but
> maybe it could simplify things a lot. Like I mentioned, I don't feel
> to sure about this being the best way to go, but I just mention it to
> see if it seems compelling to you.
It doesn't :) The only platform where the new lib currently makes sense
is x86; any generalization beyond that would be very speculative IMO. It
was hard for me (with your help counted in!) to find even this level of
abstraction, for the uses cases that already exist.
>
> ==
>
> Next I started to think about what might make the interface feel
> better as a more generic interface. So I had some ideas/questions:
>
> 1. Append sounded a bit odd for the callback function. What about
> FW_CFG_BOOT_SCRIPT_CALLBACK_FUNCTION?
Sure, I'll rename it.
>
> 2. I think the read/write function only work from offset 0 of the
> buffer. Is that correct?
Yes, that is correct, and it's intentional.
> Is there any reason we couldn't add a
> pointer within the scatch buffer for reading/writing to? It just
> seems to make the interface a bit more generic/intuitive.
That's how I initially designed the interface, but while implementing
the functions, I realized there was no good use for that feature.
The only use case for storing the data read from fw_cfg at a nonzero
offset in the scratch buffer, and for writing data to fw_cfg from a
nonzero offset in the scratch buffer, would be that you have "something
else" at offset zero. What could that thing be?
* It could be the result of an earlier read. However, if you have read
something from fw_cfg, just check the result immediately (with the
appropriate function / opcode), and then the same area (at offset zero0
becomes reusable. You don't want to proceed with the boot script without
checking the read data first anyway! If the read returns something we
dislike, we need to stop (hang) immediately.
* It could be data for another ("later") write to send to fw_cfg.
However, "later write" doesn't make much sense in this context. For each
fw_cfg write, we need to restore the data (captured in the opcode
itself!) to a known memory location, then fire off the DMA transfer,
then await completion.
If you have two writes, you could populate two distinct DMA access
stuctures and two data blobs first, then fire off the first transfer
(and wait for it to complete), and then fire off the second transfer
(and wait for it to complete). By the time you fire off the second
transfer, the memory used by the first transfer has become completely
useless.
This is why the union type for the scratch buffer (aka "zero offset
only") makes sense -- there is no need for the buffers of two separate
fw_cfg transfers to co-exist at any time. And, it lets you save reserved
memory.
The naming is not random, it really is a *scratch* buffer only. The
actual data for the transfers (and value checks) are captured within the
opcodes themselves. Those definitely co-exist. However, we need the
scratch ("temporary") buffer to serve only the currently executing opcode.
I think it could be slightly surprising when composing the operations.
For writes for example, you could be tempted to populate various
(coexistent) parts of the scratch buffer first, in one go, then queue a
multitude of opcodes for several fw_cfg write operations in another go,
each referring to a different part of the scratch buffer.
That just wastes reserved memory however. Once you are done queueing the
opcodes for a *single* fw_cfg write operation, the data from the scratch
buffer has been *copied* into the opcodes themselves, and the same
offsets in the scratch buffer are eligible for reuse. And when the
opcodes are replayed for the same single fw_cfg write, the data is
restored in-place, but as soon as the DMA completes, the same offsets
can again be overwritten by data that's being restored in-place for the
*next* fw_cfg operation (see above).
Again, as soon I tried implementing and using this extra flexibility
(which was my original approach), I realized it was unnecessary, and it
would just encourage wasting reserved memory.
The last two patches in the series illustrate the usage -- observe that
scratch buffer manipulation and opcode queueing happen in lock-step.
And, if you test an S3 resume and look at the messages produced by the
boot script executor, the reserved memory access becomes much clearer.
>
> 3. What do you think of adding 'Script'? QemuFwCfgS3ScriptWriteBytes
> or something like that?
Good idea, I'll do that too.
So, if you accept my answer to #2, I'll post (sometime...) an update
with #1 and #3 addressed.
Thanks!
Laszlo
>
> Thanks,
>
> -Jordan
>
>> I tested:
>> - ArmVirtQemu boot,
>> - OVMF boot without SMM, with S3 enabled and disabled, using a Linux
>> guest (and when S3 was enabled, I exercised it),
>> - OVMF boot with SMM, with S3 enabled and disabled, using Linux and
>> Windows guests,
>> - and whenever S3 was enabled, I exercised it
>> - and in the Windows guest, I tested VMGENID / WRITE_POINTER too.
>>
>> The diffstat looks scary, but it's due to comments, I promise.
>>
>> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=394
>> Repo: https://github.com/lersek/edk2.git
>> Branch: fw_cfg_s3
>>
>> NOTE: if you want to fetch & test the branch, you'll have to revert
>> recent commit dc4c770763d0 ("BaseTools: add error check for Macro usage
>> in the INF file", 2017-02-20) on top. It causes BaseTools to mis-build
>> OVMF. I reported the regression on the list already, in that patch's
>> thread.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (12):
>> OvmfPkg: introduce QemuFwCfgS3Lib class
>> OvmfPkg/QemuFwCfgS3Lib: add initial Base Null library instance
>> OvmfPkg/QemuFwCfgS3Lib: add initial PEI and DXE fw_cfg library
>> instances
>> ArmVirtPkg: resolve QemuFwCfgS3Lib
>> OvmfPkg: resolve QemuFwCfgS3Lib
>> ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib
>> OvmfPkg/QemuFwCfgS3Lib: add boot script opcode generation APIs to
>> libclass
>> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for Base Null instance
>> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for PEI fw_cfg instance
>> OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for DXE fw_cfg instance
>> OvmfPkg/SmmControl2Dxe: save fw_cfg boot script with QemuFwCfgS3Lib
>> OvmfPkg/AcpiPlatformDxe: save fw_cfg boot script with QemuFwCfgS3Lib
>>
>> ArmVirtPkg/ArmVirtQemu.dsc | 1 +
>> ArmVirtPkg/ArmVirtQemuKernel.dsc | 1 +
>> ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 17 -
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 2 +-
>> OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 2 +-
>> OvmfPkg/AcpiPlatformDxe/BootScript.c | 262 ++-----
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 8 +
>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf | 2 +-
>> OvmfPkg/Include/Library/QemuFwCfgLib.h | 14 -
>> OvmfPkg/Include/Library/QemuFwCfgS3Lib.h | 357 +++++++++
>> OvmfPkg/Library/LockBoxLib/LockBoxDxe.c | 1 +
>> OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 1 +
>> OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.h | 1 +
>> OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 1 +
>> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 28 -
>> OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf | 43 ++
>> OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf | 46 ++
>> OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf | 44 ++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c | 109 +++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c | 227 ++++++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c | 791 ++++++++++++++++++++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c | 85 +++
>> OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c | 48 ++
>> OvmfPkg/OvmfPkg.dec | 4 +
>> OvmfPkg/OvmfPkgIa32.dsc | 3 +
>> OvmfPkg/OvmfPkgIa32X64.dsc | 3 +
>> OvmfPkg/OvmfPkgX64.dsc | 3 +
>> OvmfPkg/PlatformPei/Platform.c | 1 +
>> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
>> OvmfPkg/SmmControl2Dxe/SmiFeatures.c | 224 ++----
>> OvmfPkg/SmmControl2Dxe/SmiFeatures.h | 5 +-
>> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.c | 6 +-
>> OvmfPkg/SmmControl2Dxe/SmmControl2Dxe.inf | 1 +
>> 33 files changed, 1917 insertions(+), 425 deletions(-)
>> create mode 100644 OvmfPkg/Include/Library/QemuFwCfgS3Lib.h
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/BaseQemuFwCfgS3LibNull.inf
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Base.c
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3BasePei.c
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Dxe.c
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3Pei.c
>> create mode 100644 OvmfPkg/Library/QemuFwCfgS3Lib/QemuFwCfgS3PeiDxe.c
>>
>> --
>> 2.9.3
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib
2017-03-06 18:41 ` Laszlo Ersek
@ 2017-03-10 9:55 ` Jordan Justen
2017-03-10 20:16 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Jordan Justen @ 2017-03-10 9:55 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Ard Biesheuvel
On 2017-03-06 10:41:31, Laszlo Ersek wrote:
> On 03/06/17 10:19, Jordan Justen wrote:
>
> > Is there any reason we couldn't add a
> > pointer within the scatch buffer for reading/writing to? It just
> > seems to make the interface a bit more generic/intuitive.
>
> That's how I initially designed the interface, but while implementing
> the functions, I realized there was no good use for that feature.
>
> The only use case for storing the data read from fw_cfg at a nonzero
> offset in the scratch buffer, and for writing data to fw_cfg from a
> nonzero offset in the scratch buffer, would be that you have "something
> else" at offset zero. What could that thing be?
>
We discussed this on irc a bit. I agree that we can proceed with your
current design.
I still think the library interface would be clearer if it had the
buffer passed to the read/write routines, but I concede that it would
complicate and grow the code for no other gain than a clearer library
interface.
I think the helper callback adds to the confusion in the library
interface, but if you don't have the buffer passed into the read/write
routines, then you need the scratch buffer, and then the callback makes
sense.
I think we could have one helper to 'call me back when boot script is
available', but didn't have the scratch allocation aspect. Of course,
having torn out all the fw-cfg context, this now seems like something
that some generic boot script lib might provide.
Then I'd leave it up to the user of the lib to allocate a buffer and
pass it into the fw-cfg boot script routines to be logged. The library
would also have to allocate the DMA transfer buffer, which is just one
way that this would be less efficient.
Once again, I agree this would be less efficient, but I think it
easier to follow. (Or, at least more intuitive from a library
interface perspective.) And, once again, I am okay with you proceeding
with your current design.
-Jordan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib
2017-03-10 9:55 ` Jordan Justen
@ 2017-03-10 20:16 ` Laszlo Ersek
2017-03-11 3:17 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2017-03-10 20:16 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01; +Cc: Ard Biesheuvel
On 03/10/17 10:55, Jordan Justen wrote:
> On 2017-03-06 10:41:31, Laszlo Ersek wrote:
>> On 03/06/17 10:19, Jordan Justen wrote:
>>
>>> Is there any reason we couldn't add a
>>> pointer within the scatch buffer for reading/writing to? It just
>>> seems to make the interface a bit more generic/intuitive.
>>
>> That's how I initially designed the interface, but while implementing
>> the functions, I realized there was no good use for that feature.
>>
>> The only use case for storing the data read from fw_cfg at a nonzero
>> offset in the scratch buffer, and for writing data to fw_cfg from a
>> nonzero offset in the scratch buffer, would be that you have "something
>> else" at offset zero. What could that thing be?
>>
>
> We discussed this on irc a bit. I agree that we can proceed with your
> current design.
>
> I still think the library interface would be clearer if it had the
> buffer passed to the read/write routines, but I concede that it would
> complicate and grow the code for no other gain than a clearer library
> interface.
>
> I think the helper callback adds to the confusion in the library
> interface, but if you don't have the buffer passed into the read/write
> routines, then you need the scratch buffer, and then the callback makes
> sense.
>
> I think we could have one helper to 'call me back when boot script is
> available', but didn't have the scratch allocation aspect. Of course,
> having torn out all the fw-cfg context, this now seems like something
> that some generic boot script lib might provide.
>
> Then I'd leave it up to the user of the lib to allocate a buffer and
> pass it into the fw-cfg boot script routines to be logged. The library
> would also have to allocate the DMA transfer buffer, which is just one
> way that this would be less efficient.
>
> Once again, I agree this would be less efficient, but I think it
> easier to follow. (Or, at least more intuitive from a library
> interface perspective.) And, once again, I am okay with you proceeding
> with your current design.
Thank you very much for taking the time to review this and for the
suggested improvements. I'd like to post v2 with the following changes:
- rename QemuFwCfgS3TransferOwnership to
QemuFwCfgS3CallWhenBootScriptReady
- rename FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION to
FW_CFG_BOOT_SCRIPT_CALLBACK_FUNCTION
- similarly, rename Append to BootScriptCallback
- rename QemuFwCfgS3Xxx to QemuFwCfgS3ScriptXxx
I wouldn't like to change the following aspects:
- Indentation of function calls -- I've been consistent about those for
several years, they satisfy the requirements of the coding std, and code
I wrote like that had been accepted even in MdeModulePkg
(PciHostBridgeDxe) and MdePkg (BaseOrderedCollectionRedBlackTreeLib).
My main argument against *exclusively* using the one-arg-per-line style
is that it wastes a lot of vertical space, especially because I'm
unwilling to write longer than 80char lines. (Those who are willing to
go up to 120 -- that is, 50% more! -- chars per line have it much
easier, because they need to break up a *lot* fewer function calls.)
- The way the scratch buffer is allocated and propagated. I entirely
agree with you on both characteristics of the v1 set: it is a bit more
efficient, and a bit harder to follow, than the alternative you
describe. I think arguments can be made equally well in favor of either
approach.
Unfortunately, I'm again out of time (with downstream responsibilities
rearing their ugly heads -- the rebase is done, but more churn is to
follow). I'll be happy if I can squeeze out a v2 with the above
non-intrusive changes early next week. (I wonder if the mental image of
the upcoming chaos is going to stress me enough to post v2 over the
weekend. Sigh.)
Thank you,
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib
2017-03-10 20:16 ` Laszlo Ersek
@ 2017-03-11 3:17 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2017-03-11 3:17 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01; +Cc: Ard Biesheuvel
On 03/10/17 21:16, Laszlo Ersek wrote:
> On 03/10/17 10:55, Jordan Justen wrote:
>> On 2017-03-06 10:41:31, Laszlo Ersek wrote:
>>> On 03/06/17 10:19, Jordan Justen wrote:
>>>
>>>> Is there any reason we couldn't add a
>>>> pointer within the scatch buffer for reading/writing to? It just
>>>> seems to make the interface a bit more generic/intuitive.
>>>
>>> That's how I initially designed the interface, but while implementing
>>> the functions, I realized there was no good use for that feature.
>>>
>>> The only use case for storing the data read from fw_cfg at a nonzero
>>> offset in the scratch buffer, and for writing data to fw_cfg from a
>>> nonzero offset in the scratch buffer, would be that you have "something
>>> else" at offset zero. What could that thing be?
>>>
>>
>> We discussed this on irc a bit. I agree that we can proceed with your
>> current design.
>>
>> I still think the library interface would be clearer if it had the
>> buffer passed to the read/write routines, but I concede that it would
>> complicate and grow the code for no other gain than a clearer library
>> interface.
>>
>> I think the helper callback adds to the confusion in the library
>> interface, but if you don't have the buffer passed into the read/write
>> routines, then you need the scratch buffer, and then the callback makes
>> sense.
>>
>> I think we could have one helper to 'call me back when boot script is
>> available', but didn't have the scratch allocation aspect. Of course,
>> having torn out all the fw-cfg context, this now seems like something
>> that some generic boot script lib might provide.
>>
>> Then I'd leave it up to the user of the lib to allocate a buffer and
>> pass it into the fw-cfg boot script routines to be logged. The library
>> would also have to allocate the DMA transfer buffer, which is just one
>> way that this would be less efficient.
>>
>> Once again, I agree this would be less efficient, but I think it
>> easier to follow. (Or, at least more intuitive from a library
>> interface perspective.) And, once again, I am okay with you proceeding
>> with your current design.
>
> Thank you very much for taking the time to review this and for the
> suggested improvements. I'd like to post v2 with the following changes:
>
> - rename QemuFwCfgS3TransferOwnership to
> QemuFwCfgS3CallWhenBootScriptReady
>
> - rename FW_CFG_BOOT_SCRIPT_APPEND_FUNCTION to
> FW_CFG_BOOT_SCRIPT_CALLBACK_FUNCTION
>
> - similarly, rename Append to BootScriptCallback
I'll use plain "Callback" here; "BootScriptCallback" would blow up all
of the comment formatting.
Thanks
Laszlo
>
> - rename QemuFwCfgS3Xxx to QemuFwCfgS3ScriptXxx
>
> I wouldn't like to change the following aspects:
>
> - Indentation of function calls -- I've been consistent about those for
> several years, they satisfy the requirements of the coding std, and code
> I wrote like that had been accepted even in MdeModulePkg
> (PciHostBridgeDxe) and MdePkg (BaseOrderedCollectionRedBlackTreeLib).
>
> My main argument against *exclusively* using the one-arg-per-line style
> is that it wastes a lot of vertical space, especially because I'm
> unwilling to write longer than 80char lines. (Those who are willing to
> go up to 120 -- that is, 50% more! -- chars per line have it much
> easier, because they need to break up a *lot* fewer function calls.)
>
> - The way the scratch buffer is allocated and propagated. I entirely
> agree with you on both characteristics of the v1 set: it is a bit more
> efficient, and a bit harder to follow, than the alternative you
> describe. I think arguments can be made equally well in favor of either
> approach.
>
> Unfortunately, I'm again out of time (with downstream responsibilities
> rearing their ugly heads -- the rebase is done, but more churn is to
> follow). I'll be happy if I can squeeze out a v2 with the above
> non-intrusive changes early next week. (I wonder if the mental image of
> the upcoming chaos is going to stress me enough to post v2 over the
> weekend. Sigh.)
>
> Thank you,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 32+ messages in thread