Re: [PATCH V2] MdeModulePkg/Gcd: Filter gCpu->SetMemoryAttributes() calls

["Zeng, Star" <star.zeng@intel.com>]

Thanks all.
Push the patch at 0c9f2cb10b7ddec56a3440e77219fd3ab1725e5c. :)


Star
-----Original Message-----
From: Kinney, Michael D 
Sent: Thursday, April 5, 2018 4:28 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Dong, Eric <eric.dong@intel.com>; Renhao Liang <liangrenhao@huawei.com>; Gao, Liming <liming.gao@intel.com>; Heyi Guo <heyi.guo@linaro.org>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH V2] MdeModulePkg/Gcd: Filter gCpu->SetMemoryAttributes() calls

Star,

Thanks for adding the comments

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, April 4, 2018 1:13 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2- devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; 
> Dong, Eric <eric.dong@intel.com>; Renhao Liang 
> <liangrenhao@huawei.com>; Gao, Liming <liming.gao@intel.com>; Heyi Guo 
> <heyi.guo@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; 
> Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH V2] MdeModulePkg/Gcd: Filter
> gCpu->SetMemoryAttributes() calls
> 
> Thanks.
> 
> Reviewed-by: Jiewen.yao@intel.com
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-
> bounces@lists.01.org] On Behalf Of Star
> > Zeng
> > Sent: Wednesday, April 4, 2018 10:08 AM
> > To: edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li
> <phoenix.liyi@huawei.com>; Dong, Eric
> > <eric.dong@intel.com>; Renhao Liang
> <liangrenhao@huawei.com>; Gao, Liming
> > <liming.gao@intel.com>; Heyi Guo
> <heyi.guo@linaro.org>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> > Subject: [edk2] [PATCH V2] MdeModulePkg/Gcd: Filter
> > gCpu->SetMemoryAttributes() calls
> >
> > From: "Kinney, Michael D"
> <michael.d.kinney@intel.com>
> >
> > 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 <heyi.guo@linaro.org>
> > Cc: Yi Li <phoenix.liyi@huawei.com>
> > Cc: Renhao Liang <liangrenhao@huawei.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Signed-off-by: Michael D Kinney
> <michael.d.kinney@intel.com>
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > 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
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel