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

[Guo Heyi <heyi.guo@linaro.org>]

That makes sense to me; I'll send a patch to make the change.

Thanks,
Heyi

On Thu, Mar 29, 2018 at 07:12:16AM +0000, Ni, Ruiyu wrote:
> 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
> > > > > >