[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [edk2-devel] [PATCH RFC v2 08/28] OvmfPkg/BaseMemEncryptSevLib: Remove CacheFlush parameter



On 04/30/21 13:51, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> The CacheFlush parameter is used to provide hint whether the specified
> range is Mmio address. Now that we have a dedicated helper to clear the
> memory encryption mask for the Mmio address range, its safe to remove the
> CacheFlush parameter from MemEncryptSev{Set,Clear}PageEncMask().

The subject and the commit message body refer to "CacheFlush", but the
parameter (at the library class level) is actually called "Flush".

(1) Please update the subject and the commit message body.


> 
> 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/AmdSevDxe/AmdSevDxe.c                                  |  3 +--
>  OvmfPkg/Include/Library/MemEncryptSevLib.h                     | 10 ++--------
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                                 |  6 ++----
>  OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c   | 10 ++--------
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c    | 16 ++++------------
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c | 14 ++++----------
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c    |  8 ++------
>  OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h       | 10 ++--------
>  OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c          |  3 +--
>  OvmfPkg/PlatformPei/AmdSev.c                                   |  3 +--
>  10 files changed, 21 insertions(+), 62 deletions(-)

Because we're modifying a library class header file, I agree we need to
update multiple modules in a single patch.


> 
> diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> index 80831b81fa..41e4b291d0 100644
> --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> @@ -120,8 +120,7 @@ AmdSevDxeEntryPoint (
>      Status = MemEncryptSevClearPageEncMask (
>                 0,             // Cr3BaseAddress -- use current CR3
>                 MapPagesBase,  // BaseAddress
> -               MapPagesCount, // NumPages
> -               TRUE           // Flush
> +               MapPagesCount // NumPages
>                 );
>      if (EFI_ERROR (Status)) {
>        DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevClearPageEncMask(): %r\n",

(2) By the removal of the comma, the comments are no longer nicely
aligned. Please insert a space character.

The rest looks OK to me.

Thanks
Laszlo

> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index c19f92afc6..9b15d80931 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -100,8 +100,6 @@ MemEncryptSevIsEnabled (
>                                        address of a memory region.
>    @param[in]  NumPages                The number of pages from start memory
>                                        region.
> -  @param[in]  Flush                   Flush the caches before clearing the bit
> -                                      (mostly TRUE except MMIO addresses)
>  
>    @retval RETURN_SUCCESS              The attributes were cleared for the
>                                        memory region.
> @@ -114,8 +112,7 @@ EFIAPI
>  MemEncryptSevClearPageEncMask (
>    IN PHYSICAL_ADDRESS         Cr3BaseAddress,
>    IN PHYSICAL_ADDRESS         BaseAddress,
> -  IN UINTN                    NumPages,
> -  IN BOOLEAN                  Flush
> +  IN UINTN                    NumPages
>    );
>  
>  /**
> @@ -128,8 +125,6 @@ MemEncryptSevClearPageEncMask (
>                                        address of a memory region.
>    @param[in]  NumPages                The number of pages from start memory
>                                        region.
> -  @param[in]  Flush                   Flush the caches before setting the bit
> -                                      (mostly TRUE except MMIO addresses)
>  
>    @retval RETURN_SUCCESS              The attributes were set for the memory
>                                        region.
> @@ -142,8 +137,7 @@ EFIAPI
>  MemEncryptSevSetPageEncMask (
>    IN PHYSICAL_ADDRESS         Cr3BaseAddress,
>    IN PHYSICAL_ADDRESS         BaseAddress,
> -  IN UINTN                    NumPages,
> -  IN BOOLEAN                  Flush
> +  IN UINTN                    NumPages
>    );
>  
>  
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index 49ffa24488..b30628078f 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -252,8 +252,7 @@ IoMmuMap (
>    Status = MemEncryptSevClearPageEncMask (
>               0,
>               MapInfo->PlainTextAddress,
> -             MapInfo->NumberOfPages,
> -             TRUE
> +             MapInfo->NumberOfPages
>               );
>    ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> @@ -407,8 +406,7 @@ IoMmuUnmapWorker (
>    Status = MemEncryptSevSetPageEncMask (
>               0,
>               MapInfo->PlainTextAddress,
> -             MapInfo->NumberOfPages,
> -             TRUE
> +             MapInfo->NumberOfPages
>               );
>    ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> index 4e8a997d42..34e7c59e2c 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c
> @@ -25,8 +25,6 @@
>                                        address of a memory region.
>    @param[in]  NumPages                The number of pages from start memory
>                                        region.
> -  @param[in]  Flush                   Flush the caches before clearing the bit
> -                                      (mostly TRUE except MMIO addresses)
>  
>    @retval RETURN_SUCCESS              The attributes were cleared for the
>                                        memory region.
> @@ -39,8 +37,7 @@ EFIAPI
>  MemEncryptSevClearPageEncMask (
>    IN PHYSICAL_ADDRESS         Cr3BaseAddress,
>    IN PHYSICAL_ADDRESS         BaseAddress,
> -  IN UINTN                    NumPages,
> -  IN BOOLEAN                  Flush
> +  IN UINTN                    NumPages
>    )
>  {
>    //
> @@ -59,8 +56,6 @@ MemEncryptSevClearPageEncMask (
>                                        address of a memory region.
>    @param[in]  NumPages                The number of pages from start memory
>                                        region.
> -  @param[in]  Flush                   Flush the caches before setting the bit
> -                                      (mostly TRUE except MMIO addresses)
>  
>    @retval RETURN_SUCCESS              The attributes were set for the memory
>                                        region.
> @@ -73,8 +68,7 @@ EFIAPI
>  MemEncryptSevSetPageEncMask (
>    IN PHYSICAL_ADDRESS         Cr3BaseAddress,
>    IN PHYSICAL_ADDRESS         BaseAddress,
> -  IN UINTN                    NumPages,
> -  IN BOOLEAN                  Flush
> +  IN UINTN                    NumPages
>    )
>  {
>    //
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> index 6786573aea..5c260c546e 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c
> @@ -27,8 +27,6 @@
>                                        address of a memory region.
>    @param[in]  NumPages                The number of pages from start memory
>                                        region.
> -  @param[in]  Flush                   Flush the caches before clearing the bit
> -                                      (mostly TRUE except MMIO addresses)
>  
>    @retval RETURN_SUCCESS              The attributes were cleared for the
>                                        memory region.
> @@ -41,15 +39,13 @@ EFIAPI
>  MemEncryptSevClearPageEncMask (
>    IN PHYSICAL_ADDRESS         Cr3BaseAddress,
>    IN PHYSICAL_ADDRESS         BaseAddress,
> -  IN UINTN                    NumPages,
> -  IN BOOLEAN                  Flush
> +  IN UINTN                    NumPages
>    )
>  {
>    return InternalMemEncryptSevSetMemoryDecrypted (
>             Cr3BaseAddress,
>             BaseAddress,
> -           EFI_PAGES_TO_SIZE (NumPages),
> -           Flush
> +           EFI_PAGES_TO_SIZE (NumPages)
>             );
>  }
>  
> @@ -63,8 +59,6 @@ MemEncryptSevClearPageEncMask (
>                                        address of a memory region.
>    @param[in]  NumPages                The number of pages from start memory
>                                        region.
> -  @param[in]  Flush                   Flush the caches before setting the bit
> -                                      (mostly TRUE except MMIO addresses)
>  
>    @retval RETURN_SUCCESS              The attributes were set for the memory
>                                        region.
> @@ -77,15 +71,13 @@ EFIAPI
>  MemEncryptSevSetPageEncMask (
>    IN PHYSICAL_ADDRESS         Cr3BaseAddress,
>    IN PHYSICAL_ADDRESS         BaseAddress,
> -  IN UINTN                    NumPages,
> -  IN BOOLEAN                  Flush
> +  IN UINTN                    NumPages
>    )
>  {
>    return InternalMemEncryptSevSetMemoryEncrypted (
>             Cr3BaseAddress,
>             BaseAddress,
> -           EFI_PAGES_TO_SIZE (NumPages),
> -           Flush
> +           EFI_PAGES_TO_SIZE (NumPages)
>             );
>  }
>  
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> index 3bcc92f2e9..707db5a74a 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/PeiDxeVirtualMemory.c
> @@ -830,8 +830,6 @@ Done:
>    @param[in]  PhysicalAddress         The physical address that is the start
>                                        address of a memory region.
>    @param[in]  Length                  The length of memory region
> -  @param[in]  Flush                   Flush the caches before applying the
> -                                      encryption mask
>  
>    @retval RETURN_SUCCESS              The attributes were cleared for the
>                                        memory region.
> @@ -844,8 +842,7 @@ EFIAPI
>  InternalMemEncryptSevSetMemoryDecrypted (
>    IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
>    IN  PHYSICAL_ADDRESS        PhysicalAddress,
> -  IN  UINTN                   Length,
> -  IN  BOOLEAN                 Flush
> +  IN  UINTN                   Length
>    )
>  {
>  
> @@ -854,7 +851,7 @@ InternalMemEncryptSevSetMemoryDecrypted (
>             PhysicalAddress,
>             Length,
>             ClearCBit,
> -           Flush,
> +           TRUE,
>             FALSE
>             );
>  }
> @@ -868,8 +865,6 @@ InternalMemEncryptSevSetMemoryDecrypted (
>    @param[in]  PhysicalAddress         The physical address that is the start
>                                        address of a memory region.
>    @param[in]  Length                  The length of memory region
> -  @param[in]  Flush                   Flush the caches before applying the
> -                                      encryption mask
>  
>    @retval RETURN_SUCCESS              The attributes were set for the memory
>                                        region.
> @@ -882,8 +877,7 @@ EFIAPI
>  InternalMemEncryptSevSetMemoryEncrypted (
>    IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
>    IN  PHYSICAL_ADDRESS        PhysicalAddress,
> -  IN  UINTN                   Length,
> -  IN  BOOLEAN                 Flush
> +  IN  UINTN                   Length
>    )
>  {
>    return SetMemoryEncDec (
> @@ -891,7 +885,7 @@ InternalMemEncryptSevSetMemoryEncrypted (
>             PhysicalAddress,
>             Length,
>             SetCBit,
> -           Flush,
> +           TRUE,
>             FALSE
>             );
>  }
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> index bca5e3febb..24d19d3ca1 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/SecVirtualMemory.c
> @@ -42,8 +42,6 @@ InternalGetMemEncryptionAddressMask (
>    @param[in]  PhysicalAddress         The physical address that is the start
>                                        address of a memory region.
>    @param[in]  Length                  The length of memory region
> -  @param[in]  Flush                   Flush the caches before applying the
> -                                      encryption mask
>  
>    @retval RETURN_SUCCESS              The attributes were cleared for the
>                                        memory region.
> @@ -56,8 +54,7 @@ EFIAPI
>  InternalMemEncryptSevSetMemoryDecrypted (
>    IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
>    IN  PHYSICAL_ADDRESS        PhysicalAddress,
> -  IN  UINTN                   Length,
> -  IN  BOOLEAN                 Flush
> +  IN  UINTN                   Length
>    )
>  {
>    //
> @@ -89,8 +86,7 @@ EFIAPI
>  InternalMemEncryptSevSetMemoryEncrypted (
>    IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
>    IN  PHYSICAL_ADDRESS        PhysicalAddress,
> -  IN  UINTN                   Length,
> -  IN  BOOLEAN                 Flush
> +  IN  UINTN                   Length
>    )
>  {
>    //
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> index 99ee7ea0e8..832ff10a33 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h
> @@ -58,8 +58,6 @@ InternalGetMemEncryptionAddressMask (
>    @param[in]  PhysicalAddress         The physical address that is the start
>                                        address of a memory region.
>    @param[in]  Length                  The length of memory region
> -  @param[in]  Flush                   Flush the caches before applying the
> -                                      encryption mask
>  
>    @retval RETURN_SUCCESS              The attributes were cleared for the
>                                        memory region.
> @@ -72,8 +70,7 @@ EFIAPI
>  InternalMemEncryptSevSetMemoryDecrypted (
>    IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
>    IN  PHYSICAL_ADDRESS        PhysicalAddress,
> -  IN  UINTN                   Length,
> -  IN  BOOLEAN                 Flush
> +  IN  UINTN                   Length
>    );
>  
>  /**
> @@ -85,8 +82,6 @@ InternalMemEncryptSevSetMemoryDecrypted (
>    @param[in]  PhysicalAddress         The physical address that is the start
>                                        address of a memory region.
>    @param[in]  Length                  The length of memory region
> -  @param[in]  Flush                   Flush the caches before applying the
> -                                      encryption mask
>  
>    @retval RETURN_SUCCESS              The attributes were set for the memory
>                                        region.
> @@ -99,8 +94,7 @@ EFIAPI
>  InternalMemEncryptSevSetMemoryEncrypted (
>    IN  PHYSICAL_ADDRESS        Cr3BaseAddress,
>    IN  PHYSICAL_ADDRESS        PhysicalAddress,
> -  IN  UINTN                   Length,
> -  IN  BOOLEAN                 Flush
> +  IN  UINTN                   Length
>    );
>  
>  /**
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index fdf2380974..c7cc5b0389 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -283,8 +283,7 @@ SmmCpuFeaturesSmmRelocationComplete (
>    Status = MemEncryptSevSetPageEncMask (
>               0,             // Cr3BaseAddress -- use current CR3
>               MapPagesBase,  // BaseAddress
> -             MapPagesCount, // NumPages
> -             TRUE           // Flush
> +             MapPagesCount  // NumPages
>               );
>    if (EFI_ERROR (Status)) {
>      DEBUG ((DEBUG_ERROR, "%a: MemEncryptSevSetPageEncMask(): %r\n",
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index dddffdebda..a8bf610022 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -72,8 +72,7 @@ AmdSevEsInitialize (
>      DecryptStatus = MemEncryptSevClearPageEncMask (
>        0,
>        GhcbBasePa + EFI_PAGES_TO_SIZE (PageCount),
> -      1,
> -      TRUE
> +      1
>        );
>      ASSERT_RETURN_ERROR (DecryptStatus);
>    }
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74794): https://edk2.groups.io/g/devel/message/74794
Mute This Topic: https://groups.io/mt/82479055/1813853
Group Owner: devel+owner edk2 groups io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive redhat com]
-=-=-=-=-=-=-=-=-=-=-=-



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]