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

["Zeng, Star" <star.zeng@intel.com>]

Agree.

And I agree we need the fix at https://lists.01.org/pipermail/edk2-devel/2018-April/023364.html from Mike for compatibility.
And I think more precious and precise information need to be added into the commit log or code comments since we have been crazy about handling attributes in the GCD service. :)


Thanks,
Star
-----Original Message-----
From: Yao, Jiewen 
Sent: Tuesday, April 3, 2018 10:26 AM
To: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao Liang <liangrenhao@huawei.com>
Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

I have a discussion with Star/Jian.

The expectation for the CPU driver is to handle PageAttribute.
The expectation for the platform driver is to use GetAttribute(), AND/OR attribute, then call SetAttribute().
Because the DRAM always has a cache attribute (no matter UC or WB), 0 should not happen. (0 might happen for a GCD reserved memory, but there is no need to set page attribute)

If all drivers use above way, there won't be any issue. There is no need to introduce a new protocol so far.

We did encounter some error, because the platform/silicon/CPU code is not updated yet. (For example, the MinnowMax which is using binary) The fix to filter 0 seems a workable way to resolve the compatibility issue.

Later, I suggest we update MinnowMax binary - Add paging handling for CPU driver, and use GetAttribute()/SetAttribute() for platform/silicon code.


Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Wang, Jian J
> Sent: Tuesday, April 3, 2018 9:24 AM
> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>; 
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; 
> Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; 
> Renhao Liang <liangrenhao@huawei.com>
> Subject: Re: [edk2] [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 
> conversion
> 
> The NX Memory Protection and Heap Guard features need to clear paging 
> attributes.
> Allowing 0 attribute is the current only choice without changing arch protocol.
> Maybe
> It's time to introduce a new protocol.
> 
> Regards,
> Jian
> 
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, April 03, 2018 9:16 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; 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>; Gao, 
> > Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; 
> > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 
> > conversion
> >
> > Current gCpu->SetMemoryAttributes does not support getting memory 
> > attributes, and has no clear description about clearing memory attributes.
> >
> > I noticed we introduced
> >
> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribut
> e.
> > h) protocol for heap guard feature in SMM environment.
> > Seemingly, we also need introduce MemoryAttribute protocol for DXE?
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, April 3, 2018 8:59 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>; 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>; Gao, 
> > Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; 
> > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 
> > conversion
> >
> > Mike,
> >
> > Agree the problem.
> > We need support only RUNTIME attribute.
> > We need support only cache attributes.
> > We need support only page attributes.
> > We need support RUNTIME + cache + page attributes.
> > Do we need support the 0 Attributes case to clear page attributes?
> > There was discussion about the 0 Attributes case at 
> > https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.
> > It came to the question that should the 0 Attributes case be handled 
> > by
> > SetMemoryAttributes() to clear the page attributes?
> >
> >
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Kinney, Michael D
> > Sent: Tuesday, April 3, 2018 5:43 AM
> > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo 
> > <heyi.guo@linaro.org>; edk2- devel@lists.01.org; Kinney, Michael D 
> > <michael.d.kinney@intel.com>
> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 
> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, 
> > Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; 
> > Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 
> > conversion
> >
> > Star,
> >
> > This commit breaks Vlv2TbltDevicePkg.
> >
> > On this platform, there are 2 calls to
> > gDS->SetMemorySpaceAttributes() for an MMIO
> > range to set only the EFI_MEMORY_RUNTIME bit.
> >
> > With this commit, ConverToCpuArchAttributes()returns 0, and 0 is 
> > passed to
> > gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on
> > Vlv2TbltDevicePkg.
> >
> > Before this commit, ConverToCpuArchAttributes() returns 
> > INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu-
> > >SetMemoryAttributes()to be skipped so no error is generated.
> >
> > I think the right fix is to skip the call to
> > gCpu->SetMemoryAttributes() if CpuArchAttributes
> > is 0 so a call that only sets the RUNTIME attribute can be supported 
> > along with call the set both the RUNTIME bit and other cache related bits.
> >
> > I will send a patch soon with the proposed fix.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Sunday, April 1, 2018 10:59 PM
> > > 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>; Ni, 
> > > Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 
> > > conversion
> > >
> > > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.
> > >
> > > Thanks,
> > > Star
> > > -----Original Message-----
> > > From: Heyi Guo [mailto:heyi.guo@linaro.org]
> > > Sent: Thursday, March 29, 2018 4:20 PM
> > > 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>; 
> > > Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu 
> > > <ruiyu.ni@intel.com>
> > > Subject: [PATCH 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 remove the check code
> > > in ConverToCpuArchAttributes(); the remaining code is enough to 
> > > grab the interested bits for
> > > Cpu->SetMemoryAttributes().
> > >
> > > 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>
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > ---
> > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----
> > >  1 file changed, 5 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c 
> > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index
> > > 77f4adb4bc01..907245a3f512 100644
> > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {
> > >    UINT64      CpuArchAttributes;
> > >
> > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))
> > > != 0) {
> > > -    return INVALID_CPU_ARCH_ATTRIBUTES;
> > > -  }
> > > -
> > >    CpuArchAttributes = Attributes & 
> > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;
> > >
> > >    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
> > > --
> > > 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel