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

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

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
> > >