> On Mar 31, 2015, at 6:54 PM, Andrew Fish <af...@apple.com> wrote:
>
>>
>> On Mar 31, 2015, at 6:25 PM, Yao, Jiewen <jiewen....@intel.com
>> <mailto:jiewen....@intel.com>> wrote:
>>
>> Yes. It seems a bug.
>> We should initialize it.
>
> Thanks!
>
> The other thing that looked strange to me was: mS3BootScriptTablePtr and
> mS3BootScriptTableSmmPtr of type SCRIPT_TABLE_PRIVATE_DATA.
> //
> // The boot script private data.
> //
> typedef struct {
> UINT8 *TableBase;
> UINT32 TableLength; // Record the actual memory
> length
> UINT16 TableMemoryPageNumber; // Record the page number
> Allocated for the table
> BOOLEAN AtRuntime; // Record if current state is
> after SmmReadyToLock
> BOOLEAN InSmm; // Record if this library is in
> SMM.
> } SCRIPT_TABLE_PRIVATE_DATA;
>
> Given mS3BootScriptTablePtr is a library global there are N copies of them in
> the system. So TableLength and TableMemoryPageNumber can get out of sync with
> reality. I don’t see the point in tracking the data in N places. Seems like
> all the instances of SCRIPT_TABLE_PRIVATE_DATA.TableBase point to
> EFI_BOOT_SCRIPT_TABLE_HEADER, so that is the only length that means anything.
>
I just noticed that mS3BootScriptTablePtr points to
PcdS3BootScriptTablePrivateDataPtr. So the delta is really only between the DXE
drivers and the SMM drivers. All the DXE Drivers share the same
mS3BootScriptTablePtr, and all the SMM drivers share the same
mS3BootScriptTablePtr. But both point to the same EFI_BOOT_SCRIPT_TABLE_HEADER.
DXE:
(lldb) p *mS3BootScriptTablePtr
(SCRIPT_TABLE_PRIVATE_DATA) $126 = {
TableBase = 0x000000007afbe000 "?"
TableLength = 0x000003d1
TableMemoryPageNumber = 0x0004
AtRuntime = true
InSmm = false
}
(lldb) p *(EFI_BOOT_SCRIPT_TABLE_HEADER *)0x000000007afbe000
(EFI_BOOT_SCRIPT_TABLE_HEADER) $127 = {
OpCode = 0x00aa
Length = 0x0d
Version = 0xafaf
TableLength = 0x000005c4
Reserved = ([0] = 0xafaf, [1] = 0xafaf)
}
SMM:
(lldb) p *(SCRIPT_TABLE_PRIVATE_DATA *)0x7b3efa90
(SCRIPT_TABLE_PRIVATE_DATA) $128 = {
TableBase = 0x000000007afbe000 "?"
TableLength = 0x000005c1
TableMemoryPageNumber = 0x0004
AtRuntime = true
InSmm = true
}
(lldb) p *(EFI_BOOT_SCRIPT_TABLE_HEADER *)0x000000007afbe000
(EFI_BOOT_SCRIPT_TABLE_HEADER) $129 = {
OpCode = 0x00aa
Length = 0x0d
Version = 0xafaf
TableLength = 0x000005c4
Reserved = ([0] = 0xafaf, [1] = 0xafaf)
}
I wonder if the reason it seem to work is some of the writes are the full set
and some writes are too short. So at the end of the day the lock box has the
correct data in it?
It still seems like it would be simpler, and safer to just use
EFI_BOOT_SCRIPT_TABLE_HEADER.TableLength?
Thanks,
Andrew Fish
> It probably generally seems to work as a driver does writes in batches. But
> if a driver did a batch of writes, and then did work in an event. Its cached
> SCRIPT_TABLE_PRIVATE_DATA would be out of data with regards to TableLength
> and TableMemoryPageNumber.
>
> It looks like a bad value in mS3BootScriptTablePtr could end up truncating
> the table. It could be that the later code has a larger value and it runs
> last? So maybe it is a race, or we are just getting lucky? Or maybe there is
> some syncing back from EFI_BOOT_SCRIPT_TABLE_HEADER that I missed in the code.
>
> Thanks,
>
> Andrew Fish
>
> PS I’ve not seen any bad S3 data. I’m writing a debugger script to dump out
> the tables, so I was poking around in memory to debug the script and noticed
> this issue.
>
> (lldb) p *mS3BootScriptTablePtr
> (SCRIPT_TABLE_PRIVATE_DATA) $120 = {
> TableBase = 0x000000007afbe000 "?"
> TableLength = 0x000003d1
> TableMemoryPageNumber = 0x0004
> AtRuntime = true
> InSmm = false
> }
> (lldb) p *(EFI_BOOT_SCRIPT_TABLE_HEADER *)0x000000007afbe000
> (EFI_BOOT_SCRIPT_TABLE_HEADER) $121 = {
> OpCode = 0x00aa
> Length = 0x0d
> Version = 0xafaf
> TableLength = 0x000005c4
> Reserved = ([0] = 0xafaf, [1] = 0xafaf)
> }
>
> The last instance of mS3BootScriptTablePtr is this one:
> (lldb) p *(SCRIPT_TABLE_PRIVATE_DATA *)0x7b3efa90
> (SCRIPT_TABLE_PRIVATE_DATA) $122 = {
> TableBase = 0x000000007afbe000 "?"
> TableLength = 0x000005c1
> TableMemoryPageNumber = 0x0004
> AtRuntime = true
> InSmm = true
> }
>
>
>>
>> Thank you
>> Yao Jiewen
>>
>> -----Original Message-----
>> From: Andrew Fish [mailto:af...@apple.com <mailto:af...@apple.com>]
>> Sent: Wednesday, April 01, 2015 6:32 AM
>> To: edk2-devel@lists.sourceforge.net
>> <mailto:edk2-devel@lists.sourceforge.net>
>> Subject: [edk2] MdeModulePkg Maintainer: The PiDxeS3BootScriptLib does not
>> seem to set EFI_BOOT_SCRIPT_TABLE_HEADER.Version
>>
>> I don’t see any code in the PiDxeS3BootScriptLib that sets a value to
>> EFI_BOOT_SCRIPT_TABLE_HEADER.Version. The data is allocated via
>> gBS->AllocatePages, and on the system I looked at the Version was set to
>> 0xafaf.
>>
>> It seems like the driver should initialize this member to a known value?
>>
>> Thanks,
>>
>> Andrew Fish
>> ------------------------------------------------------------------------------
>> Dive into the World of Parallel Programming The Go Parallel Website,
>> sponsored by Intel and developed in partnership with Slashdot Media, is your
>> hub for all things parallel software development, from weekly thought
>> leadership blogs to news, videos, case studies, tutorials and more. Take a
>> look and join the conversation now. http://goparallel.sourceforge.net/
>> <http://goparallel.sourceforge.net/>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.sourceforge.net <mailto:edk2-devel@lists.sourceforge.net>
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>> ------------------------------------------------------------------------------
>> Dive into the World of Parallel Programming The Go Parallel Website,
>> sponsored
>> by Intel and developed in partnership with Slashdot Media, is your hub for
>> all
>> things parallel software development, from weekly thought leadership blogs to
>> news, videos, case studies, tutorials and more. Take a look and join the
>> conversation now. http://goparallel.sourceforge.net/
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
>
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
