Hi Laszlo "According to the PI spec, Information is a pointer pointing to runtime memory or ACPI NVS, and the BootScript record will store the pointer (ie. not the data pointed-to)." I agree with you on first part - According to the PI spec, Information is a pointer pointing to runtime memory or ACPI NVS. And I hold different view on second part - the BootScript record will store the pointer (ie. not the data pointed-to)
NOTE: PI spec does not say anything on boot script format in memory, this is implementation specific. For security reason, BIOS S3 module will try its best to save the all S3 data to lockbox, so current S3 module implementation purposely save the information part into boot script and into lockbox later. Personally, I believe this implementation still follows PI spec, since Boot Script format is NOT defined by PI spec. "The current code also limits the size of the information to less than 255 bytes, and makes the InformationLength argument redundant." I agree with you on first part - The current code also limits the size of the information to less than 255 bytes. And I hold different view on second part - makes the InformationLength argument redundant. Right, current implementation does have such limitation. If you have concern, I agree to fix 255 bytes limitation. S3 module does use InformationLength field in current implementation. Would you please point out why it is "redundant"? Thank you Yao Jiewen -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Tuesday, December 03, 2013 7:08 PM To: edk2-devel@lists.sourceforge.net Subject: [edk2] Save State protocol violation in S3SaveState.c (MdeModulePkg) Probably a known bug that cannot be fixed for compatibility reasons: EFI_BOOT_SCRIPT_INFORMATION_OPCODE takes two arguments, "InformationLength" and "Information". According to the PI spec, Information is a pointer pointing to runtime memory or ACPI NVS, and the BootScript record will store the pointer (ie. not the data pointed-to). However the implementation in BootScriptWriteInformation() MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c and in the underlying library function S3BootScriptSaveInformation() MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c dereference the pointer when the opcode is added, and the pointed-to data is copied into the script. This is no problem per se, it's just not what the spec says. The spec would need something like: diff --git a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c index c087dd9..655496f 100644 --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c @@ -1449,14 +1449,14 @@ RETURN_STATUS EFIAPI S3BootScriptSaveInformation ( IN UINT32 InformationLength, - IN VOID *Information + IN EFI_PHYSICAL_ADDRESS Information ) { UINT8 Length; UINT8 *Script; EFI_BOOT_SCRIPT_INFORMATION ScriptInformation; - Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_INFORMATION) + InformationLength); + Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_INFORMATION) + sizeof Information); Script = S3BootScriptGetEntryAddAddress (Length); if (Script == NULL) { @@ -1472,7 +1472,7 @@ S3BootScriptSaveInformation ( ScriptInformation.InformationLength = InformationLength; CopyMem ((VOID*)Script, (VOID*)&ScriptInformation, sizeof (EFI_BOOT_SCRIPT_INFORMATION)); - CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_INFORMATION)), (VOID *) Information, (UINTN) InformationLength); + CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_INFORMATION)), &Information, sizeof Information); SyncBootScript (Script); diff --git a/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c b/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c index 60cd9b1..70e206f 100644 --- a/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c +++ b/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c @@ -418,7 +418,7 @@ BootScriptWriteInformation ( InformationLength = VA_ARG (Marker, UINT32); Information = VA_ARG (Marker, EFI_PHYSICAL_ADDRESS); - return S3BootScriptSaveInformation (InformationLength, (VOID*)(UINTN)Information); + return S3BootScriptSaveInformation (InformationLength, Information); } /** Internal function to add IO poll opcode node to the table The replay code would need the corresponding change. The current code also limits the size of the information to less than 255 bytes, and makes the InformationLength argument redundant. 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=84349351&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ 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=84349351&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
