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

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

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
>