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 <j...@linux.ibm.com>
> Cc: Min Xu <min.m...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Erdem Aktas <erdemak...@google.com>
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
> 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74790): https://edk2.groups.io/g/devel/message/74790
Mute This Topic: https://groups.io/mt/82479053/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
