Re: [PATCH] UefiCpuPkg: Add New Memory Attributes

["Oleksiy Yakovlev" <oleksiyy@ami.com>]

Hi Laszlo.

There was separate patch for MdeModulePkg submitted with this one as well. I did not include you into cc, because you are not in the maintainers list for MdeModulePkg. Patch addresses EFI_MEMORY_(RO|RP|XP) macroses and all issues you mentioned in your comment in same way as UefiCpuPkg patch (without introducing single macros and using MEMORY_ATTRIBUTE_MASK instead of | ).

Regards, Oleksiy.

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, June 16, 2020 2:12 PM
To: Oleksiy Yakovlev; devel@edk2.groups.io
Cc: eric.dong@intel.com; ray.ni@intel.com; rahul1.kumar@intel.com; Felix Polyudov; Liming Gao; Michael Kinney; Jian J Wang; Hao A Wu; Dandan Bi
Subject: Re: [PATCH] UefiCpuPkg: Add New Memory Attributes

Hi Oleksiy,

On 06/15/20 23:45, Oleksiy Yakovlev wrote:
> Add usage of EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO
> attributes introduced in UEFI 2.8
> (UEFI 2.8, mantis 1919 and 1872)
>
> Signed-off-by: Oleksiy Yakovlev <oleksiyy@ami.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.c                         | 2 +-
>  UefiCpuPkg/CpuDxe/CpuDxe.h                         | 4 +++-
>  UefiCpuPkg/CpuDxe/CpuPageTable.c                   | 2 +-
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 2 +-
>  4 files changed, 6 insertions(+), 4 deletions(-)

I suggest / request turning this patch into 5 patches:


(a) The first patch should please correct a mistake in commit
c18708d2f002 ("MdePkg-UefiSpec.h: Add UEFI 2.8 new memory attributes",
2019-11-04).

Namely, in commit c18708d2f002, the EFI_MEMORY_CPU_CRYPTO macro's
documentation includes the string "CPU?s", twice, in place of "CPU's".

I don't understand how this happened. In the mailing list archive, I can
only find Liming's confirmation that he pushed the patch:

  https://edk2.groups.io/g/devel/message/49893

but not the original patch posting.

Note that, in the context quoted in that message (that is, the patch),
the string was "CPU’s". That string did not use ASCII character 0x27,
but U+2019 (RIGHT SINGLE QUOTATION MARK). So indeed the patch was
incorrect. But the solution should not have been to replace U+2019 with
"?", but to request a repost using ASCII 0x27.

Either way, even though it is obviously not your mistake, can you please
include a patch for replacing both "CPU?s" instances with "CPU's"? In
file "MdePkg/Include/Uefi/UefiSpec.h".


For the rest of the patches, please consider:

$ git grep -E 'EFI_MEMORY_(RO|RP|XP) \| EFI_MEMORY_(RO|RP|XP) \| EFI_MEMORY_(RO|RP|XP)'

MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c:#define MEMORY_ATTRIBUTE_MASK  (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c:#define MEMORY_PAGE_ATTRIBUTES  (EFI_MEMORY_XP | EFI_MEMORY_RP | EFI_MEMORY_RO)
UefiCpuPkg/CpuDxe/CpuDxe.c:#define MEMORY_ATTRIBUTE_MASK  (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
UefiCpuPkg/CpuDxe/CpuPageTable.c:  if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0) {
UefiCpuPkg/CpuDxe/CpuPageTable.c:    Capabilities = EFI_MEMORY_RO | EFI_MEMORY_RP | EFI_MEMORY_XP;
UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c:  ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);

This output tells us the following:

- the bitmask (RP|XP|RO) is *triplicated* between the macro
  definitions in:

  - MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
  - MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
  - UefiCpuPkg/CpuDxe/CpuDxe.c

- "UefiCpuPkg/CpuDxe/CpuPageTable.c" open-codes the bitmask in two
  separate spots (rather than using MEMORY_ATTRIBUTE_MASK)

- "UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c" open codes the
  bitmask also (rather than using any macro).

(b) Therefore, the second patch should introduce a central macro for
(RP|XP|RO) somewhere under MdePkg or MdeModulePkg. Perhaps it can even
be a fixed-only PCD.

(c) The third patch should replace all of the open coded bitmasks in
MdeModulePkg (see the list above) with references to the new central
macro (or PCD).

(d) The fourth patch should do the same in UefiCpuPkg.

(e) The final patch should modify the central macro to include
EFI_MEMORY_SP and EFI_MEMORY_CPU_CRYPTO.

This is just my opinion of course, please discuss it further with the
MdePkg / MdeModulePkg / UefiCpuPkg maintainers (I've CC'd them).

Thanks,
Laszlo


>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
> index a571fc3..55ca764 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -11,7 +11,7 @@
>  #include "CpuPageTable.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)
> +#define MEMORY_ATTRIBUTE_MASK  (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)
>
>  //
>  // Global Variables
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
> index 9299eaa..37fb38e 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
> @@ -48,7 +48,9 @@
>
>  #define EFI_MEMORY_PAGETYPE_MASK      (EFI_MEMORY_RP  | \
>                                         EFI_MEMORY_XP  | \
> -                                       EFI_MEMORY_RO    \
> +                                       EFI_MEMORY_RO  | \
> +                                       EFI_MEMORY_SP  | \
> +                                       EFI_MEMORY_CPU_CRYPTO \
>                                         )
>
>  #define HEAP_GUARD_NONSTOP_MODE       \
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index 0a02cb3..d769e4a 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -717,7 +717,7 @@ ConvertMemoryPageAttributes (
>      return RETURN_INVALID_PARAMETER;
>    }
>
> -  if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) != 0) {
> +  if ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)) != 0) {
>      DEBUG ((DEBUG_ERROR, "Attributes(0x%lx) has unsupported bit\n", Attributes));
>      return EFI_UNSUPPORTED;
>    }
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> index 9c5a92a..94adf37 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
> @@ -435,7 +435,7 @@ ConvertMemoryPageAttributes (
>    EFI_PHYSICAL_ADDRESS              MaximumSupportMemAddress;
>
>    ASSERT (Attributes != 0);
> -  ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP)) == 0);
> +  ASSERT ((Attributes & ~(EFI_MEMORY_RP | EFI_MEMORY_RO | EFI_MEMORY_XP | EFI_MEMORY_SP | EFI_MEMORY_CPU_CRYPTO)) == 0);
>
>    ASSERT ((BaseAddress & (SIZE_4KB - 1)) == 0);
>    ASSERT ((Length & (SIZE_4KB - 1)) == 0);
>


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.