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

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

Just thought more about the case.

If a memory range has WB attribute, and code is to set RUNTIME attribute, then the WB attribute will be clear in GCD database, it seems wrong. :(
For this case, need have OR operation ( | RUNTIME) to original attribute in GCD database?


Thanks,
Star
-----Original Message-----
From: Yao, Jiewen 
Sent: Tuesday, April 3, 2018 10:34 AM
To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@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

That is good idea.

Just add more description as the code comment, please.

It is easy for code review later.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, April 3, 2018 10:33 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J 
> <jian.j.wang@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>; Zeng, Star 
> <star.zeng@intel.com>
> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 
> conversion
> 
> 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