Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and update smm page table

Fri, 14 Apr 2023 12:05:29 -0700 

On 4/14/23 12:19, Ni, Ray via groups.io wrote:

tom,
if the c bit is not required for non leaf page table entries, why the trunk 
code sets the c bit for all entities including nonleaf ones?



Because it's effectively the correct thing to do, even though it doesn't 
matter.




i went back to read again the smm issue you met. you said the c bit is set for 
non leaf entries that caused a deference issue. But the pagetablelib code 
doesn't set c bit to non leaf entries. then who sets the c bit?



I guess that's the main question, how did that get set? I haven't had the 
time to fully examine and follow the codepath in the pagetable library to 
figure out why it was set. Maybe as part of a page split?


Thanks,
Tom



thanks,
ray

thanks,
ray
________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Lendacky, Thomas via 
groups.io <thomas.lendacky=amd....@groups.io>
Sent: Friday, April 14, 2023 9:43:52 PM
To: Ni, Ray <ray...@intel.com>; Tan, Dun <dun....@intel.com>; devel@edk2.groups.io 
<devel@edk2.groups.io>
Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and 
update smm page table

On 4/14/23 00:07, Ni, Ray wrote:




-----Original Message-----
From: Tom Lendacky <thomas.lenda...@amd.com>
Sent: Friday, April 14, 2023 12:19 AM
To: Tan, Dun <dun....@intel.com>; devel@edk2.groups.io
Cc: Ni, Ray <ray...@intel.com>
Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and
update smm page table

On 4/13/23 04:14, Tan, Dun wrote:

Hi Tom,

Thank you for your help with testing.
For the build failure, it's because that the CpuPageTableLib instance is

added into OvmfPkg DSC in the last pacth ' OvmfPkg: Add CpuPageTableLib
required by PiSmmCpuDxe'. I have moved this patch to the head of the patch
set.


For the boot failure, I think it's because that the encrypt mask was not

applied to the memory used by page table in page table non-leaf entry.
Initially I thought the encrypt mask would only be applied to the leaf entry in
AMD SEV feature. So I treated the encryption process as non 1:1 mapping,
which only applies the encrypt mask to leaf entry. I'm also curious why the
DxeIpl patch set works good. All the page table non-leaf entries are also not
encrypted in the DxeIpl page table related patch set.

Right, and that works for SEV. All non-leaf pagetable entries are treated
as encrypted regardless of the encryption bit. Since the tables were built
being mapped encrypted, the pagetable walk works when the non-leaf
entries don't have the encryption bit set. In this case, though, the encryption
bit is present in the non-leaf entry and that is the reason why there are
issues.


