Re: [PATCH] UefiCpuPkg: Add New Memory Attributes

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

On 06/16/20 20:59, Oleksiy Yakovlev wrote:
> 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 | ).

But MdeModulePkg and UefiCpuPkg still do not share a common macro or Fixed-PCD for the mask, correct?

I think they should. UefiCpuPkg is allowed to consume MdeModulePkg content. And both are allowed to consume MdePkg content.

--*--

*Even if* we're staying strictly within UefiCpuPkg, the following macros are duplicated:

- "UefiCpuPkg/CpuDxe/CpuDxe.h":

#define EFI_MEMORY_PAGETYPE_MASK      (EFI_MEMORY_RP  | \
                                       EFI_MEMORY_XP  | \
                                       EFI_MEMORY_RO    \
                                       )

- "UefiCpuPkg/CpuDxe/CpuDxe.c":

#define MEMORY_ATTRIBUTE_MASK  (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)


They should be united, and the existent UefiCpuPkg code should adopt the new common macro ("CpuPageTable.c", "SmmCpuMemoryManagement.c").

Afterwards, the now-common macro should be extended with the new bits, in a separate patch.

Thanks
Laszlo



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