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]
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
------------------------------------------------------------------------------
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