Can you point us which doc here (https://www.amd.com/en/developer/sev.html)
says the page table is encrypted regardless the KEY_ID bits value?
How can the encryption engine know if a chunk of memory belongs to page table?


It doesn't. For an SEV guest, when the hardware walks the pagetables, it
will always treat the memory accesses as encrypted (see section 15.34.5 of
the AMD APM Vol 2 at https://www.amd.com/system/files/TechDocs/24593.pdf).

But, because the initial pagetables that are built to map everything as
encrypted/private to start with (see
OvmfPkg/ResetVector/Ia32/PageTables64.asm), only changing to shared when
specifically requested, any memory allocated and used will be encrypted.
Thus, when new pagetables are allocated/created in the CpuPageTableLib
library, they will be encrypted and so everything works. And those new
pagetables will map everything encrypted by default, except for the GHCB
pages. If they were mapped shared when they were created, then the
pagetable walk would fail.



My understanding to SEV is any physical address field in guest page table 
should have
the KEY_ID bits set if the physical pages are private to guest. Only some pages 
for GMCB
don't have KEY_ID bits set as those are shared between guest and host.


Right, the encryption bit in the leaf entry of the pagetables will
determine the encryption mode.



I thought Dun's patch works because all guest memory is marked as shared because
the KEY_ID bits in all entries are not set. Only some pages that're used by GMCB
have the KEY_ID bits set.


Just the opposite, the CpuPageTableLib library marks everything encrypted
and only clears the encryption bit for the GHCB pages.

In MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c, the
CreateIdentityMappingPageTables() function retrieves the encryption bit
and saves it in AddressEncMask. AddressEncMask is then applied to the
mapping attribute used when calling CreateOrUpdatePageTable() to build the
initial pagetables.






Here is some debug after setting PagingEntry at line 436 of
UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c:

*** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF81000
*** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF80000
*** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF83000
*** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF81000
*** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF80000
*** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF83000
*** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF81000
*** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 3FF80000
*** DEBUG: PageTableLibMapInLevel:437 - PagingEntry = 800003FC01000


Are you testing the SME or SEV?
My understanding is with SME, only the highest C bit should be set indicating
the physical page is encrypted.


I am testing SEV. There is only a single bit to indicate whether a page is
encrypted. The guest ASID is used to determine what key is used to decrypt
the page. From a pagetable leaf entry, SME and SEV are equivalent, the
encryption bit determines how the memory will be accessed.

SME and SEV differ in how they deal with instruction fetches and pagetable
walks, with SME obeying the encryption bit and SEV always performing the
accesses as encrypted accesses for security.

Thanks,
Tom






!!!! X64 Exception Type - 0D(#GP - General Protection)  CPU Apic ID -
00000000 !!!!

0x800003FC01000 isn't mapped and so it fails - I'm not exactly sure how
the #PF turns into a #GP, though, maybe because the virtual address isn't
canonical that point.

Thanks,
Tom



I'll added another patch in my code branch to fix this issue later. In the new

commit, from the perspective of CpuPageTableLib, the whole memory can
be divided into 3 categories: memory used by page table, guest private
memory and guest shared memory. CpuPageTableLib will always apply the
encrypt mask to memory used by page table, which means all the non-leaf
page table entries are encrypted. For guest private memory, this case can be
treated as non-1:1 mapping. We can apply the encrypt mask by setting the
input parameter of PageTableMap() API like " Attribute.Uint64 =
LinearAddress | AddressEncMask". For guest shared memory, this case can
be treated as normal 1:1 mapping. I'll let you know once the new patch is
ready.


Thanks,
Dun
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of

Lendacky, Thomas via groups.io

Sent: Thursday, April 13, 2023 3:26 AM
To: devel@edk2.groups.io; Tan, Dun <dun....@intel.com>
Subject: Re: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create

and update smm page table


On 4/12/23 05:17, duntan via groups.io wrote:

Hi Tom,

This patch set is to change PiSmmCpuDxeSmm code to use

CpuPageTableLib to create and update SMM page table. The Pcd
PcdPteMemoryEncryptionAddressOrMask is also used in PiSmmCpuDxeSmm
code and the whole range covered by page table is mapped encrypted,
which is different from the situation in DxeIpl module.

So could you also help do a test to make sure the AMD SEV feature still

works good in SMM with this patch set?

Here is the code branch in my fork repo:
https://github.com/td36/edk2/commits/SmmPageTable_V2


Hi Dun,

I tested at the final commit of the branch and encountered a #GP with an

SEV guest. It looks like the CpuPageTableLibrary doesn't take the encryption
bit into account. For example:


Line 436 of UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableMap.c
      PagingEntry = (IA32_PAGING_ENTRY

*)(UINTN)IA32_PNLE_PAGE_TABLE_BASE_ADDRESS (&ParentPagingEntry-

Pnle);

This will get an address with the encryption bit set and then try to

reference it. When I clear the encryption bit, the code proceeds a bit further,
but then encounters a #GP in a different location.


So it appears that the CpuPageTableLibrary doesn't deal with the

encryption bit properly.


Also, going through a build/test of each individual patch had mixed results.

      - With the second patch in the series applied, I get a build error:

        /root/kernels/ovmf-dun-build-X64/OvmfPkg/OvmfPkgX64.dsc(...):

error 4000: Instance of library class [CpuPageTableLib] is not found

                in [/root/kernels/ovmf-dun-build-

X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf] [X64]

                consumed by module [/root/kernels/ovmf-dun-build-

X64/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf]


        that isn't resolved until the final patch.

Thanks,
Tom



Thanks,
Dun

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of

duntan

Sent: Wednesday, April 12, 2023 4:54 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] [Patch V2 0/8] Use CpuPageTableLib to create and
update smm page table

In V2 patch set:
1.In 'Refinement to code about updating smm page table', use QuickSort()

in BaseLib instead or PerformQuickSort() in BaseSortLib.

2.Remove the patch to add BaseSortLib in DSC file.
3.Add a new patch to add CpuPageTableLib in UefiCpuPkg.dsc.
4.Add a temp patch to add CpuPageTableLib in OvmfPkg dsc files for
test(A previous patch I sent before '[Patch V2 4/8] OvmfPkg: Add
CpuPageTableLib required by DxeIpl in DSC file' contains all the
changes in this patch)

Dun Tan (8):
      OvmfPkg: Add CpuPageTableLib required by PiSmmCpuDxe
      UefiPayloadPkg: Add CpuPageTableLib required by PiSmmCpuDxe
      UefiCpuPkg: Use CpuPageTableLib to convert SMM paging attribute.
      UefiCpuPkg/PiSmmCpuDxeSmm: Avoid setting non-present range to

RO/NX

      UefiCpuPkg: Extern mSmmShadowStackSize in PiSmmCpuDxeSmm.h
      UefiCpuPkg: Refinement to current smm page table generation code
      UefiCpuPkg: Refinement to code about updating smm page table
      UefiCpuPkg/PiSmmCpuDxeSmm: Remove unnecessary function

     OvmfPkg/CloudHv/CloudHvX64.dsc                     |   2 +-
     OvmfPkg/OvmfPkgIa32.dsc                            |   3 ++-
     OvmfPkg/OvmfPkgIa32X64.dsc                         |   2 +-
     OvmfPkg/OvmfPkgX64.dsc                             |   2 +-
     UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/PageTbl.c           |   5 +++--
     UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c      |   3 +--
     UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmProfileArch.c    |   2 +-
     UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c              | 132 -----------------

----------------------------------------------------------------------------------------------
---------------------

     UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c         |   8 ++++++-

-

     UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  97

++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++-------------------------------------

     UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
     UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 629

++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++++++++++++++++++++++++++++++++++--------------------------
----------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------
-----------------------------------------------

     UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c             | 348

++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+++++++++-------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------
--------------------------------------------------

     UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c            | 229

++++++++++++++++++++++++++++++----------------------------------------------
----------------------------------------------------------------------------------------------
-----------------------------------------------------------

     UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c       |   3 +--
     UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmProfileArch.c     |  19 ++-------

----------

     UefiPayloadPkg/UefiPayloadPkg.dsc                  |   2 +-
     17 files changed, 510 insertions(+), 977 deletions(-)























-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103022): https://edk2.groups.io/g/devel/message/103022
Mute This Topic: https://groups.io/mt/98215421/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

























					Reply via email to