public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Jordan Justen <jordan.l.justen@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
	edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib
Date: Mon, 06 Mar 2017 01:19:52 -0800	[thread overview]
Message-ID: <148879199242.27619.7627489493363959501@jljusten-skl> (raw)
In-Reply-To: <20170223014814.10937-1-lersek@redhat.com>

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


  parent reply	other threads:[~2017-03-06  9:19 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 ` [PATCH 03/12] OvmfPkg/QemuFwCfgS3Lib: add initial PEI and DXE fw_cfg library instances Laszlo Ersek
2017-02-23  1:48 ` [PATCH 04/12] ArmVirtPkg: resolve QemuFwCfgS3Lib Laszlo Ersek
2017-02-23 11:18   ` Ard Biesheuvel
2017-02-23  1:48 ` [PATCH 05/12] OvmfPkg: " Laszlo Ersek
2017-02-23  1:48 ` [PATCH 06/12] ArmVirtPkg, OvmfPkg: retire QemuFwCfgS3Enabled() from QemuFwCfgLib 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
2017-03-10  9:58   ` Jordan Justen
2017-03-10 19:14     ` Laszlo Ersek
2017-02-23  1:48 ` [PATCH 08/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for Base Null instance Laszlo Ersek
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 ` [PATCH 10/12] OvmfPkg/QemuFwCfgS3Lib: implement opcode APIs for DXE " Laszlo Ersek
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
2017-03-13 21:32       ` Jordan Justen
2017-03-13 22:12         ` Laszlo Ersek
2017-03-13 22:43           ` Jordan Justen
2017-03-14  1:58             ` Kinney, Michael D
2017-03-14 13:48               ` Laszlo Ersek
2017-03-14 20:38                 ` Laszlo Ersek
2017-02-23  1:48 ` [PATCH 12/12] OvmfPkg/AcpiPlatformDxe: " Laszlo Ersek
2017-02-23 16:59 ` [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib Jordan Justen
2017-02-23 17:22   ` Laszlo Ersek
2017-03-06  9:19 ` Jordan Justen [this message]
2017-03-06 18:41   ` Laszlo Ersek
2017-03-10  9:55     ` Jordan Justen
2017-03-10 20:16       ` Laszlo Ersek
2017-03-11  3:17         ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=148879199242.27619.7627489493363959501@jljusten-skl \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox