[v1] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

[edk2] [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

Posted by Laszlo Ersek 7 years, 8 months ago

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".)

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

Re: [edk2] [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

Posted by Jordan Justen 7 years, 8 months ago

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

Posted by Laszlo Ersek 7 years, 8 months ago

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

Posted by Jordan Justen 7 years, 8 months ago

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

Posted by Laszlo Ersek 7 years, 8 months ago

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

Posted by Jordan Justen 7 years, 7 months ago

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

Posted by Laszlo Ersek 7 years, 7 months ago

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib

Posted by Laszlo Ersek 7 years, 7 months ago

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
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel