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

["Brijesh Singh" <brijesh.singh@amd.com>]

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 <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
>
>
>
> 
>
>