From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web12.7798.1620299323650987198 for ; Thu, 06 May 2021 04:08:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Z9fvF7H5; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1620299322; 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=fmUwtUS3+WS/UQMyoZriT7aY1fVpkJCwF2wxcI4+nEk=; b=Z9fvF7H5ijlUAv2A5ZJUwt5zwWXLQ5YbkCMgXQvAhopeJIF93HhZsYIsH9MhY871KHpb34 rpFT/dkrjVFR3ZLvbxzMDlHc7Az75fwWZaWq7s1VcL0oxU0d1elX/qNjNg9vANEux6/51l /IM44VOeDpEEER/NsrLf3+z9PKGdhWE= 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-260-j7oxCniCM26RasQ0gS8Ahw-1; Thu, 06 May 2021 07:08:39 -0400 X-MC-Unique: j7oxCniCM26RasQ0gS8Ahw-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EA0DC18397A3; Thu, 6 May 2021 11:08:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-172.ams2.redhat.com [10.36.113.172]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6E2CE687E5; Thu, 6 May 2021 11:08:35 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v2 08/28] OvmfPkg/BaseMemEncryptSevLib: Remove CacheFlush 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: <20210430115148.22267-1-brijesh.singh@amd.com> <20210430115148.22267-9-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: <885f5d84-d36a-f4a9-8498-7340ad49dbd1@redhat.com> Date: Thu, 6 May 2021 13:08:34 +0200 MIME-Version: 1.0 In-Reply-To: <20210430115148.22267-9-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 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 On 04/30/21 13:51, Brijesh Singh wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275 > > The CacheFlush parameter is used to provide 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 > CacheFlush parameter from MemEncryptSev{Set,Clear}PageEncMask(). The subject and the commit message body refer to "CacheFlush", but the parameter (at the library class level) is actually called "Flush". (1) Please update the subject and the commit message body. > > 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/AmdSevDxe/AmdSevDxe.c | 3 +-- > OvmfPkg/Include/Library/MemEncryptSevLib.h | 10 ++-------- > OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 6 ++---- > OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 10 ++-------- > OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 16 ++++------------ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 14 ++++---------- > OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c | 8 ++------ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h | 10 ++-------- > OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 3 +-- > OvmfPkg/PlatformPei/AmdSev.c | 3 +-- > 10 files changed, 21 insertions(+), 62 deletions(-) Because we're modifying a library class header file, I agree we need to update multiple modules in a single patch. > > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c > index 80831b81fa..41e4b291d0 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", (2) By the removal of the comma, the comments are no longer nicely aligned. Please insert a space character. The rest looks OK to me. Thanks Laszlo > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > index c19f92afc6..9b15d80931 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/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c > index 49ffa24488..b30628078f 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 4e8a997d42..34e7c59e2c 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 6786573aea..5c260c546e 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 3bcc92f2e9..707db5a74a 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > @@ -830,8 +830,6 @@ Done: > @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. > @@ -844,8 +842,7 @@ EFIAPI > InternalMemEncryptSevSetMemoryDecrypted ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS PhysicalAddress, > - IN UINTN Length, > - IN BOOLEAN Flush > + IN UINTN Length > ) > { > > @@ -854,7 +851,7 @@ InternalMemEncryptSevSetMemoryDecrypted ( > PhysicalAddress, > Length, > ClearCBit, > - Flush, > + TRUE, > FALSE > ); > } > @@ -868,8 +865,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. > @@ -882,8 +877,7 @@ EFIAPI > InternalMemEncryptSevSetMemoryEncrypted ( > IN PHYSICAL_ADDRESS Cr3BaseAddress, > IN PHYSICAL_ADDRESS PhysicalAddress, > - IN UINTN Length, > - IN BOOLEAN Flush > + IN UINTN Length > ) > { > return SetMemoryEncDec ( > @@ -891,7 +885,7 @@ InternalMemEncryptSevSetMemoryEncrypted ( > PhysicalAddress, > Length, > SetCBit, > - Flush, > + TRUE, > FALSE > ); > } > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c > index bca5e3febb..24d19d3ca1 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/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > index 99ee7ea0e8..832ff10a33 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/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c > index fdf2380974..c7cc5b0389 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 dddffdebda..a8bf610022 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); > } >