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 96F5B820E7 for ; Thu, 16 Feb 2017 13:56:02 -0800 (PST) Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 048A580F7C; Thu, 16 Feb 2017 21:56:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-72.phx2.redhat.com [10.3.116.72]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v1GLu1Yx031232; Thu, 16 Feb 2017 16:56:02 -0500 To: Leo Duran References: <1487278926-14303-1-git-send-email-leo.duran@amd.com> From: Laszlo Ersek Cc: edk2-devel@ml01.01.org Message-ID: Date: Thu, 16 Feb 2017 22:56:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <1487278926-14303-1-git-send-email-leo.duran@amd.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 16 Feb 2017 21:56:03 +0000 (UTC) Subject: Re: [PATCH v3 0/4] Add PCD PcdPteMemoryEncryptionAddressOrMask X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Feb 2017 21:56:02 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 02/16/17 22:02, Leo Duran wrote: > This new PCD holds the address mask for page table entries when memory > encryption is enabled on AMD processors supporting the Secure Encrypted > Virtualization (SEV) feature. > > This mask is be applied when creating 1:1 virtual to physical mapping tables. > For example, the OvmfPkg sets the PCD when launching SEV-enabled guests. > > Changes since v2: > - Apply mask to PCD to keep value within Address field > - Use variable instead of PCD in Page-Fault Handler > > To-Do: > - Add PCD support for page tables in UefiCpuPg/PiSmmCpuDxeSmm >>From a very superficial skim -- none of the patches are for OvmfPkg after all! :) --, this approach looks good to me. If I recall correctly, we're going to set the PCD in OVMF dynamically. Is that right? That usually requires investigating the possible orderings between "PCD producer" and "PCD consumers". For DXE_DRIVER and DXE_SMM_DRIVER consumers, it is good enough to set the PCD in OvmfPkg/PlatformPei (which is a PEIM, so it's bound to run earlier -- PEI comes before DXE). For PEIM consumers though, more thought might be necessary. OvmfPkg does not pull in CapsulePei, so no need to worry about that. Wrt. S3Resume2Pei: the dispatch order of S3Resume2Pei vs. PlatformPei is unspecified (both have plain TRUE depexes). However, S3Resume2Pei does not consume the PCD in its entry point function, only on the following call path: DxeLoadCore() [MdeModulePkg/Core/DxeIplPeim/DxeLoad.c] S3RestoreConfig2() [UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c] RestoreS3PageTables() PcdGet64(...) IOW, the PCD is not consumed when S3Resume2Pei starts, but when the DXE Initial Program Load PEIM realizes, at the end of the PEI phase, that we're resuming, so it branches to EFI_PEI_S3_RESUME2_PPI, rather than loading the DXE core (and starting the DXE phase). So, I think setting the PCD in OvmfPkg/PlatformPei will be safe. (BTW, if we had no such ordering guarantees between PlatformPei and another PEIM, we could still make it work: we could hook a NULL class library instance into all such PEIMs, and the lib constructor function would (re-)set the PCD afresh in each, from whatever hw characteristics SEV is detectable. Edk2 is awesome like that. For an instance of this pattern (albeit in the DXE phase), please search the OVMF DSC files for "OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.inf".) ... Perhaps this has been obvious, but I thought it wouldn't be useless to mention. Thanks Laszlo > Leo Duran (4): > MdeModulePkg: Add PCD PcdPteMemoryEncryptionAddressOrMask > MdeModulePkg/Universal/CapsulePei: Add support for PCD > PcdPteMemoryEncryptionAddressOrMask > UefiCpuPkg/Universal/Acpi/S3Resume2Pei: Add support for PCD > PcdPteMemoryEncryptionAddressOrMask > MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe: Add support for > PCD PcdPteMemoryEncryptionAddressOrMask > > MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +++- > MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 29 ++++++++++++++++------ > MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 6 +++++ > MdeModulePkg/MdeModulePkg.dec | 8 ++++++ > .../BootScriptExecutorDxe.inf | 2 ++ > .../Acpi/BootScriptExecutorDxe/ScriptExecute.c | 7 ++++++ > .../Acpi/BootScriptExecutorDxe/ScriptExecute.h | 2 ++ > .../Acpi/BootScriptExecutorDxe/X64/SetIdtEntry.c | 16 ++++++++---- > MdeModulePkg/Universal/CapsulePei/CapsulePei.inf | 2 ++ > .../Universal/CapsulePei/Common/CommonHeader.h | 7 ++++++ > MdeModulePkg/Universal/CapsulePei/UefiCapsule.c | 13 +++++++--- > MdeModulePkg/Universal/CapsulePei/X64/X64Entry.c | 23 +++++++++++------ > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 16 +++++++++--- > .../Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf | 2 ++ > 14 files changed, 109 insertions(+), 29 deletions(-) >