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);
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61348): https://edk2.groups.io/g/devel/message/61348
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]
-=-=-=-=-=-=-=-=-=-=-=-
