cosmetic comments if a v6 is needed: On 05/22/17 17:23, Brijesh Singh wrote: > When SEV is enabled, the MMIO memory range must be mapped as unencrypted > (i.e C-bit cleared). > > We need to clear the C-bit for MMIO GCD entries in order to cover the > ranges that were added during the PEI phase (through memory resource > descriptor HOBs). Additionally, the NonExistent ranges are processed > in order to cover, in advance, MMIO ranges added later in the DXE phase > by various device drivers, via the appropriate DXE memory space services. > > The approach is not transparent for later addition of system memory ranges > to the GCD memory space map. (Such ranges should be encrypted.) OVMF does > not do such a thing at the moment, so this approach should be OK. > > The driver is being added to the APRIORI DXE file so that, we clear the > C-bit from MMIO regions before any driver accesses it. > > > Cc: Jordan Justen <[email protected]> > Cc: Laszlo Ersek <[email protected]> > Cc: Leo Duran <[email protected]> > Cc: Jiewen Yao <[email protected]> > Contributed-under: TianoCore Contribution Agreement 1.0 > Suggested-by: Jiewen Yao <[email protected]> > Signed-off-by: Brijesh Singh <[email protected]> > Reviewed-by: Jiewen Yao <[email protected]> > --- > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.fdf | 2 + > OvmfPkg/OvmfPkgX64.fdf | 2 + > OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 43 ++++++++++++ > OvmfPkg/AmdSevDxe/AmdSevDxe.c | 71 ++++++++++++++++++++ > 6 files changed, 120 insertions(+) > > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index ef245635224c..daf2faadea35 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -822,6 +822,7 @@ [Components.X64] > !endif > > OvmfPkg/PlatformDxe/Platform.inf > + OvmfPkg/AmdSevDxe/AmdSevDxe.inf > > !if $(SMM_REQUIRE) == TRUE > OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 0a693f2772a7..6189088da86c 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -820,6 +820,7 @@ [Components] > !endif > > OvmfPkg/PlatformDxe/Platform.inf > + OvmfPkg/AmdSevDxe/AmdSevDxe.inf > > !if $(SMM_REQUIRE) == TRUE > OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > index 5233314139bc..12871860d001 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -190,6 +190,7 @@ [FV.DXEFV] > APRIORI DXE { > INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf > INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf > + INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf > !if $(SMM_REQUIRE) == FALSE > INF OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > !endif > @@ -351,6 +352,7 @@ [FV.DXEFV] > INF OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > INF OvmfPkg/PlatformDxe/Platform.inf > +INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf > > !if $(SMM_REQUIRE) == TRUE > INF OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index 36150101e784..ae6e66a1c08d 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -190,6 +190,7 @@ [FV.DXEFV] > APRIORI DXE { > INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf > INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf > + INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf > !if $(SMM_REQUIRE) == FALSE > INF OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf > !endif > @@ -351,6 +352,7 @@ [FV.DXEFV] > INF OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf > INF OvmfPkg/VirtioGpuDxe/VirtioGpu.inf > INF OvmfPkg/PlatformDxe/Platform.inf > +INF OvmfPkg/AmdSevDxe/AmdSevDxe.inf > > !if $(SMM_REQUIRE) == TRUE > INF OvmfPkg/SmmAccess/SmmAccess2Dxe.inf > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > new file mode 100644 > index 000000000000..41635a57a454 > --- /dev/null > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf > @@ -0,0 +1,43 @@ > +#/** @file > +# > +# Driver clears the encryption attribute from MMIO regions when SEV is > enabled > +# > +# Copyright (c) 2017, AMD Inc. All rights reserved.<BR> > +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD > +# License which accompanies this distribution. The full text of the > license may > +# be found at http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > +# > +#**/ > + > +[Defines] > + INF_VERSION = 1.25 > + BASE_NAME = AmdSevDxe > + FILE_GUID = 2ec9da37-ee35-4de9-86c5-6d9a81dc38a7 > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = AmdSevDxeEntryPoint > + > +[Sources] > + AmdSevDxe.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + BaseLib > + UefiLib > + UefiDriverEntryPoint > + UefiBootServicesTableLib > + DxeServicesTableLib > + DebugLib > + MemEncryptSevLib > + > +[Depex] > + TRUE > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > new file mode 100644 > index 000000000000..c483ae1419fd > --- /dev/null > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > @@ -0,0 +1,71 @@ > +/** @file > + > + AMD Sev Dxe driver. The driver runs in APRIORI phase and clears C-bit from
(1) "APRIORI" is not a phase, but a file in the firmware volume that instructs the DXE core to do things. Please refer to "10.3 The A Priori File" in Volume 2 of the Platform Init spec. So the comment should say, "this driver is dispatched early in DXE, due to being listed in the APRIORI DXE file". > + MMIO and NonExistent Memory space when SEV is enabled. > + > + Copyright (c) 2017, AMD Inc. All rights reserved.<BR> > + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD > + License which accompanies this distribution. The full text of the license > may > + be found at http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > + > +**/ > + > +#include <PiDxe.h> > + > +#include <Library/BaseLib.h> > +#include <Library/DebugLib.h> > +#include <Library/BaseMemoryLib.h> > +#include <Library/MemoryAllocationLib.h> > +#include <Library/UefiBootServicesTableLib.h> > +#include <Library/DxeServicesTableLib.h> > +#include <Library/MemEncryptSevLib.h> > + > +EFI_STATUS > +EFIAPI > +AmdSevDxeEntryPoint ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR *AllDescMap; > + UINTN NumEntries; > + UINTN Index; > + > + // > + // Do nothing when SEV is not enabled > + // > + if (!MemEncryptSevIsEnabled ()) { > + return EFI_UNSUPPORTED; > + } > + > + // > + // Iterate through the GCD map and clear the C-bit from MMIO and > NonExistent > + // memory space. The NonExistent memory space will be used for mapping the > MMIO > + // space added later (eg PciRootBridge). By clearing both known MMIO and > NonExistent > + // memory space can gurantee that current and furture MMIO adds will have > + // C-bit cleared. > + // > + Status = gDS->GetMemorySpaceMap (&NumEntries, &AllDescMap); > + if (Status == EFI_SUCCESS) { (2) It is a bit more idiomatic to write: !EFI_ERROR (Status)) > + for (Index = 0; Index < NumEntries; Index++) { > + CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Desc; > + > + Desc = &AllDescMap[Index]; > + if (Desc->GcdMemoryType == EfiGcdMemoryTypeMemoryMappedIo || > + Desc->GcdMemoryType == EfiGcdMemoryTypeNonExistent) { > + Status = MemEncryptSevClearPageEncMask (0, Desc->BaseAddress, > EFI_SIZE_TO_PAGES(Desc->Length), FALSE); > + ASSERT_EFI_ERROR(Status); (3) Missing space after opening paren. > + } > + } > + > + FreePool (AllDescMap); > + } > + > + return EFI_SUCCESS; > +} > (4) The lines are too long in this file. If no v6 is necessary, I'm OK with the patch as-is. Reviewed-by: Laszlo Ersek <[email protected]> Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
