On Fri, Apr 26, 2024 at 08:51:20AM -0500, Roth, Michael via groups.io wrote: > For the most part, OVMF will clear the encryption bit for MMIO regions, > but there is currently one known exception during SEC when the APIC > base address is accessed via MMIO with the encryption bit set for > SEV-ES/SEV-SNP guests. In the case of SEV-SNP, this requires special > handling on the hypervisor side which may not be available in the > future[1], so make the necessary changes in the SEC-configured page > table to clear the encryption bit for 4K region containing the APIC > base address. > > Since CpuPageTableLib is used to handle the splitting, some additional > care must be taken to clear the C-bit in all non-leaf PTEs since the > library expects that to be the case. Add handling for that when setting > up the SEC page table.
Tom just noticed another spot where a non-leaf C-bit needs to be cleared (the one mapping the GHCB page). It doesn't affect patch functionality but should be included for completeness of this change, so will send a quick v3 with this addressed. -Mike > > While here, drop special handling for the APIC base address in the > SEV-ES/SNP #VC handler. > > [1] https://lore.kernel.org/lkml/20240208002420.34mvemnzrwwsa...@amd.com/#t > > Suggested-by: Tom Lendacky <thomas.lenda...@amd.com> > Cc: Erdem Aktas <erdemak...@google.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Min Xu <min.m...@intel.com> > Cc: Tom Lendacky <thomas.lenda...@amd.com> > Cc: Jianyong Wu <jianyong...@arm.com> > Cc: Anatol Belski <anbel...@linux.microsoft.com> > Signed-off-by: Michael Roth <michael.r...@amd.com> > --- > changes since v1: > - use CpuPageTableLib to handle splitting (Gerd, Tom) > > OvmfPkg/AmdSev/AmdSevX64.fdf | 5 +- > OvmfPkg/Bhyve/BhyveX64.dsc | 1 + > OvmfPkg/CloudHv/CloudHvX64.fdf | 5 +- > OvmfPkg/Library/CcExitLib/CcExitVcHandler.c | 12 +---- > OvmfPkg/Microvm/MicrovmX64.fdf | 3 ++ > OvmfPkg/OvmfPkg.dec | 5 ++ > OvmfPkg/OvmfPkgX64.fdf | 5 +- > OvmfPkg/ResetVector/Ia32/PageTables64.asm | 20 +++---- > OvmfPkg/Sec/AmdSev.c | 58 +++++++++++++++++++++ > OvmfPkg/Sec/AmdSev.h | 14 +++++ > OvmfPkg/Sec/SecMain.c | 1 + > OvmfPkg/Sec/SecMain.inf | 3 ++ > 12 files changed, 108 insertions(+), 24 deletions(-) > > diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf > index d49555c6c8..595945181c 100644 > --- a/OvmfPkg/AmdSev/AmdSevX64.fdf > +++ b/OvmfPkg/AmdSev/AmdSevX64.fdf > @@ -77,7 +77,10 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.Pcd > 0x010C00|0x000400 > > > gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize > > > > -0x011000|0x00F000 > > +0x011000|0x001000 > > +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableSize > > + > > +0x012000|0x00E000 > > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > > > 0x020000|0x0E0000 > > diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc > index 6f305d690d..78050959f8 100644 > --- a/OvmfPkg/Bhyve/BhyveX64.dsc > +++ b/OvmfPkg/Bhyve/BhyveX64.dsc > @@ -174,6 +174,7 @@ > PeiHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/PeiHardwareInfoLib.inf > > DxeHardwareInfoLib|OvmfPkg/Library/HardwareInfoLib/DxeHardwareInfoLib.inf > > > ImagePropertiesRecordLib|MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.inf > > + CpuPageTableLib|UefiCpuPkg/Library/CpuPageTableLib/CpuPageTableLib.inf > > > > > CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf > > > FrameBufferBltLib|MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.inf > > diff --git a/OvmfPkg/CloudHv/CloudHvX64.fdf b/OvmfPkg/CloudHv/CloudHvX64.fdf > index eae3ada191..3e6688b103 100644 > --- a/OvmfPkg/CloudHv/CloudHvX64.fdf > +++ b/OvmfPkg/CloudHv/CloudHvX64.fdf > @@ -76,7 +76,10 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCp > 0x00F000|0x001000 > > > gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtr|gUefiOvmfPkgTokenSpaceGuid.PcdXenPvhStartOfDayStructPtrSize > > > > -0x010000|0x010000 > > +0x010000|0x001000 > > +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableSize > > + > > +0x011000|0x00F000 > > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > > > 0x020000|0x0E0000 > > diff --git a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c > b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c > index 549375dfed..da8f1e5db9 100644 > --- a/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c > +++ b/OvmfPkg/Library/CcExitLib/CcExitVcHandler.c > @@ -98,7 +98,7 @@ UnsupportedExit ( > Validate that the MMIO memory access is not to encrypted memory. > > > > Examine the pagetable entry for the memory specified. MMIO should not be > > - performed against encrypted memory. MMIO to the APIC page is always > allowed. > > + performed against encrypted memory. > > > > @param[in] Ghcb Pointer to the Guest-Hypervisor Communication > Block > > @param[in] MemoryAddress Memory address to validate > > @@ -118,16 +118,6 @@ ValidateMmioMemory ( > { > > MEM_ENCRYPT_SEV_ADDRESS_RANGE_STATE State; > > GHCB_EVENT_INJECTION GpEvent; > > - UINTN Address; > > - > > - // > > - // Allow APIC accesses (which will have the encryption bit set during > > - // SEC and PEI phases). > > - // > > - Address = MemoryAddress & ~(SIZE_4KB - 1); > > - if (Address == GetLocalApicBaseAddress ()) { > > - return 0; > > - } > > > > State = MemEncryptSevGetAddressRangeState ( > > 0, > > diff --git a/OvmfPkg/Microvm/MicrovmX64.fdf b/OvmfPkg/Microvm/MicrovmX64.fdf > index 825bf9f5e4..055e659a35 100644 > --- a/OvmfPkg/Microvm/MicrovmX64.fdf > +++ b/OvmfPkg/Microvm/MicrovmX64.fdf > @@ -62,6 +62,9 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvm > 0x00C000|0x001000 > > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize > > > > +0x00D000|0x001000 > > +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableSize > > + > > 0x010000|0x010000 > > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 2f7bded926..b23219ebd4 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -277,6 +277,11 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|0|UINT32|0x44 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize|0|UINT32|0x45 > > > > + ## Specify the extra page table needed to mark the APIC MMIO range as > unencrypted. > > + # The value should be a multiple of 4KB for each. > > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableBase|0x0|UINT32|0x72 > > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableSize|0x0|UINT32|0x73 > > + > > ## The base address and size of the SEV Launch Secret Area provisioned > > # after remote attestation. If this is set in the .fdf, the platform > > # is responsible for protecting the area from DXE phase overwrites. > > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index c2d3cc901e..b6e8f43566 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -97,7 +97,10 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCp > 0x00F000|0x001000 > > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecSvsmCaaBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecSvsmCaaSize > > > > -0x010000|0x010000 > > +0x010000|0x001000 > > +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableSize > > + > > +0x011000|0x00F000 > > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > > > 0x020000|0x0E0000 > > diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm > b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > index 474d22dbfa..d913a39d46 100644 > --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm > +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm > @@ -67,7 +67,7 @@ BITS 32 > ; > > ; Create page tables for 4-level paging > > ; > > -; Argument: upper 32 bits of the page table entries > > +; Argument: upper 32 bits of the leaf page table entries > > ; > > %macro CreatePageTables4Level 1 > > > > @@ -78,19 +78,19 @@ BITS 32 > ; Top level Page Directory Pointers (1 * 512GB entry) > > ; > > mov dword[PT_ADDR (0)], PT_ADDR (0x1000) + PAGE_PDE_DIRECTORY_ATTR > > - mov dword[PT_ADDR (4)], %1 > > + mov dword[PT_ADDR (4)], 0 > > > > ; > > ; Next level Page Directory Pointers (4 * 1GB entries => 4GB) > > ; > > mov dword[PT_ADDR (0x1000)], PT_ADDR (0x2000) + > PAGE_PDE_DIRECTORY_ATTR > > - mov dword[PT_ADDR (0x1004)], %1 > > + mov dword[PT_ADDR (0x1004)], 0 > > mov dword[PT_ADDR (0x1008)], PT_ADDR (0x3000) + > PAGE_PDE_DIRECTORY_ATTR > > - mov dword[PT_ADDR (0x100C)], %1 > > + mov dword[PT_ADDR (0x100C)], 0 > > mov dword[PT_ADDR (0x1010)], PT_ADDR (0x4000) + > PAGE_PDE_DIRECTORY_ATTR > > - mov dword[PT_ADDR (0x1014)], %1 > > + mov dword[PT_ADDR (0x1014)], 0 > > mov dword[PT_ADDR (0x1018)], PT_ADDR (0x5000) + > PAGE_PDE_DIRECTORY_ATTR > > - mov dword[PT_ADDR (0x101C)], %1 > > + mov dword[PT_ADDR (0x101C)], 0 > > > > ; > > ; Page Table Entries (2048 * 2MB entries => 4GB) > > @@ -141,7 +141,7 @@ BITS 32 > ; > > ; Create page tables for 5-level paging with gigabyte pages > > ; > > -; Argument: upper 32 bits of the page table entries > > +; Argument: upper 32 bits of the leaf page table entries > > ; > > ; We have 6 pages available for the early page tables, > > ; we use four of them: > > @@ -164,15 +164,15 @@ BITS 32 > > > ; level 5 > > mov dword[PT_ADDR (0)], PT_ADDR (0x1000) + PAGE_PDE_DIRECTORY_ATTR > > - mov dword[PT_ADDR (4)], %1 > > + mov dword[PT_ADDR (4)], 0 > > > > ; level 4 > > mov dword[PT_ADDR (0x1000)], PT_ADDR (0x3000) + > PAGE_PDE_DIRECTORY_ATTR > > - mov dword[PT_ADDR (0x1004)], %1 > > + mov dword[PT_ADDR (0x1004)], 0 > > > > ; level 3 (1x -> level 2, 3x 1GB) > > mov dword[PT_ADDR (0x3000)], PT_ADDR (0x2000) + > PAGE_PDE_DIRECTORY_ATTR > > - mov dword[PT_ADDR (0x3004)], %1 > > + mov dword[PT_ADDR (0x3004)], 0 > > mov dword[PT_ADDR (0x3008)], (1 << 30) + PAGE_PDE_LARGEPAGE_ATTR > > mov dword[PT_ADDR (0x300c)], %1 > > mov dword[PT_ADDR (0x3010)], (2 << 30) + PAGE_PDE_LARGEPAGE_ATTR > > diff --git a/OvmfPkg/Sec/AmdSev.c b/OvmfPkg/Sec/AmdSev.c > index 520b125132..89fba2fd18 100644 > --- a/OvmfPkg/Sec/AmdSev.c > +++ b/OvmfPkg/Sec/AmdSev.c > @@ -8,7 +8,10 @@ > **/ > > > > #include <Library/BaseLib.h> > > +#include <Library/CpuLib.h> > > +#include <Library/CpuPageTableLib.h> > > #include <Library/DebugLib.h> > > +#include <Library/LocalApicLib.h> > > #include <Library/MemEncryptSevLib.h> > > #include <Library/BaseMemoryLib.h> > > #include <Register/Amd/Ghcb.h> > > @@ -301,3 +304,58 @@ SecValidateSystemRam ( > MemEncryptSevSnpPreValidateSystemRam (Start, EFI_SIZE_TO_PAGES > ((UINTN)(End - Start))); > > } > > } > > + > > +/** > > + Map known MMIO regions unencrypted if SEV-ES is active. > > + > > + During early booting, page table entries default to having the encryption > bit > > + set for SEV-ES/SEV-SNP guests. In cases where there is MMIO to an address, > the > > + encryption bit should be cleared. Clear it here for any known MMIO accesses > > + during SEC, which is currently just the APIC base address. > > + > > +**/ > > +VOID > > +SecMapApicBaseUnencrypted ( > > + VOID > > + ) > > +{ > > + PHYSICAL_ADDRESS Cr3; > > + UINT64 ApicAddress; > > + VOID *Buffer; > > + UINTN BufferSize; > > + IA32_MAP_ATTRIBUTE MapAttribute; > > + IA32_MAP_ATTRIBUTE MapMask; > > + RETURN_STATUS Status; > > + > > + if (!SevEsIsEnabled ()) { > > + return; > > + } > > + > > + ApicAddress = (UINT64)GetLocalApicBaseAddress (); > > + Buffer = (VOID *)(UINTN)FixedPcdGet32 (PcdOvmfSecApicPageTableBase); > > + Cr3 = AsmReadCr3 (); > > + > > + MapAttribute.Uint64 = ApicAddress; > > + MapAttribute.Bits.Present = 1; > > + MapAttribute.Bits.ReadWrite = 1; > > + MapMask.Uint64 = MAX_UINT64; > > + BufferSize = SIZE_4KB; > > + > > + Status = PageTableMap ( > > + (UINTN *)&Cr3, > > + Paging4Level, > > + Buffer, > > + &BufferSize, > > + ApicAddress, > > + SIZE_4KB, > > + &MapAttribute, > > + &MapMask, > > + NULL > > + ); > > + if (RETURN_ERROR (Status)) { > > + DEBUG ((DEBUG_ERROR, "Failed to map APIC MMIO region as unencrypted: > %d\n", Status)); > > + ASSERT (FALSE); > > + } > > + > > + CpuFlushTlb (); > > +} > > diff --git a/OvmfPkg/Sec/AmdSev.h b/OvmfPkg/Sec/AmdSev.h > index f75877096e..c5ab0d5a0b 100644 > --- a/OvmfPkg/Sec/AmdSev.h > +++ b/OvmfPkg/Sec/AmdSev.h > @@ -91,4 +91,18 @@ SevSnpIsEnabled ( > VOID > > ); > > > > +/** > > + Map MMIO regions unencrypted if SEV-ES is active. > > + > > + During early booting, page table entries default to having the encryption > bit > > + set for SEV-ES/SEV-SNP guests. In cases where there is MMIO to an address, > the > > + encryption bit should be cleared. Clear it here for any known MMIO accesses > > + during SEC, which is currently just the APIC base address. > > + > > +**/ > > +VOID > > +SecMapApicBaseUnencrypted ( > > + VOID > > + ); > > + > > #endif > > diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c > index a30d4ce09e..60dfa61842 100644 > --- a/OvmfPkg/Sec/SecMain.c > +++ b/OvmfPkg/Sec/SecMain.c > @@ -938,6 +938,7 @@ SecCoreStartupWithStack ( > // interrupts before initializing the Debug Agent and the debug timer is > > // enabled. > > // > > + SecMapApicBaseUnencrypted (); > > InitializeApicTimer (0, MAX_UINT32, TRUE, 5); > > DisableApicTimerInterrupt (); > > > > diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf > index dca932a474..88c2d3fb6d 100644 > --- a/OvmfPkg/Sec/SecMain.inf > +++ b/OvmfPkg/Sec/SecMain.inf > @@ -55,6 +55,7 @@ > MemEncryptSevLib > > CpuExceptionHandlerLib > > CcProbeLib > > + CpuPageTableLib > > > > [Ppis] > > gEfiTemporaryRamSupportPpiGuid # PPI ALWAYS_PRODUCED > > @@ -83,6 +84,8 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase > > gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase > > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableBase > > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecApicPageTableSize > > > > [FeaturePcd] > > gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > > -- > 2.25.1 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118342): https://edk2.groups.io/g/devel/message/118342 Mute This Topic: https://groups.io/mt/105750506/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
