Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

I agree to remove the check.
Code only grabs the interested bit and calls Cpu->SetMemoryAttributes().

Thanks/Ray

> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, March 29, 2018 2:13 PM
> To: Guo Heyi <heyi.guo@linaro.org>
> Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; Yi Li
> <phoenix.liyi@huawei.com>; Renhao Liang <liangrenhao@huawei.com>;
> Dong, Eric <eric.dong@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> 
> I think you can simply remove the check.
> 
> Jiewen and Ruiyu, do you have any different thoughts?
> 
> Regards,
> Jian
> 
> 
> > -----Original Message-----
> > From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > Sent: Thursday, March 29, 2018 1:55 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: Guo Heyi <heyi.guo@linaro.org>; Zeng, Star <star.zeng@intel.com>;
> > edk2- devel@lists.01.org; Yi Li <phoenix.liyi@huawei.com>; Renhao
> > Liang <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>;
> > Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <liming.gao@intel.com>
> > Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > conversion
> >
> > Agree.
> >
> > And what's your suggestion to fix the current issue?
> >
> > Regards,
> > Heyi
> >
> >
> > On Thu, Mar 29, 2018 at 05:44:16AM +0000, Wang, Jian J wrote:
> > > Hi Heyi,
> > >
> > > Yeah, you're probably right. Page attributes are allowed to be
> > > cleared but we have no separate parameter or interface to differentiate
> such situation.
> > > I think there's flaw in the interface design. But it's hard to change it now.
> > >
> > > Regards,
> > > Jian
> > >
> > >
> > > > -----Original Message-----
> > > > From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > > > Sent: Thursday, March 29, 2018 1:09 PM
> > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo
> > > > <heyi.guo@linaro.org>;
> > edk2-
> > > > devel@lists.01.org; Yi Li <phoenix.liyi@huawei.com>; Renhao Liang
> > > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>;
> > > > Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <liming.gao@intel.com>
> > > > Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > > > conversion
> > > >
> > > > Hi Jian,
> > > >
> > > > I excluded CpuArchAttributes == 0 on purpose, for I don't know the
> > > > expected behaviour of gCpu->SetMemoryAttributes() if
> > > > CpuArchAttributes == 0. Does
> > that
> > > > mean we need to keep the cache attribute and remove memory
> > > > protection attributes?
> > > >
> > > > If so, then it seems we don't need the check at all.
> > > >
> > > > Thanks,
> > > >
> > > > Heyi
> > > >
> > > > On Thu, Mar 29, 2018 at 04:40:33AM +0000, Wang, Jian J wrote:
> > > > > I agree. The only issue here is that case "Attributes == 0" is
> > > > > also excluded in
> > this
> > > > patch.
> > > > > I think 0 should be CPU arch supported attributes.
> > > > >
> > > > > Regards,
> > > > > Jian
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zeng, Star
> > > > > > Sent: Thursday, March 29, 2018 11:20 AM
> > > > > > To: Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org
> > > > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang
> > > > > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>;
> > > > > > Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > > <liming.gao@intel.com>;
> > > > > > Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star
> > > > > > <star.zeng@intel.com>
> > > > > > Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > conversion
> > > > > >
> > > > > > The code before 14dde9e903bb9a719ebb8f3381da72b19509bc36
> can
> > allow
> > > > the
> > > > > > attribute to be a combination of CPU arch attribute and other
> > > > > > attributes,
> > for
> > > > > > example, UC + RUNTIME.
> > > > > > But current code could not allow the case, that seems a
> > > > > > regression in the
> > > > code.
> > > > > > So, I agree the statement.
> > > > > >
> > > > > > Jian, will you please provide the special case you are awared?
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Star
> > > > > > -----Original Message-----
> > > > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]
> > > > > > Sent: Thursday, March 29, 2018 10:32 AM
> > > > > > To: edk2-devel@lists.01.org
> > > > > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li
> > > > > > <phoenix.liyi@huawei.com>;
> > > > Renhao
> > > > > > Liang <liangrenhao@huawei.com>; Zeng, Star
> > > > > > <star.zeng@intel.com>;
> > Dong,
> > > > > > Eric <eric.dong@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>;
> > > > > > Gao, Liming <liming.gao@intel.com>
> > > > > > Subject: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > > > > > conversion
> > > > > >
> > > > > > For gDS->SetMemorySpaceAttributes(), when user passes a
> > > > > > combined
> > > > memory
> > > > > > attribute including CPU arch attribute and other attributes,
> > > > > > like EFI_MEMORY_RUNTIME, ConverToCpuArchAttributes() will
> > > > > > return INVALID_CPU_ARCH_ATTRIBUTES and skip setting
> page/cache
> > > > > > attribute
> > for
> > > > the
> > > > > > specified memory space.
> > > > > >
> > > > > > We don't see any reason to forbid combining CPU arch
> > > > > > attributes and
> > non-
> > > > CPU-
> > > > > > arch attributes when calling gDS->SetMemorySpaceAttributes(),
> > > > > > so we
> > > > change
> > > > > > ConverToCpuArchAttributes() to only check if there is valid
> > > > > > CPU arch
> > > > attributes in
> > > > > > the input "Attribute" parameter and just ignore other attributes.
> > > > > >
> > > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> > > > > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>
> > > > > > Signed-off-by: Renhao Liang <liangrenhao@huawei.com>
> > > > > > Cc: Star Zeng <star.zeng@intel.com>
> > > > > > Cc: Eric Dong <eric.dong@intel.com>
> > > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > > > ---
> > > > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index
> > 77f4adb4bc01..2ababdd14cc6
> > > > > > 100644
> > > > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > > @@ -673,8 +673,8 @@ ConverToCpuArchAttributes (  {
> > > > > >    UINT64      CpuArchAttributes;
> > > > > >
> > > > > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
> > > > > > +  if ((Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > > > +                     NONEXCLUSIVE_MEMORY_ATTRIBUTES)) == 0) {
> > > > > >      return INVALID_CPU_ARCH_ATTRIBUTES;
> > > > > >    }
> > > > > >
> > > > > > --
> > > > > > 2.7.4
> > > > >