[edk2-devel] Possible [BUG] PeiCore: HOBs after cache-as-RAM teardown

[Michael Kubacki]

Hi Benjamin,

Based on what you've described, I'll try to provide my best guess as to 
what is happening without actually debugging it. Ultimately, I will need 
to submit a patch to update PeiSerialPortLibSpiFlash.

Are you're aware, gPchSpiPpiGuid is installed in 
Silicon/Intel/KabylakeSiliconPkg/Pch/Library/PeiSpiLib/PeiSpiLib.c:

     PeiSpiInstance = (PEI_SPI_INSTANCE *) AllocateZeroPool (sizeof 
(PEI_SPI_INSTANCE));
     if (NULL == PeiSpiInstance) {
       return EFI_OUT_OF_RESOURCES;
     }

     SpiInstance = &(PeiSpiInstance->SpiInstance);
     SpiProtocolConstructor (SpiInstance);

     PeiSpiInstance->PpiDescriptor.Flags = EFI_PEI_PPI_DESCRIPTOR_PPI | 
EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST;
     PeiSpiInstance->PpiDescriptor.Guid = &gPchSpiPpiGuid;
     PeiSpiInstance->PpiDescriptor.Ppi = &(SpiInstance->SpiProtocol);

     Status = PeiServicesInstallPpi (&PeiSpiInstance->PpiDescriptor);

Note that this PPI descriptor and the PPI interface structure are 
allocated via a pool allocation on the T-RAM heap. The allocation 
contains the descriptor followed by the interface structure. There's 
several pointers set within these structures.

In the descriptor, the GUID pointer is set to the GUID value within the 
image (not the heap) and the PPI interface pointer is set to the 
interface structure in the same pool allocation in the heap. This 
interface structure contains function pointers for SPI flash operations 
as defined in the PCH_SPI_PROTOCOL structure.

The function pointers are set in SpiProtocolConstructor() .
Silicon/Intel/KabylakeSiliconPkg/Pch/LibraryPrivate/BasePchSpiCommonLib/SpiCommon.c

Those function addresses are to image addresses at their XIP address in 
T-RAM.

Starting from the heap, I suspect:

1. After PeiInstallPeiMemory () is called, PeiCheckAndSwitchStack() will 
eventually get called by the dispatcher and see the stack switch signal 
is now TRUE.
2. The heap and stack migration will occur. New heap offsets will be 
calculated. Note that pointers to structures in the heap will be updated 
after PeiCore() reentry.
3. PeiCore() will be reentered using the new permanent memory stack.
4. In this entry, ShadowedPeiCore is NULL. So the T-RAM heap and stack 
are still around, the permanent memory heap and stack are known and 
setup, but the PeiCore image is still executing at its T-RAM address.
5. The private pointer to the HOB list in the permanent memory heap (in 
addition to other lists in the heap like PPI lists) are updated.
6. Eventually, ConvertPpiPointers() will convert pointers in the PPI 
list to their permanent memory locations.
    - This includes the PPI descriptor, PPI GUID, and PPI interface 
pointers.
    - These should include pointers into T-RAM memory allocations, heap, 
and stack.
      - Note: PPI structures initialized as global variables are not 
handled. That is what PcdMigrateTemporaryRamFirmwareVolumes helps 
address. Not particularly relevant in this specific scenario.
        - Further note: For most platforms that execute from flash 
mapped as MMIO, these address should still resolve as code fetches over 
the SPI bus.
    - Anyway, for gPchSpiPpiGuid, I suspect:
      1. The PPI list pointer now points to the PPI descriptor in 
permanent memory (updated)
      2. The PPI descriptor pointer now points to the descriptor in 
permanent memory (migrated/updated)
      3. The PPI GUID pointer still points to the GUID at the XIP 
address (not migrated/update attempted)
      4. The PPI interface pointer now points to the interface structure 
in the permanent memory heap (migrated/updated)
         - The function pointers within the struct still point to the 
functions at their XIP addresses (not migrated/not updated)
      5. The HOB gSpiFlashDebugHobGuid, has been copied to the permanent 
memory heap (with other heap contents) and the HOB heap pointer was updated.
    - [other actions will obviously happen but I don't think will 
significantly affect the outcome for this situation]

Although, a PPI notify was registered in PeiSerialPortLibSpiFlash on 
gPchSpiPpiGuid. The PPI was not actually explicitly reinstalled (that I 
can find in the code) so the HOB retains the T-RAM pointers which work 
until NEM is disabled (because the PPI descriptor was in the T-RAM heap 
not backed by MMIO so it can't be fetched again).

Even if the PPI was explicitly reinstalled, with this code printing 
debug messages, the gap is too far between when a message will be 
printed and the dispatcher would invoke notifications.

For something in the debug stack especially, it was a poor 
implementation choice on my part to cache the PPI pointer in the HOB. 
Additionally, this is a library so multiple HOB instances could be 
produced which is undesirable. In conclusion, I will send a patch to 
update PeiSerialPortLibSpiFlash.

Regarding, the test point checks, are you sure the HOB is actually 
created? Were you able to successfully get the HOB at any point after it 
should have been created? For example, it looks like there's several 
preliminary checks in TestPointLibSetTable() that could prevent 
publishing of the HOB.

Regards,
Michael

On 7/7/2021 1:55 AM, Benjamin Doron wrote:
> Hi all,
> I'm working on root-causing an issue where I'm unable to retrieve any 
> debug logs after cache-as-RAM teardown on my MinPlatform board port for 
> GSoC 2021. I'm using KabylakeOpenBoardPkg/PeiSerialPortLibSpiFlash.
> 
> My best guess at the moment is that it's related to HOBs, which this 
> SerialPortLib uses. If I reinitialise the library stack, it largely 
> works, but a HOB/context structure entry "CurrentWriteOffset" also looks 
> like it's zeroed.
> 
> Also, GetFeatureImplemented() in 
> MinPlatformPkg/Test/TestPointCheckLib/PeiTestPointCheckLib.c asserts 
> with "Status = Not Found". Internally, the test point architecture uses 
> HOBs. I don't think this should happen, so, if not for this, I would 
> think that perhaps a pointer in the serial port library needed to be 
> updated.
> 
> I can see that the heap is copied over to permanent memory by the PEI 
> core dispatcher 
> (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c#L881 
> <https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c#L881>). 
> However, I can't see that the HOB list pointer is updated, so it looks 
> like it 
> (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/Hob/Hob.c#L44 
> <https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/Hob/Hob.c#L44>) 
> still has the value that's copied over from the old PEI core's data 
> (https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c#L348 
> <https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c#L348>).
> 
> Does this look like a bug, or might there be something else that I'm 
> missing?
> 
> Best regards,
> Benjamin
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77618): https://edk2.groups.io/g/devel/message/77618
Mute This Topic: https://groups.io/mt/84038809/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-