Hi Oleksiy, On 06/30/20 23:11, Oleksiy Yakovlev wrote: > Hi Laszlo. > > I think WP should be included also. Spec says that WP "typically used as a > cacheability attribute today". > > Do you want me to submit just additional patch for CpuDxe.h, or resubmit the > whole series adding this inclusion of WP to EFI_MEMORY_CACHETYPE_MASK in > CpuDxe.h?
I'd suggest posting a v3 patch set, with 4 patches in total: - v4 1/4: equals v3 1/3 - v4 2/4: equals v3 2/3 - v4 3/4: new patch (add WP to EFI_MEMORY_CACHETYPE_MASK) - v4 4/4: v3 3/3, updated The reason for my suggestion is that, once you add EFI_MEMORY_WP to EFI_MEMORY_CACHETYPE_MASK, the present patch will no longer apply verbatim (it will have to be rebased). Namely, the hunk that removes the EFI_MEMORY_CACHETYPE_MASK #define needs to be updated for EFI_MEMORY_WP. When posting v4, please pick up the feedback tags you received during the v3 review. (The small update for the final patch in the series does not invalidate my R-b.) Thank you! Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, June 24, 2020 5:42 AM > To: Oleksiy Yakovlev; devel@edk2.groups.io > Cc: liming....@intel.com; michael.d.kin...@intel.com; dandan...@intel.com; > ray...@intel.com; rahul1.ku...@intel.com; Felix Polyudov; Eric Dong > Subject: Re: [PATCH V2 3/3] UefiCpuPkg: Add New Memory Attributes > > On 06/23/20 23:55, 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). >> Use attributes bitmasks, defined in MdePkg. >> >> Signed-off-by: Oleksiy Yakovlev <oleks...@ami.com> >> --- >> UefiCpuPkg/CpuDxe/CpuDxe.c | 11 ++++------- >> UefiCpuPkg/CpuDxe/CpuDxe.h | 12 ------------ >> UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +++--- >> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 2 +- >> 4 files changed, 8 insertions(+), 23 deletions(-) >> >> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c >> index a571fc3..52cc26e 100644 >> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c >> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c >> @@ -10,9 +10,6 @@ >> #include "CpuMp.h" >> #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) >> - >> // >> // Global Variables >> // >> @@ -417,8 +414,8 @@ CpuSetMemoryAttributes ( >> return EFI_SUCCESS; >> } >> >> - CacheAttributes = Attributes & CACHE_ATTRIBUTE_MASK; >> - MemoryAttributes = Attributes & MEMORY_ATTRIBUTE_MASK; >> + CacheAttributes = Attributes & EFI_CACHE_ATTRIBUTE_MASK; >> + MemoryAttributes = Attributes & EFI_MEMORY_ATTRIBUTE_MASK; >> >> if (Attributes != (CacheAttributes | MemoryAttributes)) { >> return EFI_INVALID_PARAMETER; > > OK. > >> @@ -677,7 +674,7 @@ SetGcdMemorySpaceAttributes ( >> gDS->SetMemorySpaceAttributes ( >> RegionStart, >> RegionLength, >> - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) >> | (MemorySpaceMap[Index].Capabilities & Attributes) >> + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) | >> (MemorySpaceMap[Index].Capabilities & Attributes) >> ); >> } >> >> @@ -754,7 +751,7 @@ RefreshMemoryAttributesFromMtrr ( >> gDS->SetMemorySpaceAttributes ( >> MemorySpaceMap[Index].BaseAddress, >> MemorySpaceMap[Index].Length, >> - (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) | >> + (MemorySpaceMap[Index].Attributes & ~EFI_CACHE_ATTRIBUTE_MASK) | >> (MemorySpaceMap[Index].Capabilities & DefaultAttributes) >> ); >> } >> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h >> index 9299eaa..9771ec8 100644 >> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h >> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h >> @@ -39,18 +39,6 @@ >> #include <Guid/IdleLoopEvent.h> >> #include <Guid/VectorHandoffTable.h> >> >> -#define EFI_MEMORY_CACHETYPE_MASK (EFI_MEMORY_UC | \ >> - EFI_MEMORY_WC | \ >> - EFI_MEMORY_WT | \ >> - EFI_MEMORY_WB | \ >> - EFI_MEMORY_UCE \ >> - ) >> - >> -#define EFI_MEMORY_PAGETYPE_MASK (EFI_MEMORY_RP | \ >> - EFI_MEMORY_XP | \ >> - EFI_MEMORY_RO \ >> - ) >> - >> #define HEAP_GUARD_NONSTOP_MODE \ >> ((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > >> BIT6) >> > > (1) These changes have an additional effect. EFI_MEMORY_CACHETYPE_MASK > does not include EFI_MEMORY_WP, but EFI_CACHE_ATTRIBUTE_MASK does. > > (1a) If that change is intentional, then this patch can remain as it is, > but we need an extra patch prepended (i.e., inserted between v2 patches > #2 and #3), for adding EFI_MEMORY_WP to EFI_MEMORY_CACHETYPE_MASK first. > > (1b) If the EFI_MEMORY_WP change is not intended (i.e., it is an > oversight in this patch), then in every spot where we replace > EFI_MEMORY_CACHETYPE_MASK with EFI_CACHE_ATTRIBUTE_MASK, we need to > account for EFI_MEMORY_WP separately. > > ... After reading up on EFI_MEMORY_WP in the UEFI spec, I think it's > (1a) -- meaning that, this patch is correct, in itself. But, we should > still not hide the EFI_MEMORY_WP bugfix, for EFI_MEMORY_CACHETYPE_MASK, > in this patch. > > So please insert a new patch just before this one, that does nothing > other than include EFI_MEMORY_WP in EFI_MEMORY_CACHETYPE_MASK. > > The rest of the patch looks OK to me. Therefore, for this patch (in itself): > > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > > Eric, Ray, Rahul: correct me if I'm wrong, please. > > Thanks, > Laszlo > >> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c >> b/UefiCpuPkg/CpuDxe/CpuPageTable.c >> index 0a02cb3..06ee1b8 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_ATTRIBUTE_MASK) != 0) { >> DEBUG ((DEBUG_ERROR, "Attributes(0x%lx) has unsupported bit\n", >> Attributes)); >> return EFI_UNSUPPORTED; >> } >> @@ -1018,9 +1018,9 @@ RefreshGcdMemoryAttributesFromPaging ( >> >> Length = MIN (PageLength, MemorySpaceLength); >> if (Attributes != (MemorySpaceMap[Index].Attributes & >> - EFI_MEMORY_PAGETYPE_MASK)) { >> + EFI_MEMORY_ATTRIBUTE_MASK)) { >> NewAttributes = (MemorySpaceMap[Index].Attributes & >> - ~EFI_MEMORY_PAGETYPE_MASK) | Attributes; >> + ~EFI_MEMORY_ATTRIBUTE_MASK) | Attributes; >> Status = gDS->SetMemorySpaceAttributes ( >> BaseAddress, >> Length, >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >> b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c >> index 9c5a92a..ebfc46a 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_ATTRIBUTE_MASK) == 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 (#61971): https://edk2.groups.io/g/devel/message/61971 Mute This Topic: https://groups.io/mt/75070223/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
