[edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
Lendacky, Thomas via groups.io
thomas.lendacky=amd.com at groups.io
Tue Apr 18 21:06:07 UTC 2023
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_V2
It 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,
Tom
>
> Thanks,
> Dun
>
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Lendacky, Thomas via groups.io
> Sent: Saturday, April 15, 2023 3:05 AM
> To: devel at edk2.groups.io; Ni, Ray <ray.ni at intel.com>; Tan, Dun <dun.tan at 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 at edk2.groups.io <devel at edk2.groups.io> on behalf of
>> Lendacky, Thomas via groups.io <thomas.lendacky=amd.com at groups.io>
>> Sent: Friday, April 14, 2023 9:43:52 PM
>> To: Ni, Ray <ray.ni at intel.com>; Tan, Dun <dun.tan at intel.com>;
>> devel at edk2.groups.io <devel at 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.lendacky at amd.com>
>>>> Sent: Friday, April 14, 2023 12:19 AM
>>>> To: Tan, Dun <dun.tan at intel.com>; devel at edk2.groups.io
>>>> Cc: Ni, Ray <ray.ni at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>>>> Lendacky, Thomas via groups.io
>>>>> Sent: Thursday, April 13, 2023 3:26 AM
>>>>> To: devel at edk2.groups.io; Tan, Dun <dun.tan at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of
>>>> duntan
>>>>>> Sent: Wednesday, April 12, 2023 4:54 PM
>>>>>> To: devel at 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 (#103186): https://edk2.groups.io/g/devel/message/103186
Mute This Topic: https://groups.io/mt/98215421/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list