From: "duntan" <dun.tan@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"thomas.lendacky@amd.com" <thomas.lendacky@amd.com>,
"Ni, Ray" <ray.ni@intel.com>
Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table
Date: Tue, 18 Apr 2023 09:57:30 +0000 [thread overview]
Message-ID: <BN9PR11MB548310FFD5C681C9CF8995DAE59D9@BN9PR11MB5483.namprd11.prod.outlook.com> (raw)
In-Reply-To: <4c5afff9-39b6-6f3a-ce05-0aadb8b004b9@amd.com>
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(-)
>>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>
>
>
>
>
>
>
>
>
>
>
>
next prev parent reply other threads:[~2023-04-18 9:57 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
[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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN9PR11MB548310FFD5C681C9CF8995DAE59D9@BN9PR11MB5483.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox