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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Star,

Can you update my patch with the additional comments
and commit log message that clarifies the behavior
of gDS->SetMemorySpaceAttributes() for different bit 
settings?

Thanks,

Mike

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Monday, April 2, 2018 7:34 PM
> 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\SmmMemo
> ryAttribut
> > > 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