Hi!
On 12/03/13 16:03, Yao, Jiewen wrote:
> 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)
I disagree (and I tested it too). My example code goes like:
> EFI_STATUS Status;
> EFI_S3_SAVE_STATE_PROTOCOL *BootScript;
> STATIC CONST UINT8 Info[] = { 0xDE, 0xAD, 0xBE, 0xEF };
>
> Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, NULL,
> (VOID **) &BootScript);
> ASSERT_EFI_ERROR (Status);
>
> //
> // Despite the opcode documentation in the PI spec, the protocol
> // implementation embeds a deep copy of the info in the boot script, rather
> // than storing just a pointer to runtime or NVS storage.
> //
> Status = BootScript->Write(BootScript, EFI_BOOT_SCRIPT_INFORMATION_OPCODE,
> (UINT32) sizeof Info,
> (EFI_PHYSICAL_ADDRESS)(UINTN) &Info);
> ASSERT_EFI_ERROR (Status);
Normally this would be a violation of the protocol, because the pointer
I'm passing to BootScript->Write() doesn't point to runtime memory or
ACPI NVS.
The implementation behind BootScript->Write() is the BootScriptWrite()
function in "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c".
Relevant part:
> case EFI_BOOT_SCRIPT_INFORMATION_OPCODE:
> VA_START (Marker, OpCode);
> Status = BootScriptWriteInformation (Marker);
> VA_END (Marker);
> break;
The BootScriptWriteInformation() function says (still in the same file):
> EFI_STATUS
> BootScriptWriteInformation (
> IN VA_LIST Marker
> )
> {
> UINT32 InformationLength;
> EFI_PHYSICAL_ADDRESS Information;
>
> InformationLength = VA_ARG (Marker, UINT32);
> Information = VA_ARG (Marker, EFI_PHYSICAL_ADDRESS);
> return S3BootScriptSaveInformation (InformationLength,
> (VOID*)(UINTN)Information);
> }
The retrieval of "InformationLength" and "Information" from the stack
via the outer ellipsis / inner VA_LIST notation is correct.
When calling S3BootScriptSaveInformation(), "Information" is converted
to a pointer type. I agree that it is not dereferenced (yet), but this
is already wrong, because the caller is entitled to pass in a 64-bit
EFI_PHYSICAL_ADDRESS, and by converting it to UINTN (and then to VOID*)
we could be truncating it, if the DXE phase is 32-bit.
Anyway, this is not the main issue; let's suppose that the pointer
passed to S3BootScriptSaveInformation() still carries the full
EFI_PHYSICAL_ADDRESS.
Then, in file
"MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c":
> RETURN_STATUS
> EFIAPI
> S3BootScriptSaveInformation (
> IN UINT32 InformationLength,
> IN VOID *Information
> )
> {
> UINT8 Length;
> UINT8 *Script;
> EFI_BOOT_SCRIPT_INFORMATION ScriptInformation;
>
> Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_INFORMATION) + InformationLength);
>
> Script = S3BootScriptGetEntryAddAddress (Length);
> if (Script == NULL) {
> return RETURN_OUT_OF_RESOURCES;
> }
> //
> // Build script data
> //
> ScriptInformation.OpCode = EFI_BOOT_SCRIPT_INFORMATION_OPCODE;
> ScriptInformation.Length = Length;
>
>
> 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);
>
> SyncBootScript (Script);
>
> return RETURN_SUCCESS;
>
> }
With my example in mind, the Length local variable is set to
sizeof (EFI_BOOT_SCRIPT_INFORMATION) + 4
The "ScriptInformation" structure contains:
OpCode = EFI_BOOT_SCRIPT_INFORMATION_OPCODE
Length = sizeof (EFI_BOOT_SCRIPT_INFORMATION) + 4
InformationLength = 4
The first CopyMem() call copies this header to the beginning of the new
entry in the boot script.
The second CopyMem() call copies data into the same new entry, right
behind the header. However, it does not copy the address that has been
passed in -- it dereferences the address and copies the data. In my
case, it copies the four bytes { 0xDE, 0xAD, 0xBE, 0xEF } into the
entry.
When replaying this entry after resume, the boot script executor calls
into the same library, function S3BootScriptExecute(), file
"MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptExecute.c":
S3BootScriptExecute()
BootScriptExecuteInformation()
> VOID
> BootScriptExecuteInformation (
> IN UINT8 *Script
> )
>
> {
> UINT32 Index;
> EFI_BOOT_SCRIPT_INFORMATION Information;
> UINT8 *InformationData;
>
> CopyMem ((VOID*)&Information, (VOID*)Script,
> sizeof(EFI_BOOT_SCRIPT_INFORMATION));
>
> InformationData = Script + sizeof (EFI_BOOT_SCRIPT_INFORMATION);
> DEBUG ((EFI_D_INFO, "BootScriptExecuteInformation - 0x%08x\n", (UINTN)
> InformationData));
>
> DEBUG ((EFI_D_INFO, "BootScriptInformation: "));
> for (Index = 0; Index < Information.InformationLength; Index++) {
> DEBUG ((EFI_D_INFO, "%02x ", InformationData[Index]));
> }
> DEBUG ((EFI_D_INFO, "\n"));
> }
The replay function for the INFORMATION opcode first copies out the
opcode header from the script entry. It finds the start of the
information after the header. Then it dumps the data included in the
entry, right after the opcode header. In my case it reads:
> S3BootScriptExecute:
> TableHeader - 0x9C767000
> TableHeader.TableLength - 0x0000001B
> ExecuteBootScript - 9C76700D
> EFI_BOOT_SCRIPT_INFORMATION_OPCODE
> BootScriptExecuteInformation - 0x9C767014
> BootScriptInformation: DE AD BE EF
> ExecuteBootScript - 9C767018
> S3_BOOT_SCRIPT_LIB_TERMINATE_OPCODE
> S3BootScriptDone - Success
The bytes
DE AD BE EF
are the contents of my array (saved still before the suspend). They are
not the address of the array.
> NOTE: PI spec does not say anything on boot script format in memory,
> this is implementation specific.
I agree. But the implementation should save the pointer that I pass in,
not the pointed-to data.
> 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.
Ah, that's actually a good reason. It deviates from the spec, but I can
understand the idea.
> Personally, I believe this implementation still follows PI spec, since
> Boot Script format is NOT defined by PI spec.
It's not the format but how it works. Consider the following: according to the
spec,
I should be able to:
(1) allocate an area in Acpi NVS memory,
(2) zero it out,
(3) push the address to the boot script with an INFO opcode,
(4) *change* the contents of the area.
(5) suspend and resume.
According to the spec, after resumption I should see the contents
written in (4), not the zeroes written in (2).
> "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.
You would have to change the internal header format to do that.
Currently EFI_BOOT_SCRIPT_INFORMATION.Length is just an UINT8.
> S3 module does use InformationLength field in current implementation.
> Would you please point out why it is "redundant"?
Because of these assignments in S3BootScriptSaveInformation():
> Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_INFORMATION) + InformationLength);
> ScriptInformation.Length = Length;
> ScriptInformation.InformationLength = InformationLength;
The Length and the InformationLength fields differ only in a constant.
The EFI_BOOT_SCRIPT_INFORMATION header structure is actually correct, it
is just not used correctly. The current code does this:
+--------+--------+--------+--------+--------+--------+--------+--------------
... ---------------+
| OpCode | Length | InformationLength | deep copy of
user specified data |
+--------+--------+--------+--------+--------+--------+--------+--------------
... ---------------+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^|^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
sizeof (EFI_BOOT_SCRIPT_INFORMATION) bytes |
InformationLength bytes
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Length = sizeof (EFI_BOOT_SCRIPT_INFORMATION) +
InformationLength bytes
The Length and InformationLength fields carry the same information, you
can derive one from the other, in either direction. This is why one is
redundant.
The intended use would look like:
+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+
| OpCode | Length | InformationLength |
EFI_PHYSICAL_ADDRESS |----+
+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+
|
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^|^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
sizeof (EFI_BOOT_SCRIPT_INFORMATION) bytes |
8 bytes exactly |
|
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
Length = sizeof (EFI_BOOT_SCRIPT_INFORMATION) + 8 == 7 + 8
== 15 bytes, always |
|
|
+--------- ... -----------+
|
| User data in ACPI NVS |
<---------------------------------------------------------------------------------------------------------------+
+--------- ... -----------+
^^^^^^^^^^^^^^^^^^^^^^^^^
InformationLength bytes
The replay function should check (using the Length field) that the
payload is exactly the size of EFI_PHYSICAL_ADDRESS. Then it should
CopyMem() the payload out into an EFI_PHYSICAL_ADDRESS variable, convert
that to a pointer, and *dereference* it. And dump the data that's there.
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
