Re: [PATCH V2 2/3] MdeModulePkg: Add New Memory Attributes

["Dandan Bi" <dandan.bi@intel.com>]

Reviewed-by: Dandan Bi <dandan.bi@intel.com>


Thanks,
Dandan
> -----Original Message-----
> From: Oleksiy Yakovlev <oleksiyy@ami.com>
> Sent: Wednesday, June 24, 2020 5:56 AM
> To: devel@edk2.groups.io
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Ni, Ray
> <ray.ni@intel.com>; lersek@redhat.com; Kumar, Rahul1
> <rahul1.kumar@intel.com>; Felixp@ami.com; oleksiyy@ami.com
> Subject: [PATCH V2 2/3] MdeModulePkg: Add New Memory Attributes
> 
> Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO attributes
> introduced in UEFI 2.8.
> (UEFI 2.8, mantis 1919 and 1872).
> Use attributes bitmasks, defined in MdePkg.
> 
> Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
> ---
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c               | 11 ++---------
>  MdeModulePkg/Core/Dxe/Mem/Page.c              |  9 +++------
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c |  7 ++-----
>  MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c        | 10 ++--------
>  4 files changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index 74f3b1b..2d8c076 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -35,13 +35,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  #define PRESENT_MEMORY_ATTRIBUTES
> (EFI_RESOURCE_ATTRIBUTE_PRESENT)
> 
> -#define EXCLUSIVE_MEMORY_ATTRIBUTES   (EFI_MEMORY_UC |
> EFI_MEMORY_WC | \
> -                                       EFI_MEMORY_WT | EFI_MEMORY_WB | \
> -                                       EFI_MEMORY_WP | EFI_MEMORY_UCE)
> -
> -#define NONEXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_XP |
> EFI_MEMORY_RP | \
> -                                        EFI_MEMORY_RO)
> -
>  //
>  // Module Variables
>  //
> @@ -665,7 +658,7 @@ ConverToCpuArchAttributes (  {
>    UINT64      CpuArchAttributes;
> 
> -  CpuArchAttributes = Attributes & NONEXCLUSIVE_MEMORY_ATTRIBUTES;
> +  CpuArchAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK;
> 
>    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
>      CpuArchAttributes |= EFI_MEMORY_UC; @@ -951,7 +944,7 @@
> CoreConvertSpace (
>          // Keep original CPU arch attributes when caller just calls
>          // SetMemorySpaceAttributes() with none CPU arch attributes (for
> example, RUNTIME).
>          //
> -        Attributes |= (Entry->Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> NONEXCLUSIVE_MEMORY_ATTRIBUTES));
> +        Attributes |= (Entry->Attributes & (EFI_CACHE_ATTRIBUTE_MASK |
> + EFI_MEMORY_ATTRIBUTE_MASK));
>        }
>        Entry->Attributes = Attributes;
>        break;
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index 1f0e3d9..2c2c9cd 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -1857,8 +1857,7 @@ CoreGetMemoryMap (
>        MemoryMap->VirtualStart  = 0;
>        MemoryMap->NumberOfPages = RShiftU64
> ((MergeGcdMapEntry.EndAddress - MergeGcdMapEntry.BaseAddress + 1),
> EFI_PAGE_SHIFT);
>        MemoryMap->Attribute     = (MergeGcdMapEntry.Attributes &
> ~EFI_MEMORY_PORT_IO) |
> -                                (MergeGcdMapEntry.Capabilities & (EFI_MEMORY_RP |
> EFI_MEMORY_WP | EFI_MEMORY_XP | EFI_MEMORY_RO |
> -                                EFI_MEMORY_UC | EFI_MEMORY_UCE | EFI_MEMORY_WC
> | EFI_MEMORY_WT | EFI_MEMORY_WB));
> +                                (MergeGcdMapEntry.Capabilities &
> + (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
> 
>        if (MergeGcdMapEntry.GcdMemoryType ==
> EfiGcdMemoryTypeReserved) {
>          MemoryMap->Type = EfiReservedMemoryType; @@ -1892,8 +1891,7
> @@ CoreGetMemoryMap (
>        MemoryMap->VirtualStart  = 0;
>        MemoryMap->NumberOfPages = RShiftU64
> ((MergeGcdMapEntry.EndAddress - MergeGcdMapEntry.BaseAddress + 1),
> EFI_PAGE_SHIFT);
>        MemoryMap->Attribute     = MergeGcdMapEntry.Attributes |
> EFI_MEMORY_NV |
> -                                (MergeGcdMapEntry.Capabilities & (EFI_MEMORY_RP |
> EFI_MEMORY_WP | EFI_MEMORY_XP | EFI_MEMORY_RO |
> -                                EFI_MEMORY_UC | EFI_MEMORY_UCE | EFI_MEMORY_WC
> | EFI_MEMORY_WT | EFI_MEMORY_WB));
> +                                (MergeGcdMapEntry.Capabilities &
> + (EFI_CACHE_ATTRIBUTE_MASK | EFI_MEMORY_ATTRIBUTE_MASK));
>        MemoryMap->Type          = EfiPersistentMemory;
> 
>        //
> @@ -1935,8 +1933,7 @@ CoreGetMemoryMap (
>    MemoryMapEnd = MemoryMap;
>    MemoryMap = MemoryMapStart;
>    while (MemoryMap < MemoryMapEnd) {
> -    MemoryMap->Attribute &= ~(UINT64)(EFI_MEMORY_RP |
> EFI_MEMORY_RO |
> -                                      EFI_MEMORY_XP);
> +    MemoryMap->Attribute &= ~(UINT64)EFI_MEMORY_ATTRIBUTE_MASK;
>      MemoryMap = NEXT_MEMORY_DESCRIPTOR (MemoryMap, Size);
>    }
>    MergeMemoryMap (MemoryMapStart, &BufferSize, Size); diff --git
> a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 92a442f..7d1daf0 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -42,9 +42,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  #include
> "DxeMain.h"
>  #include "Mem/HeapGuard.h"
> 
> -#define CACHE_ATTRIBUTE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC
> | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE |
> EFI_MEMORY_WP)
> -#define MEMORY_ATTRIBUTE_MASK  (EFI_MEMORY_RP |
> EFI_MEMORY_XP | EFI_MEMORY_RO)
> -
>  //
>  // Image type definitions
>  //
> @@ -221,7 +218,7 @@ SetUefiImageMemoryAttributes (
>    Status = CoreGetMemorySpaceDescriptor(BaseAddress, &Descriptor);
>    ASSERT_EFI_ERROR(Status);
> 
> -  FinalAttributes = (Descriptor.Attributes & CACHE_ATTRIBUTE_MASK) |
> (Attributes & MEMORY_ATTRIBUTE_MASK);
> +  FinalAttributes = (Descriptor.Attributes & EFI_CACHE_ATTRIBUTE_MASK)
> + | (Attributes & EFI_MEMORY_ATTRIBUTE_MASK);
> 
>    DEBUG ((DEBUG_INFO, "SetUefiImageMemoryAttributes - 0x%016lx -
> 0x%016lx (0x%016lx)\n", BaseAddress, Length, FinalAttributes));
> 
> @@ -924,7 +921,7 @@ InitializeDxeNxMemoryProtectionPolicy (
>              (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)) {
> 
>          Attributes = GetPermissionAttributeForMemoryType
> (EfiConventionalMemory) |
> -                     (Entry->Attributes & CACHE_ATTRIBUTE_MASK);
> +                     (Entry->Attributes & EFI_CACHE_ATTRIBUTE_MASK);
> 
>          DEBUG ((DEBUG_INFO,
>            "Untested GCD memory space region: - 0x%016lx - 0x%016lx
> (0x%016lx)\n", diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> index 0385f1d..599a0cd 100644
> --- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
> @@ -39,12 +39,6 @@
> 
>  #define SMRAM_CAPABILITIES  (EFI_MEMORY_WB | EFI_MEMORY_UC)
> 
> -#define MEMORY_CACHE_ATTRIBUTES (EFI_MEMORY_UC |
> EFI_MEMORY_WC | \
> -                                 EFI_MEMORY_WT | EFI_MEMORY_WB | \
> -                                 EFI_MEMORY_WP | EFI_MEMORY_UCE)
> -
> -#define MEMORY_PAGE_ATTRIBUTES  (EFI_MEMORY_XP |
> EFI_MEMORY_RP | EFI_MEMORY_RO)
> -
>  //
>  // Function prototypes from produced protocols  // @@ -1710,7 +1704,7 @@
> SmmIplEntry (
>      CpuArch = NULL;
>      Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID
> **)&CpuArch);
>      if (!EFI_ERROR (Status)) {
> -      MemDesc.Attributes &= ~(MEMORY_CACHE_ATTRIBUTES |
> MEMORY_PAGE_ATTRIBUTES);
> +      MemDesc.Attributes &= ~(EFI_CACHE_ATTRIBUTE_MASK |
> + EFI_MEMORY_ATTRIBUTE_MASK);
>        MemDesc.Attributes |= EFI_MEMORY_WB;
>        Status = gDS->SetMemorySpaceAttributes (
>                        mSmramCacheBase,
> @@ -1727,7 +1721,7 @@ SmmIplEntry (
>                 &MemDesc
>                 );
>          DEBUG ((DEBUG_INFO, "SMRAM attributes: %016lx\n",
> MemDesc.Attributes));
> -        ASSERT ((MemDesc.Attributes & MEMORY_PAGE_ATTRIBUTES) == 0);
> +        ASSERT ((MemDesc.Attributes & EFI_MEMORY_ATTRIBUTE_MASK) ==
> 0);
>        );
>      }
>      //
> --
> 2.9.0.windows.1
> 
> 
> Please consider the environment before printing this email.
> 
> The information contained in this message may be confidential and
> proprietary to American Megatrends (AMI).  This communication is intended
> to be read only by the individual or entity to whom it is addressed or by their
> designee. If the reader of this message is not the intended recipient, you are
> on notice that any distribution of this message, in any form, is strictly
> prohibited.  Please promptly notify the sender by reply e-mail or by
> telephone at 770-246-8600, and then delete or destroy all copies of the
> transmission.