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

["Wang, Jian J" <jian.j.wang@intel.com>]

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