From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0C8912095A334 for ; Wed, 24 May 2017 07:17:33 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 656EA9D0E0; Wed, 24 May 2017 14:17:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 656EA9D0E0 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 656EA9D0E0 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-23.phx2.redhat.com [10.3.116.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id D19447D6A8; Wed, 24 May 2017 14:17:30 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org, jordan.l.justen@intel.com Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, Jiewen Yao References: <1495466592-21641-1-git-send-email-brijesh.singh@amd.com> <1495466592-21641-7-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <6ec289bb-e0d3-6744-73de-57c8a7f0f55d@redhat.com> Date: Wed, 24 May 2017 16:17:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <1495466592-21641-7-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 24 May 2017 14:17:32 +0000 (UTC) Subject: Re: [PATCH v5 06/14] OvmfPkg:AmdSevDxe: Add AmdSevDxe driver X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 May 2017 14:17:33 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Laszlo Ersek > Cc: Leo Duran > Cc: Jiewen Yao > Contributed-under: TianoCore Contribution Agreement 1.0 > Suggested-by: Jiewen Yao > Signed-off-by: Brijesh Singh > Reviewed-by: Jiewen Yao > --- > 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.
> +# > +# 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.
> + > + 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 > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +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 Thanks Laszlo