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

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

On Tue, Apr 03, 2018 at 02:25:30AM +0000, Yao, Jiewen wrote:
> 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().

Agree, "Get and then Set" seems to be the clearest definition for
gDS->SetMemoryAttributes().

Thanks,

Heyi

> 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