Andrew: I provide my patch based on option 2. Could you help review them?
Thanks Liming From: Gao, Liming [mailto:[email protected]] Sent: Thursday, January 09, 2014 4:48 PM To: [email protected] Subject: Re: [edk2] Adding TempRam support to PeiServicesTablePointerLibIdt caused my platform to crash. Andrew: Thanks for your comments. I agree that the Library Class definition does not define this behavior. I think your option 2 is a clean way. It requires to update all PeiServicesTablePointerLib instances although most implementation is an empty function. Now, I know four library PeiServicesTablePointerLib instances, three ones are in MdePkg, last one is in ArmPkg. If I miss anyone, please let me know. If no, I will work on the patch and send it for review. For new API name, have you any suggestion? My initial proposal is like below. /** Migrate the space that stores the PEI Services Table pointer. For X86 and x64 processors, Interrupt Descriptor Table (IDT) preceding space is used to store EFI_PEI_SERVICES**. Migrate IDT and the preceding space to the permanent memory. For Itanium and ARM Processor, the specific register is used to store EFI_PEI_SERVICES**. No migration is required. **/ VOID EFIAPI MigratePeiServicesTablePointer ( ); Thanks Liming From: Andrew Fish [mailto:[email protected]] Sent: Thursday, January 09, 2014 3:40 PM To: [email protected]<mailto:[email protected]> Subject: Re: [edk2] Adding TempRam support to PeiServicesTablePointerLibIdt caused my platform to crash. On Jan 8, 2014, at 11:01 PM, Gao, Liming <[email protected]<mailto:[email protected]>> wrote: Andrew: When TempRamSupportPpi is not provided, PeiCore will take role to migrate data from temp memory to physical memory. So, PeiCore also need to reinitialize the IDT in the permanent memory per PI 5.4.1.1 and PI5.4.2.1 section. Because SetPeiServicesTablePointer() is only called by PeiCore, I add IDT reinitialization in this library instance. Well that is the implementation assumption that could make this an MdeModulePkg lib. PI spec says PEI service pointer at the IDT - sizeof(UINTN). So, I just copy it. I think this implementation follows PI spec. OK so I missed this statement in the PI 1.3 spec. that seems to make you copy operation legal. 3. Any PEIM can reinitialize the IDT with the following restrictions: * The four-bytes field immediately prior to new IDT base address must reside within the temporary or permanent memory. * The four-byte field immediately preceding the old IDT base address must be copied to the four-byte field immediately preceding the new IDT base address. So technically the copy is legal and our code is error for crashing in this situation. But I still think that the Library Class definition does not define this behavior, so adding it to the existing function is not legal. So for example following the Library Class definition I could do this (assume the old scheme): 1) Our debugger does the migration of the IDT and calls SetPeiServiceTablePointer() 2) PEI Core calls SetPeiServiceTablePointer() But if I linked against the old version of the lib, and my debugger was not there the IDT would not get migrated from temp RAM and the system could crash at some point. If the function is implement per the Library Class definition then this code works fine. With the extra side effect functionality you get 3 copies of the IDT. So in conclusion I think we need to do one of three things: 1) Update the library class definition to define that this IDT migration functionality is required. There are callers assuming it is! Update MdePkg Lib spec. 2) Update the library class definition to add a 3rd API that does the migration. Update PEI Core. Update MdePkg Lib spec. 3) Add a new library class to the MdeModulePkg that does the migration. Update PEI Core. If you use more IDT resource, you could provide your instance to handle it. Yes I know that. I was concerned that PEI Core implementation assumptions are leaking into the MdePkg. Thanks, Andrew Fish /** Caches a pointer PEI Services Table. Caches the pointer to the PEI Services Table specified by PeiServicesTablePointer in a CPU specific manner as specified in the CPU binding section of the Platform Initialization Pre-EFI Initialization Core Interface Specification. If PeiServicesTablePointer is NULL, then ASSERT(). @param PeiServicesTablePointer The address of PeiServices pointer. **/ VOID EFIAPI SetPeiServicesTablePointer ( IN CONST EFI_PEI_SERVICES ** PeiServicesTablePointer ); Thanks Liming From: Andrew Fish [mailto:[email protected]] Sent: Thursday, January 09, 2014 2:38 PM To: [email protected]<mailto:[email protected]> Subject: [edk2] Adding TempRam support to PeiServicesTablePointerLibIdt caused my platform to crash. I was wondering what the history was behind svn r14846? When I synced with this version my platform crashed. It seems to be making an implementation assumptions that go beyond what is in the PI spec, and that would imply this library should be in the MdeModulePkg. Not mention the assumptions should be document. This code seems to assume it is safe to move the PEI Services Table Retrieval location plus the IDT. This is not the case on our system, and we had the debugger doing the relocation. So doing the migration here, and updating the IDTR break us. I don't see how we are doing anything wrong from a PI 1.3 point of view? Just to be clear it is not illegal for us to store info at the ITD - 0x10 per the PI spec, but this code would break that scheme. https://svn.code.sf.net/p/edk2/code/trunk/edk2/MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointer.c VOID EFIAPI SetPeiServicesTablePointer ( IN CONST EFI_PEI_SERVICES ** PeiServicesTablePointer ) { IA32_DESCRIPTOR Idtr; EFI_PHYSICAL_ADDRESS IdtBase; EFI_STATUS Status; EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI *TemporaryRamSupportPpi; ASSERT (PeiServicesTablePointer != NULL); AsmReadIdtr (&Idtr); if ((*(UINTN*)(Idtr.Base - sizeof (UINTN))) != (UINTN)PeiServicesTablePointer) { (*(UINTN*)(Idtr.Base - sizeof (UINTN))) = (UINTN)PeiServicesTablePointer; Status = (*PeiServicesTablePointer)->LocatePpi ( PeiServicesTablePointer, &gEfiTemporaryRamSupportPpiGuid, 0, NULL, (VOID**)&TemporaryRamSupportPpi ); if (EFI_ERROR (Status)) { // // If TemporaryRamSupportPpi is not found, Idt table needs to be migrated into memory. // Status = (*PeiServicesTablePointer)->AllocatePages ( PeiServicesTablePointer, EfiBootServicesCode, EFI_SIZE_TO_PAGES(Idtr.Limit + 1 + sizeof (UINTN)), &IdtBase ); if (!EFI_ERROR (Status)) { // // Migrate Idt table // CopyMem ((VOID *) (UINTN) IdtBase, (VOID *) (Idtr.Base - sizeof (UINTN)), Idtr.Limit + 1 + sizeof (UINTN)); Idtr.Base = (UINTN) IdtBase + sizeof (UINTN); AsmWriteIdtr (&Idtr); } } } } Thanks, Andrew Fish ------------------------------------------------------------------------------ CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More Businesses Are Choosing CenturyLink Cloud For Critical Workloads, Development Environments & Everything In Between. Get a Quote or Start a Free Trial Today. http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk_______________________________________________ edk2-devel mailing list [email protected]<mailto:[email protected]> https://lists.sourceforge.net/lists/listinfo/edk2-devel
MdeModulePkg.patch
Description: MdeModulePkg.patch
ArmPlatformPkg.patch
Description: ArmPlatformPkg.patch
MdePkg.patch
Description: MdePkg.patch
------------------------------------------------------------------------------ CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More Businesses Are Choosing CenturyLink Cloud For Critical Workloads, Development Environments & Everything In Between. Get a Quote or Start a Free Trial Today. http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
