On 5/6/21 5:39 AM, Laszlo Ersek via groups.io wrote:
> On 04/30/21 13:51, Brijesh Singh wrote:
>> BZ:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C2719bcebf85d4e5228e508d9107b449d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637558943897655383%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=YrgkAA3Kp6v2eJuovVho1I3kR%2FXAA5D%2BE2vaUKyFO68%3D&reserved=0
>>
>> 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.)
>
>
Thanks Laszlo, I will go through the feedback on this patch and address
them in next rev.
-Brijesh
>> 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 (#74802): https://edk2.groups.io/g/devel/message/74802
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]
-=-=-=-=-=-=-=-=-=-=-=-
