Re: [edk2-devel] [PATCH RFC v2 06/28] OvmfPkg/BaseMemEncryptSevLib: Introduce MemEncryptSevClearMmioPageEncMask()

["Laszlo Ersek" <lersek@redhat.com>]

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 <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@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