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_V2
Thanks, 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, Tom > > thanks, > 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 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? > > 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 = 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. > > 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, >>> 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 (#103158): https://edk2.groups.io/g/devel/message/103158 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] -=-=-=-=-=-=-=-=-=-=-=-
