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:ler...@redhat.com]
> Sent: Tuesday, June 16, 2020 2:12 PM
> To: Oleksiy Yakovlev; devel@edk2.groups.io
> Cc: eric.d...@intel.com; ray...@intel.com; rahul1.ku...@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 <oleks...@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.
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61398): https://edk2.groups.io/g/devel/message/61398
Mute This Topic: https://groups.io/mt/74904942/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
