Warning: long and messy email ahead.
On 06/24/15 15:37, Ard Biesheuvel wrote:
> On 24 June 2015 at 14:51, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 06/24/15 13:28, Ard Biesheuvel wrote:
>>> On 24 June 2015 at 13:15, Olivier Martin <olivier.mar...@arm.com>
>>> wrote:
>>>> I think this patch should move to
>>>> IntelFrameworkModulePkg/Universal/BdsDxe. It is a PI specification
>>>> requirement to signal gEfiEndOfDxeEventGroupGuid at the end of the
>>>> DXE phase.
>>>
>>> Yes, that makes sense, I guess. But assuming most of its users are
>>> signalling the event at some point, this would result in the event
>>> to be signalled twice.
>>> Perhaps Jeff can comment here?
>>
>> This topic has been raised several times before (without any
>> solution).
>>
>> I agree that signalling the event is a good idea.
>>
>
> So does that mean OVMF does not call it either?
Yes, it does mean that.
If OVMF had been signaling the event when I worked on ArmVirtPkg's
PlatformIntelBdsLib in Jan/Feb this year, then the latter would also be
signaling the event now. :)
> I tried grep'ing for
> instances but I could only find the one in the ARM BDS
Right. That's a good sign of the problem. No central location that does
it, nor a pattern that one could derive from several platforms.
>
>> I also agree that
>>
>> (a) either it should be done in
>> "IntelFrameworkModulePkg/Universal/BdsDxe", or
>>
>> (b) since PlatformBdsLib instances are linked into that driver, it
>> should be codified as a rule, perhaps in
>>
>> IntelFrameworkModulePkg/Include/Library/PlatformBdsLib.h
>>
>> that function FOO of the library instance is *responsible* for
>> signaling the event.
>>
>
> I'd be interested in understanding how it is called by the platforms
> people actually work with. For instance, the v2.5 properties table
> feature clearly relies on it, so implementing and testing that must
> have occurred on an out-of-tree platform that issues this call
> *somewhere*
Rest assured that all of the proprietary forks signal the event. :) I
just wish the guidelines were codified somewhere. (Or, well, a rule that
"there is no rule".)
>
>> What I would not like to see is platforms signaling the event in
>> wildly different places. (Unless, of course, that flexibility was an
>> explicit design choice!) Obviously, I'm thinking that OvmfPkg and
>> ArmVirtPkg should behave similarly in this context.
>>
>
> Agreed.
>
>> So I think we need some guidance here, from the inventors of the
>> event, and from the implementors of
>> "IntelFrameworkModulePkg/Universal/BdsDxe".
>>
>> FWIW, how about this: when the DXE core is done dispatching, it
>> decides it is time to enter BDS. (See "gEfiBdsArchProtocolGuid" in
>> "IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf".) Why doesn't
>> the DXE core itself signal end-of-dxe?
>>
>
> If it's a PI spec requirement to do so, then I agree it makes sense to
> put it in DXE core.
>
>> It is true that BDS might connect a number of devices, making
>> available further drivers on them; but those drivers are considered
>> 3rd party (ie. UEFI_DRIVER) modules, not part of the platform in the
>> strict sense.
>>
>
> By that logic, the DXE phase only ever terminates at
> ExitBootServices(),
Correct.
> which is probably not what was intended.
Hm... I just remembered something... Right, this is a mess. Please
search for, and download, the following whitepaper:
A Tour Beyond BIOS
Implementing S3 Resume with EDKII
Refer to section "Normal Boot Flow for S3":
[...]
There are 2 important notes:
* PlatformBDS need 1) call AcpiS3->S3Save() to save S3 system
information, then 2) signal EndOfDxe to give DXE silicon driver
last chance to save boot script, and finally 3) signal
SmmReadyToLock to close boot script and save boot script to
LockBox. These calls must be in this order, or the boot script
might not be able to save correctly.
[...]
OVMF implements S3 -- and after I post my SMM patches, OVMF's S3 will be
complete with SMM stuff --, but OVMF does not implement step (2) from
the above.
It is not a problem in practice: we have no "DXE silicon driver".
Although we do have a boot script, it does nothing (it just contains an
informational opcode that is printed during S3 resume). We don't need
any drivers to extend the boot script; PlatformPei runs during S3 resume
too -- in a custom way -- and re-sets chipset state, and the rest is up
to the OS. Works fine, so lack of step (2) is not a problem for us,
considering just S3 resume.
However, steps (1) and (3) are crucial even for OVMF, and we had to jump
through some hoops to get them right. (I'm stating this in retrospect:
when we were working on S3 for OVMF, this whitepaper didn't exist yet.)
Please refer to:
https://github.com/tianocore/edk2/commit/5a217a06
I'm mentioning those "hoops" because they enlighten the problem with
End-of-Dxe -- the same issue plagues step (3) as well, not just step
(2).
Namely, if you wanted to signal End-of-Dxe yourself, then according to
the quote from the whitepaper, it should be step (2), ie. occur *after*
the AcpiS3->S3Save() call. The problem is that the Intel BDS driver
performs that call itself, in
"IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c", function
BdsLibBootViaBootOption():
2243 //
2244 // Notes: this code can be remove after the s3 script table
2245 // hook on the event EVT_SIGNAL_READY_TO_BOOT or
2246 // EVT_SIGNAL_LEGACY_BOOT
2247 //
2248 Status = gBS->LocateProtocol (&gEfiAcpiS3SaveProtocolGuid, NULL,
(VOID **) &AcpiS3Save);
2249 if (!EFI_ERROR (Status)) {
2250 AcpiS3Save->S3Save (AcpiS3Save, NULL);
2251 }
Therefore, if you want to signal End-of-Dxe, you must do it *after* this
call. However, the function that perfoms the call,
BdsLibBootViaBootOption(), might not return at all to your
PlatformBdsLib instance -- because it boots the OS. Simply put, there
may be no "after".
And, you're not allowed to signal End-of-Dxe *before*, because then you
would violate the ordering set forth in the whitepaper.
Hence the trick in commit 5a217a06: we perform step (3) *inside*
AcpiS3Save->S3Save() -- ie. inside step (1) --, at the end of that
function. (Search the patch for "SaveS3BootScript".)
So the root issue here is that BdsLibBootViaBootOption() does something
it *probably* shouldn't. (Refer to the code comments again.) It
seriously restricts where a platform (doing S3) can actually signal
End-of-Dxe.
... I think this analysis enumerates our options:
- Fix the bug in BdsLibBootViaBootOption(); it should not call
AcpiS3Save->S3Save(). That should be left to PlatformBdsLib. Then we
can talk about where to signal End-of-Dxe in PlatformBdsLib as well,
with any flexibility.
- Or, OVMF will have to signal End-of-Dxe within the SaveS3BootScript()
function (see again the patch), right before installing
"gEfiDxeSmmReadyToLockProtocolGuid". Only that would place the new
step (2) between existent steps (1) and (3).
In parallel, ArmVirtPkg, which doesn't implement S3 or SMM, could
signal End-of-Dxe wherever it wanted to. Meaning, your patch is
correct (assuming it doesn't break anything in practice), *if* we
prefer this option.
- Or, OVMF could ignore the relative ordering requirement between steps
(1) and (2) -- because we don't have a DXE silicon driver that appends
to the S3 boot script --, ie. OVMF could signal End-of-Dxe somewhere
in PlatformBdsLib, before BdsLibBootViaBootOption() is reached. This
would leave OVMF with an order of (2), (1), (3), but in practice that
could still work.
Your patch would be correct for ArmVirtPkg (same as in the previous
option), but unlike in the previous option, OvmfPkg could actually
follow ArmVirtPkg's pattern (and signal End-of-Dxe in
PlatformBdsInit(), for example).
I think #1 is unlikely to happen, and I'm worried about the deliberate
ordering violation in #3.
#2 is ugly as heck, but we're ugly already. For ArmVirtPkg we could go
with your patch, and I could experiment with #2 for OvmfPkg.
Thanks
Laszlo
------------------------------------------------------------------------------
Monitor 25 network devices or servers for free with OpManager!
OpManager is web-based network management software that monitors
network devices and physical & virtual servers, alerts via email & sms
for fault. Monitor 25 devices for free with no restriction. Download now
http://ad.doubleclick.net/ddm/clk/292181274;119417398;o
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
