On 4/19/23 00:39, duntan via groups.io wrote:
Hi Tom,This PF happened because that CR0.WP is set and DxeMemEncryptSevLib sets a part of smm page table as RO before ReadyToLock while CpuSmm driver assumes the smm page table is not marked as RO before ReadyToLock. The code flow to set smm page table to RO is QemuFlashBeforeProbe()-->MemEncryptSevClearMmioPageEncMask()-->SetMemoryEncDec()-->EnablePageTableProtection(). New page table memory allocated by DxeMemEncryptSevLib is marked as RO in EnablePageTableProtection(). However, In PiSmmCpuDxeSmm InitPaging() function, the RO page table marked by DxeMemEncryptSevLib may be modified again and InitPaging() function doesn't clear CR0.WP in advance since it has the assumption that smm page table memory is always RW before SetPageTableAttributes(), which causes this PF. So Could you please help test the SEV enabled smm code without my patch set? I think this issue may also happen in current edk2 code.
Upstream master branch boots without issue. SmmInstallProtocolInterface: 47B7FA8C-F4BD-4AF6-8200-333086F0D2C8 0 GetUefiMemoryMap Patch page table start ... Patch page table done! MemoryAttributesTable: Version - 0x00000001 NumberOfEntries - 0x00000035 DescriptorSize - 0x00000030 Entry (0x3FF01028) ... Thanks, Tom
Thanks, Dun -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky, Thomas via groups.io Sent: Wednesday, April 19, 2023 5:06 AM To: Tan, Dun <dun....@intel.com>; devel@edk2.groups.io; Ni, Ray <ray...@intel.com> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table On 4/18/23 04:57, Tan, Dun wrote:Hi Tom, I added a new patch in my code branch. This new patch removes the code that may apply AddressEncMask to smm page table non-leaf entries when splitting page table. Could you please help test if this code branch works? https://github.com/td36/edk2/tree/SmmPageTable_V2It got further, but it still crashed: SmmInstallProtocolInterface: 47B7FA8C-F4BD-4AF6-8200-333086F0D2C8 0 GetUefiMemoryMap Patch page table start ... !!!! X64 Exception Type - 0E(#PF - Page-Fault) CPU Apic ID - 00000000 !!!! ExceptionData - 0000000000000003 I:0 R:0 U:0 W:1 P:1 PK:0 SS:0 SGX:0 RIP - 000000003FFC7744, CS - 0000000000000038, RFLAGS - 0000000000010082 RAX - 00000000FFFFFF83, RCX - 000000003FFB5C78, RDX - 0000000000000000 RBX - 000000003FC01000, RSP - 000000003FFB4790, RBP - 000000003FFB47B0 RSI - 000000003FC01000, RDI - 0000000000000000 R8 - 000000003FFB4840, R9 - 0000000000000002, R10 - 0000000000000000 R11 - 0000000000000000, R12 - 000000003FFB5C78, R13 - 000000003FFB4840 R14 - 0000000080000000, R15 - 0000000000000000 DS - 0000000000000020, ES - 0000000000000020, FS - 0000000000000020 GS - 0000000000000020, SS - 0000000000000020 CR0 - 0000000080010033, CR2 - 000000003FC01000, CR3 - 000000003FF81000 CR4 - 0000000000000668, CR8 - 0000000000000000 DR0 - 0000000000000000, DR1 - 0000000000000000, DR2 - 0000000000000000 DR3 - 0000000000000000, DR6 - 00000000FFFF0FF0, DR7 - 0000000000000400 GDTR - 000000003FFAC000 000000000000004F, LDTR - 0000000000000000 IDTR - 000000003FFAF000 00000000000001FF, TR - 0000000000000040 FXSAVE_STATE - 000000003FFB0C60 SMM exception at access (0x3FC01000) It is invoked from the instruction before IP(0x3FFC7744) in module (/root/kernels/ovmf-dun-build-Ia32X64/Build/Ovmf3264/DEBUG_GCC5/X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSm m/DEBUG/PiSmmCpuDxeSmm.dll) Thanks, TomThanks, Dun -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky, Thomas via groups.io Sent: Saturday, April 15, 2023 3:05 AM To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>; Tan, Dun <dun....@intel.com> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table On 4/14/23 12:19, Ni, Ray via groups.io wrote:tom, if the c bit is not required for non leaf page table entries, why the trunk code sets the c bit for all entities including nonleaf ones?Because it's effectively the correct thing to do, even though it doesn't matter.i went back to read again the smm issue you met. you said the c bit is set for non leaf entries that caused a deference issue. But the pagetablelib code doesn't set c bit to non leaf entries. then who sets the c bit?I guess that's the main question, how did that get set? I haven't had the time to fully examine and follow the codepath in the pagetable library to figure out why it was set. Maybe as part of a page split? Thanks, Tomthanks, ray thanks, ray ________________________________ From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Lendacky, Thomas via groups.io <thomas.lendacky=amd....@groups.io> Sent: Friday, April 14, 2023 9:43:52 PM To: Ni, Ray <ray...@intel.com>; Tan, Dun <dun....@intel.com>; devel@edk2.groups.io <devel@edk2.groups.io> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table On 4/14/23 00:07, Ni, Ray wrote:-----Original Message----- From: Tom Lendacky <thomas.lenda...@amd.com> Sent: Friday, April 14, 2023 12:19 AM To: Tan, Dun <dun....@intel.com>; devel@edk2.groups.io Cc: Ni, Ray <ray...@intel.com> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table On 4/13/23 04:14, Tan, Dun wrote:Hi Tom, Thank you for your help with testing. For the build failure, it's because that the CpuPageTableLib instance isadded into OvmfPkg DSC in the last pacth ' OvmfPkg: Add CpuPageTableLib required by PiSmmCpuDxe'. I have moved this patch to the head of the patch set.For the boot failure, I think it's because that the encrypt mask was notapplied to the memory used by page table in page table non-leaf entry. Initially I thought the encrypt mask would only be applied to the leaf entry in AMD SEV feature. So I treated the encryption process as non 1:1 mapping, which only applies the encrypt mask to leaf entry. I'm also curious why the DxeIpl patch set works good. All the page table non-leaf entries are also not encrypted in the DxeIpl page table related patch set. Right, and that works for SEV. All non-leaf pagetable entries are treated as encrypted regardless of the encryption bit. Since the tables were built being mapped encrypted, the pagetable walk works when the non-leaf entries don't have the encryption bit set. In this case, though, the encryption bit is present in the non-leaf entry and that is the reason why there are issues.Can you point us which doc here (https://www.amd.com/en/developer/sev.html) says the page table is encrypted regardless the KEY_ID bits value? How can the encryption engine know if a chunk of memory belongs to page table?It doesn't. For an SEV guest, when the hardware walks the pagetables, it will always treat the memory accesses as encrypted (see section 15.34.5 of the AMD APM Vol 2 at https://www.amd.com/system/files/TechDocs/24593.pdf). But, because the initial pagetables that are built to map everything as encrypted/private to start with (see OvmfPkg/ResetVector/Ia32/PageTables64.asm), only changing to shared when specifically requested, any memory allocated and used will be encrypted. Thus, when new pagetables are allocated/created in the CpuPageTableLib library, they will be encrypted and so everything works. And those new pagetables will map everything encrypted by default, except for the GHCB pages. If they were mapped shared when they were created, then the pagetable walk would fail.My understanding to SEV is any physical address field in guest page table should have the KEY_ID bits set if the physical pages are private to guest. Only some pages for GMCB don't have KEY_ID bits set as those are shared between guest and host.Right, the encryption bit in the leaf entry of the pagetables will determine the encryption mode.I thought Dun's patch works because all guest memory is marked as shared because the KEY_ID bits in all entries are not set. Only some pages that're used by GMCB have the KEY_ID bits set.Just the opposite, the CpuPageTableLib library marks everything encrypted and only clears the encryption bit for the GHCB pages. In MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c, the CreateIdentityMappingPageTables() function retrieves the encryption bit and saves it in AddressEncMask. AddressEncMask is then applied to the mapping attribute used when calling CreateOrUpdatePageTable() to build the initial pagetables.Here is some debug after setting PagingEntry at line 436 of UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c: *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF81000 *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF80000 *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF83000 *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF81000 *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF80000 *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF83000 *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF81000 *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF80000 *** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 800003FC01000Are you testing the SME or SEV? My understanding is with SME, only the highest C bit should be set indicating the physical page is encrypted.I am testing SEV. There is only a single bit to indicate whether a page is encrypted. The guest ASID is used to determine what key is used to decrypt the page. From a pagetable leaf entry, SME and SEV are equivalent, the encryption bit determines how the memory will be accessed. SME and SEV differ in how they deal with instruction fetches and pagetable walks, with SME obeying the encryption bit and SEV always performing the accesses as encrypted accesses for security. Thanks, Tom!!!! X64 Exception Type - 0D(#GP - General Protection) CPU Apic ID - 00000000 !!!! 0x800003FC01000 isn't mapped and so it fails - I'm not exactly sure how the #PF turns into a #GP, though, maybe because the virtual address isn't canonical that point. Thanks, TomI'll added another patch in my code branch to fix this issue later. In the newcommit, from the perspective of CpuPageTableLib, the whole memory can be divided into 3 categories: memory used by page table, guest private memory and guest shared memory. CpuPageTableLib will always apply the encrypt mask to memory used by page table, which means all the non-leaf page table entries are encrypted. For guest private memory, this case can be treated as non-1:1 mapping. We can apply the encrypt mask by setting the input parameter of PageTableMap() API like " Attribute.Uint64 = LinearAddress | AddressEncMask". For guest shared memory, this case can be treated as normal 1:1 mapping. I'll let you know once the new patch is ready.Thanks, Dun -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf OfLendacky, Thomas via groups.ioSent: Thursday, April 13, 2023 3:26 AM To: devel@edk2.groups.io; Tan, Dun <dun....@intel.com> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to createand update smm page tableOn 4/12/23 05:17, duntan via groups.io wrote:Hi Tom, This patch set is to change PiSmmCpuDxeSmm code to useCpuPageTableLib to create and update SMM page table. The Pcd PcdPteMemoryEncryptionAddressOrMask is also used in PiSmmCpuDxeSmm code and the whole range covered by page table is mapped encrypted, which is different from the situation in DxeIpl module.So could you also help do a test to make sure the AMD SEV feature stillworks good in SMM with this patch set?Here is the code branch in my fork repo: https://github.com/td36/edk2/commits/SmmPageTable_V2Hi Dun, I tested at the final commit of the branch and encountered a #GP with anSEV guest. It looks like the CpuPageTableLibrary doesn't take the encryption bit into account. For example:Line 436 of UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c PagingEntry = (IA32_PAGING_ENTRY*)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-Pnle); This will get an address with the encryption bit set and then try toreference it. When I clear the encryption bit, the code proceeds a bit further, but then encounters a #GP in a different location.So it appears that the CpuPageTableLibrary doesn't deal with theencryption bit properly.Also, going through a build/test of each individual patch had mixed results. - With the second patch in the series applied, I get a build error: /root/kernels/ovmf-dun-build-X64/OvmfPkg/OvmfPkgX64.dsc(...):error 4000: Instance of library class [CpuPageTableLib] is not foundin [/root/kernels/ovmf-dun-build-X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf] [X64]consumed by module [/root/kernels/ovmf-dun-build-X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf]that isn't resolved until the final patch. Thanks, TomThanks, Dun -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf OfduntanSent: Wednesday, April 12, 2023 4:54 PM To: devel@edk2.groups.io Subject: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table In V2 patch set: 1.In 'Refinement to code about updating smm page table', use QuickSort()in BaseLib instead or PerformQuickSort() in BaseSortLib.2.Remove the patch to add BaseSortLib in DSC file. 3.Add a new patch to add CpuPageTableLib in UefiCpuPkg.dsc. 4.Add a temp patch to add CpuPageTableLib in OvmfPkg dsc files for test(A previous patch I sent before '[Patch V2 4/8] OvmfPkg: Add CpuPageTableLib required by DxeIpl in DSC file' contains all the changes in this patch) Dun Tan (8): OvmfPkg: Add CpuPageTableLib required by PiSmmCpuDxe UefiPayloadPkg: Add CpuPageTableLib required by PiSmmCpuDxe UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute. UefiCpuPkg/PiSmmCpuDxeSmm: Avoid setting non-present range toRO/NXUefiCpuPkg: Extern mSmmShadowStackSize in PiSmmCpuDxeSmm.h UefiCpuPkg: Refinement to current smm page table generation code UefiCpuPkg: Refinement to code about updating smm page table UefiCpuPkg/PiSmmCpuDxeSmm: Remove unnecessary function OvmfPkg/CloudHv/CloudHvX64.dsc | 2 +- OvmfPkg/OvmfPkgIa32.dsc | 3 ++- OvmfPkg/OvmfPkgIa32X64.dsc | 2 +- OvmfPkg/OvmfPkgX64.dsc | 2 +- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c | 5 +++-- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 3 +-- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c | 2 +- UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 132 ------------------------------------------------------------------------------------ - -------------------------- ---------------------UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 8 ++++++--UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 97++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ ++-------------------------------------UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 1 + UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 629++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ ++++++++++++++++++++++++++++++++++++++++++------------------------- ++++++++++++++++++++++++++++++++++++++++++- ------------------------------------------------------------------- - -------------------------- ------------------------------------------------------------------- - -------------------------- ------------------------------------------------------------------- - -------------------------- -----------------------------------------------UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 348++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ +++++++++---------------------------------------------------------- +++++++++- +++++++++-------------------- ------------------------------------------------------------------- - -------------------------- --------------------------------------------------UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c | 229++++++++++++++++++++++++++++++------------------------------------- ++++++++++++++++++++++++++++++- ++++++++++++++++++++++++++++++-------- ------------------------------------------------------------------- - -------------------------- -----------------------------------------------------------UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 3 +-- UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c | 19 ++-----------------UefiPayloadPkg/UefiPayloadPkg.dsc | 2 +- 17 files changed, 510 insertions(+), 977 deletions(-)
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103221): https://edk2.groups.io/g/devel/message/103221 Mute This Topic: https://groups.io/mt/98215421/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
