tianocore / edk2

EDK II
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II
Other
4.64k stars 2.51k forks source link

UefiCpuPkg: MpLib: Allow Compatibility with PeiServicesTablePointerLibIdt #6008

Closed os-d closed 3 months ago

os-d commented 3 months ago

Description

Fix to allow compatibility with PeiServicesTablePointerLibIdt

APs all hit exceptions because silicon specific code expected a valid PeiServicesTablePointer before the IDT address.

Per the PI spec 1.8A section I-9.4.2, "For x64 processors the EFI_PEI_SERVICES ** is stored in the eight bytes immediately preceding the Interrupt Descriptor Table."

This patch aligns the MpLib code to align to the PI spec.

How This Was Tested

Shipping on multiple generations of physical and virtual platforms.

Integration Instructions

N/A.

os-d commented 3 months ago

@niruiyu, this was originally discovered because silicon code running on the APs was expecting to find a valid PeiServices pointer before the IDT base. We can work with the silicon code owners to ensure this is fixed, but can you point to the PI spec where it says PEI Services are not callable from APs? I am not seeing it and in fact I am seeing that the pointer to PEI services is passed in the WhoAmI PPI which is allowed to be called from APs:

typedef
EFI_STATUS
(EFIAPI *EFI_PEI_MP_SERVICES_WHOAMI)(
IN CONST EFI_PEI_SERVICES **PeiServices,
IN EFI_PEI_MP_SERVICES_PPI *This,
OUT UINTN *ProcessorNumber
);
...
This services returns the handle number for the calling processor. This service may be called from the BSP
and APs.

This is from section I-12.3.9.11 of PI spec 1.8A.

And section I-9.4.2 just says the PEI Services pointer will be the 8 bytes preceding the IDT for x64 processors, but does not specify only the BSP.

I agree that APs should not be using PEI services, but I can't see spec justification for that.

niruiyu commented 3 months ago

@niruiyu, this was originally discovered because silicon code running on the APs was expecting to find a valid PeiServices pointer before the IDT base. We can work with the silicon code owners to ensure this is fixed, but can you point to the PI spec where it says PEI Services are not callable from APs? I am not seeing it and in fact I am seeing that the pointer to PEI services is passed in the WhoAmI PPI which is allowed to be called from APs:

typedef
EFI_STATUS
(EFIAPI *EFI_PEI_MP_SERVICES_WHOAMI)(
IN CONST EFI_PEI_SERVICES **PeiServices,
IN EFI_PEI_MP_SERVICES_PPI *This,
OUT UINTN *ProcessorNumber
);
...
This services returns the handle number for the calling processor. This service may be called from the BSP
and APs.

This is from section I-12.3.9.11 of PI spec 1.8A.

And section I-9.4.2 just says the PEI Services pointer will be the 8 bytes preceding the IDT for x64 processors, but does not specify only the BSP.

I agree that APs should not be using PEI services, but I can't see spec justification for that.

I tried 2 mins to find such a restriction in PI spec and I failed. But, that does not mean APs can call PEI services. If you check the PeiCore implementation you would know the PEI services are not MP-safe. These services cannot be called concurrently by multiple processors. I do not want to argue from spec perspective. This patch cannot be accepted. The silicon init code that calls PEI services from APs should be fixed.

os-d commented 3 months ago

@niruiyu, I agree PEI Services are not MP safe and that the silicon code needs to be fixed. With your confirmation we can work with the x64 silicon code vendors to fix up their code and point to this PR as evidence that APs cannot use MP services. I will close this PR.