On 09/22/20 21:59, Tom Lendacky wrote:
From: Tom Lendacky <email@example.com>Looks like this is a fix for commit 7b7508ad784d ("UefiCpuPkg: Allow AP
booting under SEV-ES", 2020-08-17).
In retrospect, that commit changed "ApResetVectorSize" -- which is
passed to GetWakeupBuffer() -- from value [a]
RendezvousFunnelSize + sizeof (MP_CPU_EXCHANGE_INFO)
to value [b]
ALIGN_VALUE ((RendezvousFunnelSize +
AP_RESET_STACK_SIZE * PcdGet32(PcdCpuMaxLogicalProcessorNumber)
The currently proposed patch does not entirely restore
"ApResetVectorSize" to the value it used to carry before commit
7b7508ad784d. Namely, while the patch eliminates the last addend, two
other changes from commit 7b7508ad784d remain in place:
- the addition of "SwitchToRealSize",
- the alignment up to CPU_STACK_ALIGNMENT.
As far as I understand, "SwitchToRealSize" is never zero
(AsmGetAddressMap() populates it with a difference of build-time
constants). I think it's not useful when SEV-ES is inactive.
Furthermore, the alignment for stack purposes is useless if we won't
have AP stacks (i.e., again when SEV-ES is inactive).
(1) Therefore I'd propose:
- folding GetApResetStackSize() into GetApResetVectorSize(),
- modifying GetApResetVectorSize() such that it return the original sum
[a] if SEV-ES is inactive, and the larger sum [b] if SEV-ES is active.
Hmmm. OK, maybe "SwitchToRealSize" should remain in place. It's a small
addition, and it reflects a code portion that is permanent. However, I
do think the alignment is both useless and confusing. If we won't
allocate an array of stacks, the alignment really makes no sense.
(2) A style comment: PcdGetBool()'s return value should not be compared
with TRUE or FALSE; just use PcdGetBool() as the whole controlling
expression for the "if".
(3) Even better... can you modify GetApResetVectorSize() to take
&CpuMpData rather than &CpuMpData->AddressMap, and then check
Hmmm, wait, that's not really simple, as we call GetApResetVectorSize()
from MpInitLibInitialize() too, way before we set
CpuMpData->SevEsIsEnabled from the PCD.
So I guess we should pass a dedicated BOOLEAN parameter to
GetApResetVectorSize(), called "SevEsIsEnabled". At the call site in
MpInitLibInitialize(), we should pass in the PCD's value. At the call
site in AllocateResetVector(), we should pass in
The reason I'm suggesting (3) is that I don't feel comfortable with
checking dynamic PCDs outside of entry point functions / initialization