Hi, On 12/27/13 08:06, Yao, Jiewen wrote: > Hi > I am not clear why you think EDKII S3 support requires SMM. > > Per current design EDKII S3 support only requires lockbox, because boot > script will be saved there. > > SMM is just one possible implementation for lockbox. > > If OVMF has another way to create lockbox, it does not need SMM.
I think this is incorrect. The patches in the series (which is now at v2) seek to list the dependencies carefully in the commit messages, justifying why every piece that is pulled is necessary. The series is built in a bottom-up fashion, ie. first providing lower level building blocks and then building on those blocks. So the dependencies are not always direct and/or trivial to see. But, the dependencies on SMM are there: - In patch "OvmfPkg: S3 Suspend: enable creation/saving of an S3 Boot Script": The PiDxeS3BootScriptLib library instance (for library class S3BootScriptLib) depends on EFI_SMM_BASE2_PROTOCOL directly. - In patch "OvmfPkg: S3 Resume: pull in PEIM orchestrating S3 Resume": The EFI_PEI_S3_RESUME2 PEIM implementation (= S3Resume2Pei) depends on PEI_SMM_ACCESS_PPI directly. Also, as far as I could see, the LockBox interface had not been standardized (either PI or UEFI). For me to cut the dependency chain at LockBox and implement it would have been completely arbitrary, and therefore a risk of wasting work. (Honestly, the idea never came to me before you mentioned it.) On the other hand, the SMM protocols looked standard and therefore a good boundary to cut at. Then, > -----Original Message----- > From: Jordan Justen [mailto:jljus...@gmail.com] > Sent: Friday, December 27, 2013 2:38 PM > To: Laszlo Ersek > Cc: edk2-devel@lists.sourceforge.net > Subject: Re: [edk2] [RFC 00/15] S3 suspend/resume for OVMF (WIP) > > On Tue, Dec 3, 2013 at 12:23 AM, Laszlo Ersek <ler...@redhat.com> wrote: >> On 12/02/13 22:42, Jordan Justen wrote: >>> Mike, >>> >>> Do you think it would be better for OVMF to add EMU SMM modules, or >>> to fix the core S3 modules to not require SMM? >> >> Yes, that's the crucial question. My approach was to use whatever's >> in-tree and provide shims for the missing protocols etc. >> >> FWIW I agree with Eugene too -- the ~40 hours I've spent on this since >> Friday evening tell me (maybe incorrectly) that "fixing" the S3 >> modules not to require SMM looks rather like "rearchitecting" them. > > I'm sure you've spent considerably more time on this now. Correct, I've been working on this close to full-time since end of November / start of December. > Are you still convinced that a 'fake' SMM is the best way to go? The 'fake' > SMM support you've developed for OVMF certainly doesn't seem trivial. Yes, I'm convinced. Actually, after spending the effort and seeing that it works with both Linux and Windows guests in practice, I'm more convinced than ever :) As I wrote above, "My approach was to use whatever's in-tree and provide shims for the missing protocols". It was hard to find all the modules and to understand their inter-dependencies to a sufficient extent, but the size of the *new* OVMF code that the series adds for satisfying SMM requirements would not go down significantly if we "cut" the abstraction at some other point and faked some other system component (like LockBox). - The entire reservation stuff would have to happen anyway, with or without SMM. (a) There are memory areas that OVMF overwrites at startup no matter what, (b) however we implemented LockBox, we'd need RAM at some known location to hang the entire data tree upon. - The OVMF-specific differentiation between the S3 and the full-config boot paths (eg. placement of permanent PEI memory) is independent of SMM. - Something like EmuNvramLib would be necessary to make sure all components (SEC, PEI, DXE) understand which reserved region is where. EmuNvramLib only sums a few numbers based on a few PCDs, and if S3 is disabled on the qemu side, it returns zeros for sizes (I documented that case clearly as "disabled"). - If S3 is disabled via qemu configuration, the RAM reservations that are required only for S3 implementation should go away. This is again independent of SMM, so we couldn't save code here. - The SMM abstractions (DXE and PEI drivers) are not big, they're just shims and expose one of the reserved areas. They have clear, official interface contracts too. If we implemented another LockBox library instance, this code would be replaced by code that *included* the same functionality (access to reserved area) but our new LockBoxLib would also have to implement (= duplicate) the lockbox-specific features. - I agree that DiscloseSmstSmm is ugly. But it's also tiny. - Copying and specializing AcpiS3SaveDxe is unavoidable. That driver / protocol is where IntelFrameworkPkg's BDS code yanks the prepare-for-S3 chain. The original AcpiS3SaveDxe has a wart that is inappropriate for us (dependency on MP services), and it is also the *only* place inside of which we can save the boot script and emit/install EFI_DXE_SMM_READY_TO_LOCK_PROTOCOL. Please see the patches for details. I agree that the complexity of existent edk2 code that the patchset pulls into OVMF, and that of the *data flow* is not optimal. I tried to mitigate that with documentation (both explicit docs and detailed commit messages). Eg. the high-level data flow is documented in "OvmfPkg: PlatformPei: detect S3 Resume in CMOS and set boot mode accordingly". > It also seems likely to add confusion for anyone trying to understand the > code. It took me weeks to develop a working understanding of the pulled-in code. (*) But, again, I assumed this code worked in practice, and I wanted to reuse it as much as possible. (*) Anyway, that's standard for anyone new to edk2. Consider for example that all important functions are called through protocols / function pointers, and you must keep grepping the source for protocol GUIDs to find the callees. This invariably and immediately trips up everyone who tries to follow function calls by name. > I guess I'm just frustrated that EDK II's S3 support would require > those SMM interfaces. Guess how frustrated I was when I realized that :) > I can see taking advantage of SMM when it is > available... I'll admit I'm a biased person to ask about this. I've already spent the effort and the result apparently works, and it tends to reuse as much as I could manage from the edk2 tree, giving us future fixes too without much backporting burden, hopefully. We could only be sure about the existence of a better alternative if we actually wrote it, and honestly I don't feel attracted to that avenue :) At least not right now -- I've got something of a deadline here too. Your point of view as upstream maintainer may be understandably different, of course. I also won't pretend that reviewing the series won't be a pain. Thanks, Laszlo ------------------------------------------------------------------------------ Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro! http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
