:p
atchew
Login
This patch series provides some fixes around AP creation: - An erratum on AMD hardware requires that a VMSA not be aligned on a 2MB boundary. To work around this issue, allocate 2 pages of memory and using the page that is not 2MB aligned and freeing the other. - When parking APs after exiting boot services, the current SNP support will perform an allocation that will not be reflected in memory map being supplied to the OS. Instead of allocating new VMSAs each time, re-use the current VMSA. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4353 --- These patches are based on commit: f80f052277c8 ("OvmfPkg/RiscVVirt: Add Stack HOB") Cc: Eric Dong <eric.dong@intel.com> Cc: Ray Ni <ray.ni@intel.com> Cc: Rahul Kumar <rahul1.kumar@intel.com> Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Michael Roth <michael.roth@amd.com> Cc: Ashish Kalra <Ashish.Kalra@amd.com> Tom Lendacky (2): UefiCpuPkg/MpInitLib: Ensure SEV-SNP VMSA allocations are not 2MB aligned UefiCpuPkg/MpInitLib: Reuse VMSA allocation to avoid unreserved allocation UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c | 224 ++++++++++++++-------- 1 file changed, 144 insertions(+), 80 deletions(-) -- 2.39.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101007): https://edk2.groups.io/g/devel/message/101007 Mute This Topic: https://groups.io/mt/97524216/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4353 Due to an erratum, an SEV-SNP VMSA cannot be 2MB aligned. To work around this issue, allocate two pages instead of one. Because of the way that page allocation is implemented, always try to use the second page. If the second page is not 2MB aligned, free the first page and use the second page. If the second page is 2MB aligned, free the second page and use the first page. Freeing in this way reduces holes in the memory map. Fixes: 06544455d0d4 ("UefiCpuPkg/MpInitLib: Use SEV-SNP AP Creation ...") Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> --- UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c | 24 +++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c index XXXXXXX..XXXXXXX 100644 --- a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c +++ b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c @@ -XXX,XX +XXX,XX @@ #include <Register/Amd/Fam17Msr.h> #include <Register/Amd/Ghcb.h> +#define IS_ALIGNED(x, y) ((((UINTN)(x) & (y - 1)) == 0)) + /** Create an SEV-SNP AP save area (VMSA) for use in running the vCPU. @@ -XXX,XX +XXX,XX @@ SevSnpCreateSaveArea ( UINT32 ApicId ) { + UINT8 *Pages; SEV_ES_SAVE_AREA *SaveArea; IA32_CR0 ApCr0; IA32_CR0 ResetCr0; @@ -XXX,XX +XXX,XX @@ SevSnpCreateSaveArea ( // // Allocate a single page for the SEV-ES Save Area and initialize it. + // Due to an erratum that prevents a VMSA being on a 2MB boundary, + // allocate an extra page to work around the issue. // - SaveArea = AllocateReservedPages (1); - if (!SaveArea) { + Pages = AllocateReservedPages (2); + if (!Pages) { return; } + // + // Since page allocation works by allocating downward in the address space, + // try to always free the first (lower address) page to limit possible holes + // in the memory map. So, if the address of the second page is 2MB aligned, + // then use the first page and free the second page. Otherwise, free the + // first page and use the second page. + // + if (IS_ALIGNED (Pages + EFI_PAGE_SIZE, SIZE_2MB)) { + SaveArea = (SEV_ES_SAVE_AREA *)Pages; + FreePages (Pages + EFI_PAGE_SIZE, 1); + } else { + SaveArea = (SEV_ES_SAVE_AREA *)(Pages + EFI_PAGE_SIZE); + FreePages (Pages, 1); + } + ZeroMem (SaveArea, EFI_PAGE_SIZE); // -- 2.39.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101008): https://edk2.groups.io/g/devel/message/101008 Mute This Topic: https://groups.io/mt/97524218/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
https://bugzilla.tianocore.org/show_bug.cgi?id=4353 When parking the APs on exiting from UEFI, a new page allocation is made. This allocation, however, does not end up being marked reserved in the memory map supplied to the OS. To avoid this, re-use the VMSA by clearing the VMSA RMP flag, updating the page contents and re-setting the VMSA RMP flag. Fixes: 06544455d0d4 ("UefiCpuPkg/MpInitLib: Use SEV-SNP AP Creation ...") Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> --- UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c | 234 +++++++++++++--------- 1 file changed, 139 insertions(+), 95 deletions(-) diff --git a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c index XXXXXXX..XXXXXXX 100644 --- a/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c +++ b/UefiCpuPkg/Library/MpInitLib/X64/AmdSev.c @@ -XXX,XX +XXX,XX @@ #define IS_ALIGNED(x, y) ((((UINTN)(x) & (y - 1)) == 0)) /** - Create an SEV-SNP AP save area (VMSA) for use in running the vCPU. + Perform the requested AP Creation action. - @param[in] CpuMpData Pointer to CPU MP Data - @param[in] CpuData Pointer to CPU AP Data + @param[in] SaveArea Pointer to VM save area (VMSA) @param[in] ApicId APIC ID of the vCPU + @param[in] Action AP action to perform + + @retval TRUE Action completed successfully + @retval FALSE Action did not complete successfully **/ -VOID -SevSnpCreateSaveArea ( - IN CPU_MP_DATA *CpuMpData, - IN CPU_AP_DATA *CpuData, - UINT32 ApicId +STATIC +BOOLEAN +SevSnpPerformApAction ( + IN SEV_ES_SAVE_AREA *SaveArea, + IN UINT32 ApicId, + IN UINTN Action ) { - UINT8 *Pages; - SEV_ES_SAVE_AREA *SaveArea; - IA32_CR0 ApCr0; - IA32_CR0 ResetCr0; - IA32_CR4 ApCr4; - IA32_CR4 ResetCr4; - UINTN StartIp; - UINT8 SipiVector; - UINT32 RmpAdjustStatus; - UINT64 VmgExitStatus; MSR_SEV_ES_GHCB_REGISTER Msr; GHCB *Ghcb; BOOLEAN InterruptState; UINT64 ExitInfo1; UINT64 ExitInfo2; + UINT64 VmgExitStatus; + UINT32 RmpAdjustStatus; - // - // Allocate a single page for the SEV-ES Save Area and initialize it. - // Due to an erratum that prevents a VMSA being on a 2MB boundary, - // allocate an extra page to work around the issue. - // - Pages = AllocateReservedPages (2); - if (!Pages) { - return; + if (Action == SVM_VMGEXIT_SNP_AP_CREATE) { + // + // To turn the page into a recognized VMSA page, issue RMPADJUST: + // Target VMPL but numerically higher than current VMPL + // Target PermissionMask is not used + // + RmpAdjustStatus = SevSnpRmpAdjust ( + (EFI_PHYSICAL_ADDRESS)(UINTN)SaveArea, + TRUE + ); + if (RmpAdjustStatus != 0) { + DEBUG ((DEBUG_INFO, "SEV-SNP: RMPADJUST failed for VMSA creation\n")); + ASSERT (FALSE); + + return FALSE; + } + } + + ExitInfo1 = (UINT64)ApicId << 32; + ExitInfo1 |= Action; + ExitInfo2 = (UINT64)(UINTN)SaveArea; + + Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); + Ghcb = Msr.Ghcb; + + CcExitVmgInit (Ghcb, &InterruptState); + + if (Action == SVM_VMGEXIT_SNP_AP_CREATE) { + Ghcb->SaveArea.Rax = SaveArea->SevFeatures; + CcExitVmgSetOffsetValid (Ghcb, GhcbRax); } - // - // Since page allocation works by allocating downward in the address space, - // try to always free the first (lower address) page to limit possible holes - // in the memory map. So, if the address of the second page is 2MB aligned, - // then use the first page and free the second page. Otherwise, free the - // first page and use the second page. - // - if (IS_ALIGNED (Pages + EFI_PAGE_SIZE, SIZE_2MB)) { - SaveArea = (SEV_ES_SAVE_AREA *)Pages; - FreePages (Pages + EFI_PAGE_SIZE, 1); + VmgExitStatus = CcExitVmgExit ( + Ghcb, + SVM_EXIT_SNP_AP_CREATION, + ExitInfo1, + ExitInfo2 + ); + + CcExitVmgDone (Ghcb, InterruptState); + + if (VmgExitStatus != 0) { + DEBUG ((DEBUG_INFO, "SEV-SNP: AP Destroy failed\n")); + ASSERT (FALSE); + + return FALSE; + } + + if (Action == SVM_VMGEXIT_SNP_AP_DESTROY) { + // + // Make the current VMSA not runnable and accessible to be + // reprogrammed. + // + RmpAdjustStatus = SevSnpRmpAdjust ( + (EFI_PHYSICAL_ADDRESS)(UINTN)SaveArea, + FALSE + ); + if (RmpAdjustStatus != 0) { + DEBUG ((DEBUG_INFO, "SEV-SNP: RMPADJUST failed for VMSA reset\n")); + ASSERT (FALSE); + + return FALSE; + } + } + + return TRUE; +} + +/** + Create an SEV-SNP AP save area (VMSA) for use in running the vCPU. + + @param[in] CpuMpData Pointer to CPU MP Data + @param[in] CpuData Pointer to CPU AP Data + @param[in] ApicId APIC ID of the vCPU +**/ +VOID +SevSnpCreateSaveArea ( + IN CPU_MP_DATA *CpuMpData, + IN CPU_AP_DATA *CpuData, + UINT32 ApicId + ) +{ + UINT8 *Pages; + SEV_ES_SAVE_AREA *SaveArea; + IA32_CR0 ApCr0; + IA32_CR0 ResetCr0; + IA32_CR4 ApCr4; + IA32_CR4 ResetCr4; + UINTN StartIp; + UINT8 SipiVector; + + if (CpuData->SevEsSaveArea == NULL) { + // + // Allocate a single page for the SEV-ES Save Area and initialize it. + // Due to an erratum that prevents a VMSA being on a 2MB boundary, + // allocate an extra page to work around the issue. + // + Pages = AllocateReservedPages (2); + if (!Pages) { + return; + } + + // + // Since page allocation works by allocating downward in the address space, + // try to always free the first (lower address) page to limit possible holes + // in the memory map. So, if the address of the second page is 2MB aligned, + // then use the first page and free the second page. Otherwise, free the + // first page and use the second page. + // + if (IS_ALIGNED (Pages + EFI_PAGE_SIZE, SIZE_2MB)) { + SaveArea = (SEV_ES_SAVE_AREA *)Pages; + FreePages (Pages + EFI_PAGE_SIZE, 1); + } else { + SaveArea = (SEV_ES_SAVE_AREA *)(Pages + EFI_PAGE_SIZE); + FreePages (Pages, 1); + } + + CpuData->SevEsSaveArea = SaveArea; } else { - SaveArea = (SEV_ES_SAVE_AREA *)(Pages + EFI_PAGE_SIZE); - FreePages (Pages, 1); + SaveArea = CpuData->SevEsSaveArea; + + // + // Tell the hypervisor to not use the current VMSA + // + if (!SevSnpPerformApAction (SaveArea, ApicId, SVM_VMGEXIT_SNP_AP_DESTROY)) { + return; + } } ZeroMem (SaveArea, EFI_PAGE_SIZE); @@ -XXX,XX +XXX,XX @@ SevSnpCreateSaveArea ( SaveArea->Vmpl = 0; SaveArea->SevFeatures = AsmReadMsr64 (MSR_SEV_STATUS) >> 2; - // - // To turn the page into a recognized VMSA page, issue RMPADJUST: - // Target VMPL but numerically higher than current VMPL - // Target PermissionMask is not used - // - RmpAdjustStatus = SevSnpRmpAdjust ( - (EFI_PHYSICAL_ADDRESS)(UINTN)SaveArea, - TRUE - ); - ASSERT (RmpAdjustStatus == 0); - - ExitInfo1 = (UINT64)ApicId << 32; - ExitInfo1 |= SVM_VMGEXIT_SNP_AP_CREATE; - ExitInfo2 = (UINT64)(UINTN)SaveArea; - - Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); - Ghcb = Msr.Ghcb; - - CcExitVmgInit (Ghcb, &InterruptState); - Ghcb->SaveArea.Rax = SaveArea->SevFeatures; - CcExitVmgSetOffsetValid (Ghcb, GhcbRax); - VmgExitStatus = CcExitVmgExit ( - Ghcb, - SVM_EXIT_SNP_AP_CREATION, - ExitInfo1, - ExitInfo2 - ); - CcExitVmgDone (Ghcb, InterruptState); - - ASSERT (VmgExitStatus == 0); - if (VmgExitStatus != 0) { - RmpAdjustStatus = SevSnpRmpAdjust ( - (EFI_PHYSICAL_ADDRESS)(UINTN)SaveArea, - FALSE - ); - if (RmpAdjustStatus == 0) { - FreePages (SaveArea, 1); - } else { - DEBUG ((DEBUG_INFO, "SEV-SNP: RMPADJUST failed, leaking VMSA page\n")); - } - - SaveArea = NULL; - } - - if (CpuData->SevEsSaveArea) { - RmpAdjustStatus = SevSnpRmpAdjust ( - (EFI_PHYSICAL_ADDRESS)(UINTN)CpuData->SevEsSaveArea, - FALSE - ); - if (RmpAdjustStatus == 0) { - FreePages (CpuData->SevEsSaveArea, 1); - } else { - DEBUG ((DEBUG_INFO, "SEV-SNP: RMPADJUST failed, leaking VMSA page\n")); - } - } - - CpuData->SevEsSaveArea = SaveArea; + SevSnpPerformApAction (SaveArea, ApicId, SVM_VMGEXIT_SNP_AP_CREATE); } /** -- 2.39.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101009): https://edk2.groups.io/g/devel/message/101009 Mute This Topic: https://groups.io/mt/97524223/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
This patch series provides fixes around AP startup and sorting: - The CPUID_EXTENDED_TOPOLOGY CPUID leaf takes a sub-leaf as input. The current SEV-SNP support is attempting to retrieve the this leaf with sub-leaf 0, but is calling AsmCpuid(), which does not clear ECX before invoking the CPUID instruction. Therefore, because of the calling convention, the leaf value becomes the sub-leaf value and ends up returning incorrect information. Change the call from AsmCpuid() to AsmCpuidEx(). - When sorting the CPUs by APIC ID, the VMSA associated with the vCPU should follow the APIC ID. Update the sorting code to swap the VMSA pointer during the sort. --- These patches are based on commit: da219919538b ("BaseTools: GenFw: auto-set nxcompat flag") Tom Lendacky (2): UefiCpuPkg/MpInitLib: Use AsmCpuidEx() for CPUID_EXTENDED_TOPOLOGY leaf UefiCpuPkg/MpInitLib: Copy SEV-ES save area pointer during APIC ID sorting UefiCpuPkg/Library/MpInitLib/AmdSev.c | 9 ++++++++- UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 +++++++- 2 files changed, 15 insertions(+), 2 deletions(-) -- 2.42.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110766): https://edk2.groups.io/g/devel/message/110766 Mute This Topic: https://groups.io/mt/102432041/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
The CPUID_EXTENDED_TOPOLOGY CPUID leaf takes a subleaf as input when returning CPUID information. However, the AsmCpuid() function does not zero out ECX before the CPUID instruction, so the input leaf is used as the sub-leaf for the CPUID request and returns erroneous/invalid CPUID data, since the intent of the request was to get data related to sub-leaf 0. Instead, use AsmCpuidEx() for the CPUID_EXTENDED_TOPOLOGY leaf. Fixes: d4d7c9ad5fe5 ("UefiCpuPkg/MpInitLib: use BSP to do extended ...") Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> --- UefiCpuPkg/Library/MpInitLib/AmdSev.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/AmdSev.c b/UefiCpuPkg/Library/MpInitLib/AmdSev.c index XXXXXXX..XXXXXXX 100644 --- a/UefiCpuPkg/Library/MpInitLib/AmdSev.c +++ b/UefiCpuPkg/Library/MpInitLib/AmdSev.c @@ -XXX,XX +XXX,XX @@ FillExchangeInfoDataSevEs ( if (StdRangeMax >= CPUID_EXTENDED_TOPOLOGY) { CPUID_EXTENDED_TOPOLOGY_EBX ExtTopoEbx; - AsmCpuid (CPUID_EXTENDED_TOPOLOGY, NULL, &ExtTopoEbx.Uint32, NULL, NULL); + AsmCpuidEx ( + CPUID_EXTENDED_TOPOLOGY, + 0, + NULL, + &ExtTopoEbx.Uint32, + NULL, + NULL + ); ExchangeInfo->ExtTopoAvail = !!ExtTopoEbx.Bits.LogicalProcessors; } } -- 2.42.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110767): https://edk2.groups.io/g/devel/message/110767 Mute This Topic: https://groups.io/mt/102432043/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
With SEV-SNP, the SEV-ES save area for a vCPU should be unique to that vCPU. After commit 3323359a811a, the VMSA allocation was re-used, but when sorting the CPUs by APIC ID, the save area was not updated to follow the original CPU. Similar to the StartupApSignal address, the SevEsSaveArea address should be updated when sorting the CPUs. This does not cause an issue at this time because all APs are in HLT state and then are (re)started at the same time, with the same VMSA contents. However, this should be fixed to account for any change in future behavior. Fixes: 3323359a811a ("UefiCpuPkg/MpInitLib: Reuse VMSA allocation to ...") Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> --- UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c index XXXXXXX..XXXXXXX 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -XXX,XX +XXX,XX @@ SortApicId ( UINT32 ApCount; CPU_INFO_IN_HOB *CpuInfoInHob; volatile UINT32 *StartupApSignal; + VOID *SevEsSaveArea; ApCount = CpuMpData->CpuCount - 1; CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob; @@ -XXX,XX +XXX,XX @@ SortApicId ( CopyMem (&CpuInfoInHob[Index1], &CpuInfo, sizeof (CPU_INFO_IN_HOB)); // - // Also exchange the StartupApSignal. + // Also exchange the StartupApSignal and SevEsSaveArea. // StartupApSignal = CpuMpData->CpuData[Index3].StartupApSignal; CpuMpData->CpuData[Index3].StartupApSignal = CpuMpData->CpuData[Index1].StartupApSignal; CpuMpData->CpuData[Index1].StartupApSignal = StartupApSignal; + + SevEsSaveArea = CpuMpData->CpuData[Index3].SevEsSaveArea; + CpuMpData->CpuData[Index3].SevEsSaveArea = + CpuMpData->CpuData[Index1].SevEsSaveArea; + CpuMpData->CpuData[Index1].SevEsSaveArea = SevEsSaveArea; } } -- 2.42.0 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110768): https://edk2.groups.io/g/devel/message/110768 Mute This Topic: https://groups.io/mt/102432047/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-