On Jan 8, 2014, at 11:01 PM, Gao, Liming <[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]
> 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]
> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
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
