On 2016/4/8 17:50, Laszlo Ersek wrote:
On 04/08/16 09:09, Zeng, Star wrote:
On 2016/4/8 14:55, Yao, Jiewen wrote:
Thanks.
1/3 - I think this is valuable to save SMRAM size. Reviewed-by:
[email protected]
2/3 - I think this is valuable to save Reserved memory size.
Reviewed-by: [email protected]
3/3 - I have concern on this one, because MasterBootMode cannot be
used as indicator for S3 mode or not. It cannot be used as indicator
if this PCD is ready or not. Also, I do not see big benefit on just
saving one PPI data structure. It has no impact to OS. I suggest we
drop it.
If the PcdAcpiS3Enable is declared as FixedAtBuild, platform can switch
out the S3Resume2Pei in *.dsc and *.fdf, then the check has no benefit.
If the PcdAcpiS3Enable is declared as Dynamic, we can save only one PPI
at most as PEI phase has no unload.
Ok, I admit its benefit is small. I will drop this [3/3] patch.
I appreciate that you CC'd me on this; indeed I care a lot about this.
I have no input on patch #1, because OvmfPkg does not include
SmmS3SaveState.
I agree that it's best to drop patch #3, due to the reasons discussed by
Jiewen and you.
Patch #2 is relevant for OVMF:
In the SMM-less build, I expect patch #2 not to cause any difference in
behavior. Namely, in the SMM-less build of OVMF, BootScriptExecutorDxe's
dependency on the LockBox protocol GUID is not satisfied anyway
(intentionally), when S3 is disabled, so any change to the entry point
function is invisible in that case.
In the SMM-ful build, I expect patch #2 to have an impact, when S3 is
disabled, yes. The LockBox protocol is produced by the SMM LockBox
protocol driver in any case, so the depex is satisfied, and
BootScriptExecutorDxe is dispatched (although never used by the rest of
OVMF). Patch #2 should make BootScriptExecutorDxe bail out early, but
the rest of OVMF shouldn't care (only be happy about the saved memory).
.... Yes, that's what happens.
I tested the following cases (and compared the logs & behavior),
without patch #2, and then with patch #2:
guest OS SMM S3 OVMF QEMU tests done
enabled enabled platform machine type
-------- ------- ------- ---------- ------------ ---------------
Fedora no no X64 pc normal boot
Fedora no yes X64 pc normal boot, S3
Fedora yes no Ia32X64 q35 normal boot
Fedora yes yes Ia32X64 q35 normal boot, S3
Nothing broke, and the logs make sense, so I'm okay with patch #2.
For patch #2:
Tested-by: Laszlo Ersek <[email protected]>
Reviewed-by: Laszlo Ersek <[email protected]>
Really appreciate your review and test effort.
Thanks,
Star
Thanks!
Laszlo
Thanks,
Star
Thank you
Yao Jiewen
-----Original Message-----
From: Zeng, Star
Sent: Friday, April 8, 2016 2:28 PM
To: [email protected]
Cc: Tian, Feng <[email protected]>; Yao, Jiewen
<[email protected]>;
Fan, Jeff <[email protected]>; Laszlo Ersek <[email protected]>
Subject: [PATCH 0/3] Update some S3 related modules to consume
PcdAcpiS3Enable to control the code
Cc: Feng Tian <[email protected]>
Cc: Jiewen Yao <[email protected]>
Cc: Jeff Fan <[email protected]>
Cc: Laszlo Ersek <[email protected]>
Laszlo, I guess you have interest.
Star Zeng (3):
MdeModulePkg SmmS3SaveStateDxe: Consume PcdAcpiS3Enable to
control the
code
MdeModulePkg BootScriptExecutorDxe: Consume PcdAcpiS3Enable to
control
the code
UefiCpuPkg S3Resume2Pei: Consume PcdAcpiS3Enable to control the
code
.../Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf | 3
++-
MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c |
6 +++++-
MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.c
| 7 +++++--
MdeModulePkg/Universal/Acpi/SmmS3SaveState/SmmS3SaveState.inf
| 6 +++++-
UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c
| 6 +++++-
UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf
| 5 +++--
6 files changed, 25 insertions(+), 8 deletions(-)
--
2.7.0.windows.1
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
