* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
[not found] <1755241E6695EAE7.1885@groups.io>
@ 2023-04-12 8:58 ` duntan
2023-04-12 10:17 ` duntan
1 sibling, 0 replies; 16+ messages in thread
From: duntan @ 2023-04-12 8:58 UTC (permalink / raw)
To: devel@edk2.groups.io, Gerd Hoffmann
Hi Gerd,
OvmfPkg/OvmfPkgIa32X64.dsc fails to build with first version patch set is because that CpuPageTableLib is not in OvmfPkg DSC files. The reason why I didn't add CpuPageTableLib into OvmfPkg DSC files is because a previous patch I sent before '[Patch V2 4/8] OvmfPkg: Add CpuPageTableLib required by DxeIpl in DSC file' contains the changes to do this.
In the V2 patch, I add a temp patch 'OvmfPkg: Add CpuPageTableLib required by PiSmmCpuDxe' to add CpuPageTableLib into OvmfPkg DSC files. This temp patch will be removed after DxeIpl page table related code is merged. You can directly test the V2 patch set. Thanks for helping to test.
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(-)
--
2.39.1.windows.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
[not found] <1755241E6695EAE7.1885@groups.io>
2023-04-12 8:58 ` [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table duntan
@ 2023-04-12 10:17 ` duntan
2023-04-12 19:25 ` Lendacky, Thomas
1 sibling, 1 reply; 16+ messages in thread
From: duntan @ 2023-04-12 10:17 UTC (permalink / raw)
To: devel@edk2.groups.io, Tom Lendacky
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
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(-)
--
2.39.1.windows.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-12 10:17 ` duntan
@ 2023-04-12 19:25 ` Lendacky, Thomas
2023-04-13 9:14 ` duntan
0 siblings, 1 reply; 16+ messages in thread
From: Lendacky, Thomas @ 2023-04-12 19:25 UTC (permalink / raw)
To: devel, dun.tan
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(-)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-12 19:25 ` Lendacky, Thomas
@ 2023-04-13 9:14 ` duntan
2023-04-13 16:18 ` Lendacky, Thomas
0 siblings, 1 reply; 16+ messages in thread
From: duntan @ 2023-04-13 9:14 UTC (permalink / raw)
To: devel@edk2.groups.io, thomas.lendacky@amd.com; +Cc: Ni, Ray
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.
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.tan@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(-)
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-13 9:14 ` duntan
@ 2023-04-13 16:18 ` Lendacky, Thomas
2023-04-14 5:07 ` Ni, Ray
0 siblings, 1 reply; 16+ messages in thread
From: Lendacky, Thomas @ 2023-04-13 16:18 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io; +Cc: Ni, Ray
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.
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
!!!! 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.tan@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(-)
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-13 16:18 ` Lendacky, Thomas
@ 2023-04-14 5:07 ` Ni, Ray
2023-04-14 13:43 ` Lendacky, Thomas
0 siblings, 1 reply; 16+ messages in thread
From: Ni, Ray @ 2023-04-14 5:07 UTC (permalink / raw)
To: Tom Lendacky, Tan, Dun, devel@edk2.groups.io
> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Friday, April 14, 2023 12:19 AM
> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@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.tan@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(-)
> >>
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-14 5:07 ` Ni, Ray
@ 2023-04-14 13:43 ` Lendacky, Thomas
2023-04-14 17:19 ` Ni, Ray
0 siblings, 1 reply; 16+ messages in thread
From: Lendacky, Thomas @ 2023-04-14 13:43 UTC (permalink / raw)
To: Ni, Ray, Tan, Dun, devel@edk2.groups.io
On 4/14/23 00:07, Ni, Ray wrote:
>
>
>> -----Original Message-----
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>> Sent: Friday, April 14, 2023 12:19 AM
>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>
>>>
>>>
>>>
>>>
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-14 13:43 ` Lendacky, Thomas
@ 2023-04-14 17:19 ` Ni, Ray
2023-04-14 19:05 ` Lendacky, Thomas
0 siblings, 1 reply; 16+ messages in thread
From: Ni, Ray @ 2023-04-14 17:19 UTC (permalink / raw)
To: devel@edk2.groups.io, thomas.lendacky@amd.com, Tan, Dun
[-- Attachment #1: Type: text/plain, Size: 13719 bytes --]
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?
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?
thanks,
ray
thanks,
ray
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Lendacky, Thomas via groups.io <thomas.lendacky=amd.com@groups.io>
Sent: Friday, April 14, 2023 9:43:52 PM
To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@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.lendacky@amd.com>
>> Sent: Friday, April 14, 2023 12:19 AM
>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>
>>>
>>>
>>>
>>>
>>>
[-- Attachment #2: Type: text/html, Size: 19654 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-14 17:19 ` Ni, Ray
@ 2023-04-14 19:05 ` Lendacky, Thomas
2023-04-18 9:57 ` duntan
0 siblings, 1 reply; 16+ messages in thread
From: Lendacky, Thomas @ 2023-04-14 19:05 UTC (permalink / raw)
To: devel, ray.ni, Tan, Dun
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.com@groups.io>
> Sent: Friday, April 14, 2023 9:43:52 PM
> To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@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.lendacky@amd.com>
>>> Sent: Friday, April 14, 2023 12:19 AM
>>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>
>
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-14 19:05 ` Lendacky, Thomas
@ 2023-04-18 9:57 ` duntan
2023-04-18 21:06 ` Lendacky, Thomas
0 siblings, 1 reply; 16+ messages in thread
From: duntan @ 2023-04-18 9:57 UTC (permalink / raw)
To: devel@edk2.groups.io, thomas.lendacky@amd.com, Ni, Ray
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.ni@intel.com>; Tan, Dun <dun.tan@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.com@groups.io>
> Sent: Friday, April 14, 2023 9:43:52 PM
> To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@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.lendacky@amd.com>
>>> Sent: Friday, April 14, 2023 12:19 AM
>>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>
>
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-18 9:57 ` duntan
@ 2023-04-18 21:06 ` Lendacky, Thomas
2023-04-19 5:39 ` duntan
0 siblings, 1 reply; 16+ messages in thread
From: Lendacky, Thomas @ 2023-04-18 21:06 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io, Ni, Ray
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@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.ni@intel.com>; Tan, Dun <dun.tan@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.com@groups.io>
>> Sent: Friday, April 14, 2023 9:43:52 PM
>> To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@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.lendacky@amd.com>
>>>> Sent: Friday, April 14, 2023 12:19 AM
>>>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>>>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-18 21:06 ` Lendacky, Thomas
@ 2023-04-19 5:39 ` duntan
2023-04-19 13:19 ` Lendacky, Thomas
0 siblings, 1 reply; 16+ messages in thread
From: duntan @ 2023-04-19 5:39 UTC (permalink / raw)
To: devel@edk2.groups.io, thomas.lendacky@amd.com, Ni, Ray
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.
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.tan@intel.com>; devel@edk2.groups.io; Ni, Ray <ray.ni@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_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@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.ni@intel.com>; Tan, Dun
> <dun.tan@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.com@groups.io>
>> Sent: Friday, April 14, 2023 9:43:52 PM
>> To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@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.lendacky@amd.com>
>>>> Sent: Friday, April 14, 2023 12:19 AM
>>>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>>>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-19 5:39 ` duntan
@ 2023-04-19 13:19 ` Lendacky, Thomas
2023-04-20 9:07 ` duntan
0 siblings, 1 reply; 16+ messages in thread
From: Lendacky, Thomas @ 2023-04-19 13:19 UTC (permalink / raw)
To: devel, dun.tan, Ni, Ray
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.tan@intel.com>; devel@edk2.groups.io; Ni, Ray <ray.ni@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_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@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.ni@intel.com>; Tan, Dun
>> <dun.tan@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.com@groups.io>
>>> Sent: Friday, April 14, 2023 9:43:52 PM
>>> To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@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.lendacky@amd.com>
>>>>> Sent: Friday, April 14, 2023 12:19 AM
>>>>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>>>>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-19 13:19 ` Lendacky, Thomas
@ 2023-04-20 9:07 ` duntan
2023-04-20 15:45 ` Lendacky, Thomas
0 siblings, 1 reply; 16+ messages in thread
From: duntan @ 2023-04-20 9:07 UTC (permalink / raw)
To: devel@edk2.groups.io, thomas.lendacky@amd.com, Ni, Ray
Hi Tom,
Thanks for confirming. Is 1G page table supported in your test? If supported, there is a reason can explain why this difference exists.
In current upstream master branch:
Smm code always create 2M page table for [0, 4G] range(in Gen4GPageTable ) to reuse code. In QemuFlashBeforeProbe() of OvmfPkg\QemuFlashFvbServicesRuntimeDxe\QemuFlashSmm.c, the range to clear EncMask is 2M aligned. No page split happens and smm page table memory is all RW before ReadyToLock. But in my opinion, the PF also may happen if the range is not 2M aligned.
In my code branch:
If 1G page table is supported, 1G page table is created to map [0, 4G]. Then page split (1G-->2M) happens in QemuFlashBeforeProbe() and the new memory for page table is marked as RO before ReadyToLock. That's why the PF happens in InitPaging().
To solve this issue, I think the assumption that smm page table is always RW before ReadyToLock should be dropped. I have added 2 new patches to clear WP before modifying smm page table. Could you please help test again?
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: Wednesday, April 19, 2023 9:19 PM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
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.tan@intel.com>; devel@edk2.groups.io; Ni, Ray
> <ray.ni@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_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/Ue
> fiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSm
> m/DEBUG/PiSmmCpuDxeSmm.dll)
>
>
> Thanks,
> Tom
>
>>
>> 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.ni@intel.com>; Tan, Dun
>> <dun.tan@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.com@groups.io>
>>> Sent: Friday, April 14, 2023 9:43:52 PM
>>> To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@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.lendacky@amd.com>
>>>>> Sent: Friday, April 14, 2023 12:19 AM
>>>>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>>>>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-20 9:07 ` duntan
@ 2023-04-20 15:45 ` Lendacky, Thomas
2023-04-21 0:44 ` duntan
0 siblings, 1 reply; 16+ messages in thread
From: Lendacky, Thomas @ 2023-04-20 15:45 UTC (permalink / raw)
To: devel, dun.tan, Ni, Ray
On 4/20/23 04:07, duntan via groups.io wrote:
> Hi Tom,
>
> Thanks for confirming. Is 1G page table supported in your test? If supported, there is a reason can explain why this difference exists.
> In current upstream master branch:
> Smm code always create 2M page table for [0, 4G] range(in Gen4GPageTable ) to reuse code. In QemuFlashBeforeProbe() of OvmfPkg\QemuFlashFvbServicesRuntimeDxe\QemuFlashSmm.c, the range to clear EncMask is 2M aligned. No page split happens and smm page table memory is all RW before ReadyToLock. But in my opinion, the PF also may happen if the range is not 2M aligned.
>
> In my code branch:
> If 1G page table is supported, 1G page table is created to map [0, 4G]. Then page split (1G-->2M) happens in QemuFlashBeforeProbe() and the new memory for page table is marked as RO before ReadyToLock. That's why the PF happens in InitPaging().
>
> To solve this issue, I think the assumption that smm page table is always RW before ReadyToLock should be dropped. I have added 2 new patches to clear WP before modifying smm page table. Could you please help test again?
> https://github.com/td36/edk2/tree/SmmPageTable_V2
Hi Dun,
Yes, this branch successfully boots an SEV guest with SMM enabled.
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 9:19 PM
> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
>
> 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.tan@intel.com>; devel@edk2.groups.io; Ni, Ray
>> <ray.ni@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_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/Ue
>> fiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSm
>> m/DEBUG/PiSmmCpuDxeSmm.dll)
>>
>>
>> Thanks,
>> Tom
>>
>>>
>>> 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.ni@intel.com>; Tan, Dun
>>> <dun.tan@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.com@groups.io>
>>>> Sent: Friday, April 14, 2023 9:43:52 PM
>>>> To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@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.lendacky@amd.com>
>>>>>> Sent: Friday, April 14, 2023 12:19 AM
>>>>>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>>>>>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
2023-04-20 15:45 ` Lendacky, Thomas
@ 2023-04-21 0:44 ` duntan
0 siblings, 0 replies; 16+ messages in thread
From: duntan @ 2023-04-21 0:44 UTC (permalink / raw)
To: Tom Lendacky, devel@edk2.groups.io, Ni, Ray
Thanks a lot! I'll update my patch set later.
Thanks,
Dun
-----Original Message-----
From: Tom Lendacky <thomas.lendacky@amd.com>
Sent: Thursday, April 20, 2023 11:45 PM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
On 4/20/23 04:07, duntan via groups.io wrote:
> Hi Tom,
>
> Thanks for confirming. Is 1G page table supported in your test? If supported, there is a reason can explain why this difference exists.
> In current upstream master branch:
> Smm code always create 2M page table for [0, 4G] range(in Gen4GPageTable ) to reuse code. In QemuFlashBeforeProbe() of OvmfPkg\QemuFlashFvbServicesRuntimeDxe\QemuFlashSmm.c, the range to clear EncMask is 2M aligned. No page split happens and smm page table memory is all RW before ReadyToLock. But in my opinion, the PF also may happen if the range is not 2M aligned.
>
> In my code branch:
> If 1G page table is supported, 1G page table is created to map [0, 4G]. Then page split (1G-->2M) happens in QemuFlashBeforeProbe() and the new memory for page table is marked as RO before ReadyToLock. That's why the PF happens in InitPaging().
>
> To solve this issue, I think the assumption that smm page table is always RW before ReadyToLock should be dropped. I have added 2 new patches to clear WP before modifying smm page table. Could you please help test again?
> https://github.com/td36/edk2/tree/SmmPageTable_V2
Hi Dun,
Yes, this branch successfully boots an SEV guest with SMM enabled.
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 9:19 PM
> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>; Ni, Ray
> <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create
> and update smm page table
>
> 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.tan@intel.com>; devel@edk2.groups.io; Ni, Ray
>> <ray.ni@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_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/U
>> e fiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSm
>> m/DEBUG/PiSmmCpuDxeSmm.dll)
>>
>>
>> Thanks,
>> Tom
>>
>>>
>>> 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.ni@intel.com>; Tan, Dun
>>> <dun.tan@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.com@groups.io>
>>>> Sent: Friday, April 14, 2023 9:43:52 PM
>>>> To: Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@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.lendacky@amd.com>
>>>>>> Sent: Friday, April 14, 2023 12:19 AM
>>>>>> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
>>>>>> Cc: Ni, Ray <ray.ni@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.tan@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(-)
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
>
>
>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-04-21 0:44 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1755241E6695EAE7.1885@groups.io>
2023-04-12 8:58 ` [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table duntan
2023-04-12 10:17 ` duntan
2023-04-12 19:25 ` Lendacky, Thomas
2023-04-13 9:14 ` duntan
2023-04-13 16:18 ` Lendacky, Thomas
2023-04-14 5:07 ` Ni, Ray
2023-04-14 13:43 ` Lendacky, Thomas
2023-04-14 17:19 ` Ni, Ray
2023-04-14 19:05 ` Lendacky, Thomas
2023-04-18 9:57 ` duntan
2023-04-18 21:06 ` Lendacky, Thomas
2023-04-19 5:39 ` duntan
2023-04-19 13:19 ` Lendacky, Thomas
2023-04-20 9:07 ` duntan
2023-04-20 15:45 ` Lendacky, Thomas
2023-04-21 0:44 ` duntan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox