> -----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 is > added 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 not > applied 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? 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. 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. > > 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 = 800003FC01000 Are 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. > !!!! 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, > Tom > > > > > I'll added another patch in my code branch to fix this issue later. In the > > new > commit, 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 Of > Lendacky, Thomas via groups.io > > Sent: 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 create > and update smm page table > > > > On 4/12/23 05:17, duntan via groups.io wrote: > >> Hi Tom, > >> > >> This patch set is to change PiSmmCpuDxeSmm code to use > CpuPageTableLib 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 still > works good in SMM with this patch set? > >> Here is the code branch in my fork repo: > >> https://github.com/td36/edk2/commits/SmmPageTable_V2 > > > > Hi Dun, > > > > I tested at the final commit of the branch and encountered a #GP with an > SEV 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 to > reference 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 the > encryption 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 found > > in [/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, > > Tom > > > >> > >> Thanks, > >> Dun > >> > >> -----Original Message----- > >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of > duntan > >> Sent: 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 to > RO/NX > >> UefiCpuPkg: 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 (#102964): https://edk2.groups.io/g/devel/message/102964 Mute This Topic: https://groups.io/mt/98215421/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
