From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 82F358032A for ; Mon, 6 Mar 2017 10:41:34 -0800 (PST) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 06EA43B717; Mon, 6 Mar 2017 18:41:35 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-77.phx2.redhat.com [10.3.117.77]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v26IfXJ2011080; Mon, 6 Mar 2017 13:41:33 -0500 To: Jordan Justen , edk2-devel-01 References: <20170223014814.10937-1-lersek@redhat.com> <148879199242.27619.7627489493363959501@jljusten-skl> Cc: Ard Biesheuvel From: Laszlo Ersek Message-ID: <3e3242e1-2578-3b14-636e-58d8000b41da@redhat.com> Date: Mon, 6 Mar 2017 19:41:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <148879199242.27619.7627489493363959501@jljusten-skl> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Mon, 06 Mar 2017 18:41:35 +0000 (UTC) Subject: Re: [PATCH 00/12] ArmVirtPkg, OvmfPkg: factor out QemuFwCfgS3Lib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Mar 2017 18:41:34 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 >> Cc: Jordan Justen >> >> 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