* [PATCH] UefiCpuPkg: Add New Memory Attributes @ 2020-06-15 21:45 Oleksiy Yakovlev 2020-06-16 18:11 ` Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Oleksiy Yakovlev @ 2020-06-15 21:45 UTC (permalink / raw) To: devel; +Cc: eric.dong, ray.ni, lersek, rahul1.kumar, Felixp, oleksiyy 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(-) 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); -- 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. ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg: Add New Memory Attributes 2020-06-15 21:45 [PATCH] UefiCpuPkg: Add New Memory Attributes Oleksiy Yakovlev @ 2020-06-16 18:11 ` Laszlo Ersek 2020-06-16 18:59 ` Oleksiy Yakovlev 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2020-06-16 18:11 UTC (permalink / raw) To: Oleksiy Yakovlev, devel Cc: eric.dong, ray.ni, rahul1.kumar, Felixp, Liming Gao, Michael Kinney, Jian J Wang, Hao A Wu, Dandan Bi 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); > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg: Add New Memory Attributes 2020-06-16 18:11 ` Laszlo Ersek @ 2020-06-16 18:59 ` Oleksiy Yakovlev 2020-06-17 10:35 ` Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Oleksiy Yakovlev @ 2020-06-16 18:59 UTC (permalink / raw) To: Laszlo Ersek, 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 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] UefiCpuPkg: Add New Memory Attributes 2020-06-16 18:59 ` Oleksiy Yakovlev @ 2020-06-17 10:35 ` Laszlo Ersek 0 siblings, 0 replies; 4+ messages in thread From: Laszlo Ersek @ 2020-06-17 10:35 UTC (permalink / raw) 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 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. > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-17 10:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-06-15 21:45 [PATCH] UefiCpuPkg: Add New Memory Attributes Oleksiy Yakovlev 2020-06-16 18:11 ` Laszlo Ersek 2020-06-16 18:59 ` Oleksiy Yakovlev 2020-06-17 10:35 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox