Star, Thanks for adding the comments
Reviewed-by: Michael D Kinney <[email protected]> Mike > -----Original Message----- > From: Yao, Jiewen > Sent: Wednesday, April 4, 2018 1:13 AM > To: Zeng, Star <[email protected]>; edk2- > [email protected] > Cc: Ni, Ruiyu <[email protected]>; Yi Li > <[email protected]>; Dong, Eric > <[email protected]>; Renhao Liang > <[email protected]>; Gao, Liming > <[email protected]>; Heyi Guo <[email protected]>; > Kinney, Michael D <[email protected]>; Zeng, > Star <[email protected]> > Subject: RE: [edk2] [PATCH V2] MdeModulePkg/Gcd: Filter > gCpu->SetMemoryAttributes() calls > > Thanks. > > Reviewed-by: [email protected] > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel- > [email protected]] On Behalf Of Star > > Zeng > > Sent: Wednesday, April 4, 2018 10:08 AM > > To: [email protected] > > Cc: Ni, Ruiyu <[email protected]>; Yi Li > <[email protected]>; Dong, Eric > > <[email protected]>; Renhao Liang > <[email protected]>; Gao, Liming > > <[email protected]>; Heyi Guo > <[email protected]>; Kinney, Michael D > > <[email protected]>; Zeng, Star > <[email protected]> > > Subject: [edk2] [PATCH V2] MdeModulePkg/Gcd: Filter > > gCpu->SetMemoryAttributes() calls > > > > From: "Kinney, Michael D" > <[email protected]> > > > > This patch fixes an issue with VlvTbltDevicePkg > introduced > > by commit 5b91bf82c67b586b9588cbe4bbffa1588f6b5926. > > > > The history is as below. > > To support heap guard feature, > > 14dde9e903bb9a719ebb8f3381da72b19509bc36 > > added support for SetMemorySpaceAttributes() to > handle page attributes, > > but after that, a combination of CPU arch attributes > and other attributes > > was not allowed anymore, for example, UC + RUNTIME. > It is a regression. > > Then 5b91bf82c67b586b9588cbe4bbffa1588f6b5926 was to > fix the regression, > > and we thought 0 CPU arch attributes may be used to > clear CPU arch > > attributes, so 0 CPU arch attributes was allowed to > be sent to > > gCpu->SetMemoryAttributes(). > > > > But some implementation of CPU driver may return > error for 0 CPU arch > > attributes. That fails the case that caller just > calls > > SetMemorySpaceAttributes() with none CPU arch > attributes (for example, > > RUNTIME), and the purpose of the case is not to clear > CPU arch attributes. > > > > This patch filters the call to gCpu- > >SetMemoryAttributes() > > if the requested attributes is 0. It also removes > the #define > > INVALID_CPU_ARCH_ATTRIBUTES that is no longer used. > > > > Cc: Heyi Guo <[email protected]> > > Cc: Yi Li <[email protected]> > > Cc: Renhao Liang <[email protected]> > > Cc: Star Zeng <[email protected]> > > Cc: Eric Dong <[email protected]> > > Cc: Liming Gao <[email protected]> > > Cc: Jian J Wang <[email protected]> > > Cc: Ruiyu Ni <[email protected]> > > Signed-off-by: Michael D Kinney > <[email protected]> > > Signed-off-by: Star Zeng <[email protected]> > > Contributed-under: TianoCore Contribution Agreement > 1.1 > > --- > > MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 25 > ++++++++++++++++++++++--- > > 1 file changed, 22 insertions(+), 3 deletions(-) > > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > > index 907245a3f512..31ddf9c3bb51 100644 > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c > > @@ -48,8 +48,6 @@ WITHOUT WARRANTIES OR > REPRESENTATIONS OF ANY > > KIND, EITHER EXPRESS OR IMPLIED. > > #define NONEXCLUSIVE_MEMORY_ATTRIBUTES > (EFI_MEMORY_XP | > > EFI_MEMORY_RP | \ > > > EFI_MEMORY_RO) > > > > -#define INVALID_CPU_ARCH_ATTRIBUTES 0xffffffff > > - > > // > > // Module Variables > > // > > @@ -873,7 +871,21 @@ CoreConvertSpace ( > > // Call CPU Arch Protocol to attempt to set > attributes on the range > > // > > CpuArchAttributes = ConverToCpuArchAttributes > (Attributes); > > - if (CpuArchAttributes != > INVALID_CPU_ARCH_ATTRIBUTES) { > > + // > > + // CPU arch attributes include page attributes > and cache attributes. > > + // Only page attributes supports to be cleared, > but not cache attributes. > > + // Caller is expected to use > GetMemorySpaceDescriptor() to get the current > > + // attributes, AND/OR attributes, and then calls > > SetMemorySpaceAttributes() > > + // to set the new attributes. > > + // So 0 CPU arch attributes should not happen as > memory should always > > have > > + // a cache attribute (no matter UC or WB, etc). > > + // > > + // Here, 0 CPU arch attributes will be filtered > to be compatible with the > > + // case that caller just calls > SetMemorySpaceAttributes() with none CPU > > + // arch attributes (for example, RUNTIME) as the > purpose of the case is not > > + // to clear CPU arch attributes. > > + // > > + if (CpuArchAttributes != 0) { > > if (gCpu == NULL) { > > Status = EFI_NOT_AVAILABLE_YET; > > } else { > > @@ -936,6 +948,13 @@ CoreConvertSpace ( > > // Set attributes operation > > // > > case GCD_SET_ATTRIBUTES_MEMORY_OPERATION: > > + if (CpuArchAttributes == 0) { > > + // > > + // Keep original CPU arch attributes when > caller just calls > > + // SetMemorySpaceAttributes() with none CPU > arch attributes (for > > example, RUNTIME). > > + // > > + Attributes |= (Entry->Attributes & > > (EXCLUSIVE_MEMORY_ATTRIBUTES | > NONEXCLUSIVE_MEMORY_ATTRIBUTES)); > > + } > > Entry->Attributes = Attributes; > > break; > > // > > -- > > 2.7.0.windows.1 > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
