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.web08.7576.1620297585106960490 for ; Thu, 06 May 2021 03:39:45 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=QEnv+Mpi; 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=1620297584; 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=OiIkKng+oEw02FG5oQVWC8gtZMxuxPCWUOTUtkOjrP0=; b=QEnv+Mpiit8Euqyal0fWpI6Dpv45y62geZMoz3MHsqe/UQZk4rtrtxk+pnQuKaiQ2xUj/w +wjem5s7uLizL/HiuecNnFkBiYXE1L218CGmZ7IayAu35tW2BIAYsUZvpE3kkb2enwmmDU 4bLQBl+VmVBal9lU5QphPL/F+ktQn5I= 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-122-mQvcC4wDMSyKqsybyt2FGA-1; Thu, 06 May 2021 06:39:40 -0400 X-MC-Unique: mQvcC4wDMSyKqsybyt2FGA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7A98F8015A8; Thu, 6 May 2021 10:39:38 +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 DBFBE19D7C; Thu, 6 May 2021 10:39:35 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH RFC v2 06/28] OvmfPkg/BaseMemEncryptSevLib: Introduce MemEncryptSevClearMmioPageEncMask() 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-7-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 6 May 2021 12:39:34 +0200 MIME-Version: 1.0 In-Reply-To: <20210430115148.22267-7-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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 MemEncryptSevClearMmioPageEncMask() helper can be used for clearing > the memory encryption mask for the Mmio region from the current page > table context. The commit message, and five comments in the patch, say "current page table context". However, Cr3BaseAddress is taken explicitly. I realize the files being modified in this patch already make similarly incorrect statements, but I'd like if we avoided adding more. (1) Please just drop "current page table context" from the mentioned six locations. (Explaining that Cr3BaseAddress=0 means "current CR3" is of course valid.) > > 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 | 25 +++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c | 31 ++++++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c | 33 +++++++++++++++ > OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 44 ++++++++++++++++++-- > OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h | 23 ++++++++++ > 5 files changed, 153 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h > index 99f15a7d12..c19f92afc6 100644 > --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h > +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h > @@ -203,4 +203,29 @@ MemEncryptSevGetAddressRangeState ( > IN UINTN Length > ); > > +/** > + This function clears memory encryption bit for the mmio region specified by > + BaseAddress and NumPages from the current page table context. > + > + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use > + current CR3) > + @param[in] BaseAddress The physical address that is the start > + address of a mmio region. > + @param[in] NumPages The number of pages from start memory > + region. > + > + @retval RETURN_SUCCESS The attributes were cleared for the > + memory region. > + @retval RETURN_INVALID_PARAMETER Number of pages is zero. > + @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute > + is not supported > +**/ > +RETURN_STATUS > +EFIAPI > +MemEncryptSevClearMmioPageEncMask ( > + IN PHYSICAL_ADDRESS Cr3BaseAddress, > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINTN NumPages > + ); > + > #endif // _MEM_ENCRYPT_SEV_LIB_H_ > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > index 12a5bf495b..4e8a997d42 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c > @@ -111,3 +111,34 @@ MemEncryptSevGetAddressRangeState ( > // > return MemEncryptSevAddressRangeEncrypted; > } > + > +/** > + This function clears memory encryption bit for the mmio region specified by > + BaseAddress and NumPages from the current page table context. > + > + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use > + current CR3) > + @param[in] BaseAddress The physical address that is the start > + address of a mmio region. > + @param[in] NumPages The number of pages from start memory > + region. > + > + @retval RETURN_SUCCESS The attributes were cleared for the > + memory region. > + @retval RETURN_INVALID_PARAMETER Number of pages is zero. > + @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute > + is not supported > +**/ > +RETURN_STATUS > +EFIAPI > +MemEncryptSevClearMmioPageEncMask ( > + IN PHYSICAL_ADDRESS Cr3BaseAddress, > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINTN NumPages > + ) > +{ > + // > + // Memory encryption bit is not accessible in 32-bit mode > + // > + return RETURN_UNSUPPORTED; > +} > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > index 4fea6a6be0..6786573aea 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c > @@ -118,3 +118,36 @@ MemEncryptSevGetAddressRangeState ( > Length > ); > } > + > +/** > + This function clears memory encryption bit for the mmio region specified by > + BaseAddress and NumPages from the current page table context. > + > + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use > + current CR3) > + @param[in] BaseAddress The physical address that is the start > + address of a mmio region. > + @param[in] NumPages The number of pages from start memory > + region. > + > + @retval RETURN_SUCCESS The attributes were cleared for the > + memory region. > + @retval RETURN_INVALID_PARAMETER Number of pages is zero. > + @retval RETURN_UNSUPPORTED Clearing the memory encryption attribute > + is not supported > +**/ > +RETURN_STATUS > +EFIAPI > +MemEncryptSevClearMmioPageEncMask ( > + IN PHYSICAL_ADDRESS Cr3BaseAddress, > + IN PHYSICAL_ADDRESS BaseAddress, > + IN UINTN NumPages > + ) > +{ > + return InternalMemEncryptSevClearMmioPageEncMask ( > + Cr3BaseAddress, > + BaseAddress, > + EFI_PAGES_TO_SIZE (NumPages) > + ); > + > +} > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > index d3455e812b..3bcc92f2e9 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c > @@ -557,6 +557,7 @@ EnableReadOnlyPageWriteProtect ( > @param[in] Mode Set or Clear mode > @param[in] CacheFlush Flush the caches before applying the > encryption mask > + @param[in] IsMmio The address is Mmio address. > > @retval RETURN_SUCCESS The attributes were cleared for the > memory region. (2) The parameter's name in the documentation ("IsMmio") does not match the actual parameter name ("Mmio"). > @@ -572,7 +573,8 @@ SetMemoryEncDec ( > IN PHYSICAL_ADDRESS PhysicalAddress, > IN UINTN Length, > IN MAP_RANGE_MODE Mode, > - IN BOOLEAN CacheFlush > + IN BOOLEAN CacheFlush, > + IN BOOLEAN Mmio > ) > { > PAGE_MAP_AND_DIRECTORY_POINTER *PageMapLevel4Entry; The "Mmio" parameter is not used for anything in this patch. It's very confusing. (3) Please remove the addition of the Mmio parameter from this patch. Please introduce the Mmio parameter only when it is utilized -- as far as I can tell from a quick git-blame, that's patch #26 ("OvmfPkg/MemEncryptSevLib: Change the page state in the RMP table"). As a result, in this patch, InternalMemEncryptSevClearMmioPageEncMask() will effectively become a slight simplification of InternalMemEncryptSevSetMemoryDecrypted() -- rather than forwarding "Flush" for "CacheFlush", it will pass constant FALSE for "CacheFlush". (4) Please mention the above fact (= last paragraph above) in the commit message. > @@ -852,7 +854,8 @@ InternalMemEncryptSevSetMemoryDecrypted ( > PhysicalAddress, > Length, > ClearCBit, > - Flush > + Flush, > + FALSE > ); > } > > @@ -888,6 +891,41 @@ InternalMemEncryptSevSetMemoryEncrypted ( > PhysicalAddress, > Length, > SetCBit, > - Flush > + Flush, > + FALSE > + ); > +} > + > +/** > + This function clears memory encryption bit for the Mmio region specified by > + PhysicalAddress and Length from the current page table context. > + > + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use > + current CR3) > + @param[in] PhysicalAddress The physical address that is the start > + address of a mmio region. > + @param[in] Length The length of memory region > + > + @retval RETURN_SUCCESS The attributes were cleared for the > + memory region. > + @retval RETURN_INVALID_PARAMETER Number of pages is zero. This function does not take a NumPages parameter but a Length parameter. (5) Please update the RETURN_INVALID_PARAMETER comment accordingly -- in the header file as well. > + @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute > + is not supported > +**/ > +RETURN_STATUS > +EFIAPI > +InternalMemEncryptSevClearMmioPageEncMask ( > + IN PHYSICAL_ADDRESS Cr3BaseAddress, > + IN PHYSICAL_ADDRESS PhysicalAddress, > + IN UINTN Length > + ) > +{ > + return SetMemoryEncDec ( > + Cr3BaseAddress, > + PhysicalAddress, > + Length, > + ClearCBit, > + FALSE, > + TRUE > ); > } > diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > index fe2a0b2826..99ee7ea0e8 100644 > --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h > @@ -126,4 +126,27 @@ InternalMemEncryptSevGetAddressRangeState ( > IN UINTN Length > ); > > +/** > + This function clears memory encryption bit for the Mmio region specified by > + PhysicalAddress and Length from the current page table context. > + > + @param[in] Cr3BaseAddress Cr3 Base Address (if zero then use > + current CR3) > + @param[in] PhysicalAddress The physical address that is the start > + address of a mmio region. > + @param[in] Length The length of memory region > + > + @retval RETURN_SUCCESS The attributes were cleared for the > + memory region. > + @retval RETURN_INVALID_PARAMETER Number of pages is zero. This function does not take a NumPages parameter but a Length parameter. (6) Please update the RETURN_INVALID_PARAMETER comment accordingly -- same as in the C file. > + @retval RETURN_UNSUPPORTED Clearing the memory encyrption attribute > + is not supported > +**/ > +RETURN_STATUS > +EFIAPI > +InternalMemEncryptSevClearMmioPageEncMask ( > + IN PHYSICAL_ADDRESS Cr3BaseAddress, > + IN PHYSICAL_ADDRESS PhysicalAddress, > + IN UINTN Length > + ); > #endif > Please carefully audit the rest of the series for comment blocks. Such issues render reviews inefficient. Thanks Laszlo