From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.11668.1620734135381766754 for ; Tue, 11 May 2021 04:55:35 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=DqsRX4YQ; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1620734134; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=iEVVGOn7hvR5FMTGCLt4Gmu2ue1y8/4tuSrAk9aoRqo=; b=DqsRX4YQn0hqnxzWwWVPjBhlB2DK7urRRIg7AC0l4sncuODsjbXNT3Sm9+k/dugVTff+Dc CmlwMUb2b/tiz27nHrjWwicl/Mb4zMz6z0BzCWOLaHxsrym1csZ8aqPwPVLZMDUAOYsJB9 B/EKZAVcKtn7K2yGRjN3cKRSkd1m7qU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-391-ZKvhtEYPMtKhEJeNqEk12A-1; Tue, 11 May 2021 07:55:31 -0400 X-MC-Unique: ZKvhtEYPMtKhEJeNqEk12A-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 756AC1854E27; Tue, 11 May 2021 11:55:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-233.ams2.redhat.com [10.36.112.233]) by smtp.corp.redhat.com (Postfix) with ESMTP id 46CEC60C04; Tue, 11 May 2021 11:55:26 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 13/13] OvmfPkg/BaseMemEncryptSevLib: remove Flush parameter To: devel@edk2.groups.io, brijesh.singh@amd.com Cc: James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Ard Biesheuvel , Erdem Aktas References: <20210507203838.23706-1-brijesh.singh@amd.com> <20210507203838.23706-14-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: <3c85698e-0b5e-38b0-f752-30204cb78cad@redhat.com> Date: Tue, 11 May 2021 13:55:25 +0200 MIME-Version: 1.0 In-Reply-To: <20210507203838.23706-14-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit I don't fully understand the updates in this patch: On 05/07/21 22:38, Brijesh Singh wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275 > > The Flush parameter is used to provide a hint whether the specified range > is Mmio address. Now that we have a dedicated helper to clear the > memory encryption mask for the Mmio address range, its safe to remove the > Flush parameter from MemEncryptSev{Set,Clear}PageEncMask(). This looks good; it matches my request (1) from: https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg00109.html > > Cc: James Bottomley > Cc: Min Xu > Cc: Jiewen Yao > Cc: Tom Lendacky > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Laszlo Ersek > Cc: Erdem Aktas > Signed-off-by: Brijesh Singh > --- > OvmfPkg/Include/Library/MemEncryptSevLib.h | 10 ++---- > .../BaseMemEncryptSevLib/X64/VirtualMemory.h | 10 ++---- > OvmfPkg/AmdSevDxe/AmdSevDxe.c | 3 +- > OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 6 ++-- > .../Ia32/MemEncryptSevLib.c | 10 ++---- > .../X64/MemEncryptSevLib.c | 16 +++------- > .../X64/PeiDxeVirtualMemory.c | 32 +++++++++++-------- > .../X64/SecVirtualMemory.c | 8 ++--- > .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +- > OvmfPkg/PlatformPei/AmdSev.c | 3 +- > 10 files changed, 35 insertions(+), 66 deletions(-) > > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > index b91490d5d44d..76d06c206c8b 100644 > --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h > +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > @@ -100,8 +100,6 @@ MemEncryptSevIsEnabled ( > address of a memory region. > @param[in] NumPages The number of pages from start memory > region. > - @param[in] Flush Flush the caches before clearing the bit > - (mostly TRUE except MMIO addresses) > > @retval RETURN_SUCCESS The attributes were cleared for the > memory region. > @@ -114,8 +112,7 @@ EFIAPI > MemEncryptSevClearPageEncMask ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS BaseAddress, > - IN UINTN NumPages, > - IN BOOLEAN Flush > + IN UINTN NumPages > ); > > /** > @@ -128,8 +125,6 @@ MemEncryptSevClearPageEncMask ( > address of a memory region. > @param[in] NumPages The number of pages from start memory > region. > - @param[in] Flush Flush the caches before setting the bit > - (mostly TRUE except MMIO addresses) > > @retval RETURN_SUCCESS The attributes were set for the memory > region. > @@ -142,8 +137,7 @@ EFIAPI > MemEncryptSevSetPageEncMask ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS BaseAddress, > - IN UINTN NumPages, > - IN BOOLEAN Flush > + IN UINTN NumPages > ); > > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > index 8dc39e647b90..21bbbd1c4f9c 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > @@ -58,8 +58,6 @@ InternalGetMemEncryptionAddressMask ( > @param[in] PhysicalAddress The physical address that is the start > address of a memory region. > @param[in] Length The length of memory region > - @param[in] Flush Flush the caches before applying the > - encryption mask > > @retval RETURN_SUCCESS The attributes were cleared for the > memory region. > @@ -72,8 +70,7 @@ EFIAPI > InternalMemEncryptSevSetMemoryDecrypted ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS PhysicalAddress, > - IN UINTN Length, > - IN BOOLEAN Flush > + IN UINTN Length > ); > > /** > @@ -85,8 +82,6 @@ InternalMemEncryptSevSetMemoryDecrypted ( > @param[in] PhysicalAddress The physical address that is the start > address of a memory region. > @param[in] Length The length of memory region > - @param[in] Flush Flush the caches before applying the > - encryption mask > > @retval RETURN_SUCCESS The attributes were set for the memory > region. > @@ -99,8 +94,7 @@ EFIAPI > InternalMemEncryptSevSetMemoryEncrypted ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS PhysicalAddress, > - IN UINTN Length, > - IN BOOLEAN Flush > + IN UINTN Length > ); > > /** > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > index 80831b81facf..41e4b291d070 100644 > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > @@ -120,8 +120,7 @@ AmdSevDxeEntryPoint ( > Status = MemEncryptSevClearPageEncMask ( > 0, // Cr3BaseAddress -- use current CR3 > MapPagesBase, // BaseAddress > - MapPagesCount, // NumPages > - TRUE // Flush > + MapPagesCount // NumPages > ); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevClearPageEncMask(): %r\n", (1) You missed my comment (2) in . > diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c > index 49ffa2448811..b30628078f73 100644 > --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c > +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c > @@ -252,8 +252,7 @@ IoMmuMap ( > Status = MemEncryptSevClearPageEncMask ( > 0, > MapInfo->PlainTextAddress, > - MapInfo->NumberOfPages, > - TRUE > + MapInfo->NumberOfPages > ); > ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > @@ -407,8 +406,7 @@ IoMmuUnmapWorker ( > Status = MemEncryptSevSetPageEncMask ( > 0, > MapInfo->PlainTextAddress, > - MapInfo->NumberOfPages, > - TRUE > + MapInfo->NumberOfPages > ); > ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > index 169d3118e44f..be260e0d1014 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > @@ -25,8 +25,6 @@ > address of a memory region. > @param[in] NumPages The number of pages from start memory > region. > - @param[in] Flush Flush the caches before clearing the bit > - (mostly TRUE except MMIO addresses) > > @retval RETURN_SUCCESS The attributes were cleared for the > memory region. > @@ -39,8 +37,7 @@ EFIAPI > MemEncryptSevClearPageEncMask ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS BaseAddress, > - IN UINTN NumPages, > - IN BOOLEAN Flush > + IN UINTN NumPages > ) > { > // > @@ -59,8 +56,6 @@ MemEncryptSevClearPageEncMask ( > address of a memory region. > @param[in] NumPages The number of pages from start memory > region. > - @param[in] Flush Flush the caches before setting the bit > - (mostly TRUE except MMIO addresses) > > @retval RETURN_SUCCESS The attributes were set for the memory > region. > @@ -73,8 +68,7 @@ EFIAPI > MemEncryptSevSetPageEncMask ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS BaseAddress, > - IN UINTN NumPages, > - IN BOOLEAN Flush > + IN UINTN NumPages > ) > { > // > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > index a2bf698bcde7..a57e8fd37fa7 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > @@ -27,8 +27,6 @@ > address of a memory region. > @param[in] NumPages The number of pages from start memory > region. > - @param[in] Flush Flush the caches before clearing the bit > - (mostly TRUE except MMIO addresses) > > @retval RETURN_SUCCESS The attributes were cleared for the > memory region. > @@ -41,15 +39,13 @@ EFIAPI > MemEncryptSevClearPageEncMask ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS BaseAddress, > - IN UINTN NumPages, > - IN BOOLEAN Flush > + IN UINTN NumPages > ) > { > return InternalMemEncryptSevSetMemoryDecrypted ( > Cr3BaseAddress, > BaseAddress, > - EFI_PAGES_TO_SIZE (NumPages), > - Flush > + EFI_PAGES_TO_SIZE (NumPages) > ); > } > > @@ -63,8 +59,6 @@ MemEncryptSevClearPageEncMask ( > address of a memory region. > @param[in] NumPages The number of pages from start memory > region. > - @param[in] Flush Flush the caches before setting the bit > - (mostly TRUE except MMIO addresses) > > @retval RETURN_SUCCESS The attributes were set for the memory > region. > @@ -77,15 +71,13 @@ EFIAPI > MemEncryptSevSetPageEncMask ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS BaseAddress, > - IN UINTN NumPages, > - IN BOOLEAN Flush > + IN UINTN NumPages > ) > { > return InternalMemEncryptSevSetMemoryEncrypted ( > Cr3BaseAddress, > BaseAddress, > - EFI_PAGES_TO_SIZE (NumPages), > - Flush > + EFI_PAGES_TO_SIZE (NumPages) > ); > } > > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > index a18d336a8789..ad1021bd3e43 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > @@ -555,8 +555,7 @@ EnableReadOnlyPageWriteProtect ( > address of a memory region. > @param[in] Length The length of memory region > @param[in] Mode Set or Clear mode > - @param[in] CacheFlush Flush the caches before applying the > - encryption mask > + @param[in] Mmio The physical address range is Mmio. > > @retval RETURN_SUCCESS The attributes were cleared for the > memory region. > @@ -572,7 +571,7 @@ SetMemoryEncDec ( > IN PHYSICAL_ADDRESS PhysicalAddress, > IN UINTN Length, > IN MAP_RANGE_MODE Mode, > - IN BOOLEAN CacheFlush > + IN BOOLEAN Mmio > ) > { > PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry; > @@ -585,12 +584,23 @@ SetMemoryEncDec ( > UINT64 AddressEncMask; > BOOLEAN IsWpEnabled; > RETURN_STATUS Status; > + BOOLEAN CacheFlush; > > // > // Set PageMapLevel4Entry to suppress incorrect compiler/analyzer warnings. > // > PageMapLevel4Entry = NULL; > > + // > + // The cache need to flushed for the non-Mmio address range. > + // > + if (Mmio == TRUE) { > + CacheFlush = FALSE; > + } else { > + CacheFlush = TRUE; > + } > + > + // > DEBUG (( > DEBUG_VERBOSE, > "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u\n", (2) The calculation of "CacheFlush" from "Mmio" is awkward. First, we don't compare BOOLEANs against TRUE or FALSE, BOOLEANs just stand alone in controlling expression (or otherwise "logical") context. Second, why not just write: CacheFlush = !Mmio; But even so... (3) ... The introduction of the "Mmio" parameter is inexplicable to me. It apparently replaces CacheFlush (with inverse meaning), but neither the commit message, nor the (RFCv2 -> PATCH) changelog, explain why this replacement makes sense. The SetMemoryEncDec() function is an internal function (not a library class API), so this change doesn't necessarily conflict with the commit message -- but having this change in this particular patch (the last patch in the series) seems unjustified. In the previous version, we updated two SetMemoryEncDec() call sites: in InternalMemEncryptSevSetMemoryDecrypted() and InternalMemEncryptSevSetMemoryEncrypted(), we replaced the forwarding of "Flush" with TRUE constants. In this version, we update *three* SetMemoryEncDec() call sites: - in InternalMemEncryptSevSetMemoryDecrypted() and InternalMemEncryptSevSetMemoryEncrypted(), we replace the forwarding of "Flush" with FALSE constants, - and in InternalMemEncryptSevClearMmioPageEncMask(), we replace the *constant* FALSE with TRUE. I think this very last point -- regarding InternalMemEncryptSevClearMmioPageEncMask() -- shows that the replacement of CacheFlush with Mmio, at this point in the series, is unwarranted. Minimally, this replacement / negation should be a separate patch, but even then, I think we'd need a defensible purpose (which is not clear to me at this point); *plus*, the "re-calculation" of CacheFlush inside SetMemoryEncDec() from Mmio feels like a cop-out. It's only being done to save some additional replacements in the patch, but it leaves us with a stricly worse -- harder to understand -- function. If you really need this replacement / negation, then please do it in a separate patch, with a good commit message; furthermore, please replace CacheFlush *completely*, in SetMemoryEncDec() -- please don't reintroduce it. Thanks, Laszlo > @@ -828,8 +838,6 @@ SetMemoryEncDec ( > @param[in] PhysicalAddress The physical address that is the start > address of a memory region. > @param[in] Length The length of memory region > - @param[in] Flush Flush the caches before applying the > - encryption mask > > @retval RETURN_SUCCESS The attributes were cleared for the > memory region. > @@ -842,8 +850,7 @@ EFIAPI > InternalMemEncryptSevSetMemoryDecrypted ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS PhysicalAddress, > - IN UINTN Length, > - IN BOOLEAN Flush > + IN UINTN Length > ) > { > > @@ -852,7 +859,7 @@ InternalMemEncryptSevSetMemoryDecrypted ( > PhysicalAddress, > Length, > ClearCBit, > - Flush > + FALSE > ); > } > > @@ -865,8 +872,6 @@ InternalMemEncryptSevSetMemoryDecrypted ( > @param[in] PhysicalAddress The physical address that is the start > address of a memory region. > @param[in] Length The length of memory region > - @param[in] Flush Flush the caches before applying the > - encryption mask > > @retval RETURN_SUCCESS The attributes were set for the memory > region. > @@ -879,8 +884,7 @@ EFIAPI > InternalMemEncryptSevSetMemoryEncrypted ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS PhysicalAddress, > - IN UINTN Length, > - IN BOOLEAN Flush > + IN UINTN Length > ) > { > return SetMemoryEncDec ( > @@ -888,7 +892,7 @@ InternalMemEncryptSevSetMemoryEncrypted ( > PhysicalAddress, > Length, > SetCBit, > - Flush > + FALSE > ); > } > > @@ -921,6 +925,6 @@ InternalMemEncryptSevClearMmioPageEncMask ( > PhysicalAddress, > Length, > ClearCBit, > - FALSE > + TRUE > ); > } > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c > index bca5e3febb1b..24d19d3ca161 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c > @@ -42,8 +42,6 @@ InternalGetMemEncryptionAddressMask ( > @param[in] PhysicalAddress The physical address that is the start > address of a memory region. > @param[in] Length The length of memory region > - @param[in] Flush Flush the caches before applying the > - encryption mask > > @retval RETURN_SUCCESS The attributes were cleared for the > memory region. > @@ -56,8 +54,7 @@ EFIAPI > InternalMemEncryptSevSetMemoryDecrypted ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS PhysicalAddress, > - IN UINTN Length, > - IN BOOLEAN Flush > + IN UINTN Length > ) > { > // > @@ -89,8 +86,7 @@ EFIAPI > InternalMemEncryptSevSetMemoryEncrypted ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS PhysicalAddress, > - IN UINTN Length, > - IN BOOLEAN Flush > + IN UINTN Length > ) > { > // > diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > index fdf2380974fa..c7cc5b0389c8 100644 > --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > @@ -283,8 +283,7 @@ SmmCpuFeaturesSmmRelocationComplete ( > Status = MemEncryptSevSetPageEncMask ( > 0, // Cr3BaseAddress -- use current CR3 > MapPagesBase, // BaseAddress > - MapPagesCount, // NumPages > - TRUE // Flush > + MapPagesCount // NumPages > ); > if (EFI_ERROR (Status)) { > DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevSetPageEncMask(): %r\n", > diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c > index dddffdebda4b..a8bf610022ba 100644 > --- a/OvmfPkg/PlatformPei/AmdSev.c > +++ b/OvmfPkg/PlatformPei/AmdSev.c > @@ -72,8 +72,7 @@ AmdSevEsInitialize ( > DecryptStatus = MemEncryptSevClearPageEncMask ( > 0, > GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount), > - 1, > - TRUE > + 1 > ); > ASSERT_RETURN_ERROR (DecryptStatus); > } >