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

2018-04-03 Thread Guo Heyi 
On Tue, Apr 03, 2018 at 02:25:30AM +, 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-boun...@lists.01.org] On Behalf Of Wang,
> > Jian J
> > Sent: Tuesday, April 3, 2018 9:24 AM
> > To: Zeng, Star <star.z...@intel.com>; Kinney, Michael D
> > <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>;
> > edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; Gao,
> > Liming <liming....@intel.com>; Dong, Eric <eric.d...@intel.com>; Renhao
> > Liang <liangren...@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.kin...@intel.com>; Heyi Guo
> > > <heyi@linaro.org>; edk2-devel@lists.01.org
> > > Cc: Yi Li <phoenix.l...@huawei.com>; Renhao Liang
> > > <liangren...@huawei.com>; Dong, Eric <eric.d...@intel.com>; Gao, Liming
> > > <liming@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Ni, Ruiyu
> > > <ruiyu...@intel.com>; Zeng, Star <star.z...@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.kin...@intel.com>; Heyi Guo
> > > <heyi@linaro.org>; edk2-devel@lists.01.org
> > > Cc: Yi Li <phoenix.l...@huawei.com>; Renhao Liang
> > > <liangren...@huawei.com>; Dong, Eric <eric.d...@intel.com>; Gao, Liming
> > > <liming@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Ni, Ruiyu
> > > <ruiyu...@intel.com>; Zeng, Star <star.z...@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?
> > >
> > >
> >

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

2018-04-03 Thread Zeng, Star 
Sure, I can do that.

Thanks,
Star
-Original Message-
From: Kinney, Michael D 
Sent: Wednesday, April 4, 2018 5:45 AM
To: Yao, Jiewen <jiewen@intel.com>; Zeng, Star <star.z...@intel.com>; Wang, 
Jian J <jian.j.w...@intel.com>; Heyi Guo <heyi@linaro.org>; 
edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kin...@intel.com>
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; Gao, 
Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; Renhao Liang 
<liangren...@huawei.com>
Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

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.z...@intel.com>; Wang, Jian J 
> <jian.j.w...@intel.com>; Kinney, Michael D 
> <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>; 
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; 
> Gao, Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; 
> Renhao Liang <liangren...@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@intel.com>; Wang, Jian J
> <jian.j.w...@intel.com>;
> > Kinney, Michael D <michael.d.kin...@intel.com>; Heyi
> Guo
> > <heyi@linaro.org>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li
> <phoenix.l...@huawei.com>; Gao,
> > Liming <liming@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Renhao
> > Liang <liangren...@huawei.com>; Zeng, Star
> <star.z...@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.w...@intel.com>; Zeng, Star
> <star.z...@intel.com>;
> > Kinney, Michael D <michael.d.kin...@intel.com>; Heyi
> Guo
> > <heyi@linaro.org>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li
> <phoenix.l...@huawei.com>; Gao,
> > Liming <liming@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Renhao
> > Liang <liangren...@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-
> boun...@lists.01.org] On Behalf Of
> > > Wang, Jian J
> > > Sent: Tuesday, April 3, 2018 9:24 AM
> > > To: Zeng, S

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

2018-04-03 Thread Kinney, Michael D 
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.z...@intel.com>; Wang, Jian J
> <jian.j.w...@intel.com>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Heyi Guo
> <heyi@linaro.org>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li
> <phoenix.l...@huawei.com>; Gao, Liming
> <liming@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Renhao Liang
> <liangren...@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@intel.com>; Wang, Jian J
> <jian.j.w...@intel.com>;
> > Kinney, Michael D <michael.d.kin...@intel.com>; Heyi
> Guo
> > <heyi@linaro.org>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li
> <phoenix.l...@huawei.com>; Gao,
> > Liming <liming@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Renhao
> > Liang <liangren...@huawei.com>; Zeng, Star
> <star.z...@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.w...@intel.com>; Zeng, Star
> <star.z...@intel.com>;
> > Kinney, Michael D <michael.d.kin...@intel.com>; Heyi
> Guo
> > <heyi@linaro.org>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li
> <phoenix.l...@huawei.com>; Gao,
> > Liming <liming@intel.com>; Dong, Eric
> <eric.d...@intel.com>; Renhao
> > Liang <liangren...@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-
> boun...@lists.01.org] On Behalf Of
> > > Wang, Jian J
> > > Sent: Tuesday, April 3, 2018 9:24 AM
> > > To: Zeng, Star <star.z...@intel.com>; Kinney,
> Michael D
> > > <michael.d.kin...@intel.com>; Heyi Guo
> <heyi@linaro.org>;
> > > edk2-devel@lists.01.org
> > > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li
> <phoenix.l...@huawei.com>;
> > > Gao, Liming <liming@intel.com>; Dong, Eric
> <eric.d...@intel.com>;
> > > Renhao Liang <liangren...@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

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

2018-04-03 Thread Yao, Jiewen 
Right.
I think we only support clear the page attribute, and we don't support the 
cache attribute.
Using OR for RUNTIME is good.

Thank you
Yao Jiewen

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, April 3, 2018 10:41 AM
> To: Yao, Jiewen <jiewen@intel.com>; Wang, Jian J <jian.j.w...@intel.com>;
> Kinney, Michael D <michael.d.kin...@intel.com>; Heyi Guo
> <heyi@linaro.org>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; Gao,
> Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; Renhao
> Liang <liangren...@huawei.com>; Zeng, Star <star.z...@intel.com>
> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> 
> 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.z...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>;
> Kinney, Michael D <michael.d.kin...@intel.com>; Heyi Guo
> <heyi@linaro.org>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; Gao,
> Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; Renhao
> Liang <liangren...@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@intel.com>; Wang, Jian J
> > <jian.j.w...@intel.com>; Kinney, Michael D
> > <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>;
> > edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>;
> > Gao, Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>;
> > Renhao Liang <liangren...@huawei.com>; Zeng, Star
> > <star.z...@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.w...@intel.com>; Zeng, Star
> > <star.z...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>;
> > Heyi Guo <heyi@linaro.org>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>;
> > Gao, Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>;
> > Renhao Liang <liangren...@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---

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

2018-04-02 Thread Zeng, Star 
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.z...@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; 
Kinney, Michael D <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>; 
edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; Gao, 
Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; Renhao Liang 
<liangren...@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@intel.com>; Wang, Jian J 
> <jian.j.w...@intel.com>; Kinney, Michael D 
> <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>; 
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; 
> Gao, Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; 
> Renhao Liang <liangren...@huawei.com>; Zeng, Star 
> <star.z...@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.w...@intel.com>; Zeng, Star 
> <star.z...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>; 
> Heyi Guo <heyi@linaro.org>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; 
> Gao, Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; 
> Renhao Liang <liangren...@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-boun...@lists.01.org] On Behalf 
> > Of Wang, Jian J
> > Sent: Tuesday, April 3, 2018 9:24 AM
> > To: Zeng, Star <star.z...@intel.com>; Kinney, Michael D 
> > <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>; 
> > edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; 
> > Gao, Liming <liming@intel.com>; Dong, Eric 
> > <eric.d...@intel.com>; Renhao Liang <liangren...@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.kin...@intel.com>; Heyi Gu

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

2018-04-02 Thread Yao, Jiewen 
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@intel.com>; Wang, Jian J <jian.j.w...@intel.com>;
> Kinney, Michael D <michael.d.kin...@intel.com>; Heyi Guo
> <heyi@linaro.org>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; Gao,
> Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; Renhao
> Liang <liangren...@huawei.com>; Zeng, Star <star.z...@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.w...@intel.com>; Zeng, Star <star.z...@intel.com>;
> Kinney, Michael D <michael.d.kin...@intel.com>; Heyi Guo
> <heyi@linaro.org>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; Gao,
> Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; Renhao
> Liang <liangren...@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-boun...@lists.01.org] On Behalf Of
> > Wang, Jian J
> > Sent: Tuesday, April 3, 2018 9:24 AM
> > To: Zeng, Star <star.z...@intel.com>; Kinney, Michael D
> > <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>;
> > edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>;
> > Gao, Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>;
> > Renhao Liang <liangren...@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.kin...@intel.com>; Heyi Guo
> > > <heyi@linaro.org>; edk2-devel@lists.01.org
> > > Cc: Yi Li <phoenix.l...@huawei.com>; Renhao Liang
> > > <liangren...@huawei.com>; Dong, Eric <eric.d...@intel.com>; Gao,
> > > Liming <liming@intel.com>; Wang, Jian J <jian.j.w...@intel.com>;
> > > Ni, Ruiyu <ruiyu...@intel.com>; Zeng, Star <star.z...@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

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

2018-04-02 Thread Zeng, Star 
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.w...@intel.com>; Zeng, Star <star.z...@intel.com>; 
Kinney, Michael D <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>; 
edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; Gao, 
Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; Renhao Liang 
<liangren...@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-boun...@lists.01.org] On Behalf Of 
> Wang, Jian J
> Sent: Tuesday, April 3, 2018 9:24 AM
> To: Zeng, Star <star.z...@intel.com>; Kinney, Michael D 
> <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>; 
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; 
> Gao, Liming <liming....@intel.com>; Dong, Eric <eric.d...@intel.com>; 
> Renhao Liang <liangren...@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.kin...@intel.com>; Heyi Guo 
> > <heyi@linaro.org>; edk2-devel@lists.01.org
> > Cc: Yi Li <phoenix.l...@huawei.com>; Renhao Liang 
> > <liangren...@huawei.com>; Dong, Eric <eric.d...@intel.com>; Gao, 
> > Liming <liming@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; 
> > Ni, Ruiyu <ruiyu...@intel.com>; Zeng, Star <star.z...@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.kin...@intel.com>; Heyi Guo 
> > <heyi@linaro.org>; edk2-devel@lists.01.org
> > Cc: Yi Li <phoenix.l...@huawei.com>; Renhao Liang 
> > <liangren...@huawei.com>; Dong, Eric <eric.d...@intel.com>; Gao, 
> > Liming <liming@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; 
> > Ni, Ruiyu <ruiyu...@intel.com>; Zeng, Star <star.z...@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 discu

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

2018-04-02 Thread Yao, Jiewen 
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-boun...@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Tuesday, April 3, 2018 9:24 AM
> To: Zeng, Star <star.z...@intel.com>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Heyi Guo <heyi@linaro.org>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yi Li <phoenix.l...@huawei.com>; Gao,
> Liming <liming@intel.com>; Dong, Eric <eric.d...@intel.com>; Renhao
> Liang <liangren...@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.kin...@intel.com>; Heyi Guo
> > <heyi@linaro.org>; edk2-devel@lists.01.org
> > Cc: Yi Li <phoenix.l...@huawei.com>; Renhao Liang
> > <liangren...@huawei.com>; Dong, Eric <eric.d...@intel.com>; Gao, Liming
> > <liming@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Ni, Ruiyu
> > <ruiyu...@intel.com>; Zeng, Star <star.z...@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.kin...@intel.com>; Heyi Guo
> > <heyi@linaro.org>; edk2-devel@lists.01.org
> > Cc: Yi Li <phoenix.l...@huawei.com>; Renhao Liang
> > <liangren...@huawei.com>; Dong, Eric <eric.d...@intel.com>; Gao, Liming
> > <liming@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Ni, Ruiyu
> > <ruiyu...@intel.com>; Zeng, Star <star.z...@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.z...@intel.com>; Heyi Guo <heyi@linaro.org>; edk2-
> > de...@lists.01.org; Kinney, Michael D <michael.d.kin...@intel.com>
> > Cc: Yi Li <phoenix.l...@huawei.com>; Renhao Liang
> > <liangren...@huawei.com>; Dong, Eric <eric.d...@intel.com>; Gao, Liming
> > <liming@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Ni, Ruiyu
> > <ruiyu...@intel.com>
> > Subject: RE: [PATCH 1/1] MdeModu

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

2018-04-02 Thread Wang, Jian J 
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 ; Heyi Guo
> ; edk2-devel@lists.01.org
> Cc: Yi Li ; Renhao Liang
> ; Dong, Eric ; Gao, Liming
> ; Wang, Jian J ; Ni, Ruiyu
> ; Zeng, Star 
> 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\SmmMemoryAttribute.
> 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 ; Heyi Guo
> ; edk2-devel@lists.01.org
> Cc: Yi Li ; Renhao Liang
> ; Dong, Eric ; Gao, Liming
> ; Wang, Jian J ; Ni, Ruiyu
> ; Zeng, Star 
> 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 ; Heyi Guo ; edk2-
> de...@lists.01.org; Kinney, Michael D 
> Cc: Yi Li ; Renhao Liang
> ; Dong, Eric ; Gao, Liming
> ; Wang, Jian J ; Ni, Ruiyu
> 
> 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 ; edk2- de...@lists.01.org
> > Cc: Yi Li ; Renhao Liang
> > ; Dong, Eric ; Kinney,
> > Michael D ; Gao, Liming
> > ; Wang, Jian J ; Ni,
> > Ruiyu ; Zeng, Star 
> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > conversion
> >
> > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: Heyi Guo [mailto:heyi@linaro.org]
> > Sent: Thursday, March 29, 2018 4:20 PM
> > To: edk2-devel@lists.01.org
> > Cc: Heyi Guo ; Yi Li ;
> > Renhao Liang ; Zeng, Star
> > ; Dong, Eric ; Kinney,
> > Michael D ; Gao, Liming
> > ; Wang, Jian J ; Ni,
> > Ruiyu 
> > 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

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

2018-04-02 Thread Zeng, Star 
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\SmmMemoryAttribute.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 ; Heyi Guo 
; edk2-devel@lists.01.org
Cc: Yi Li ; Renhao Liang ; 
Dong, Eric ; Gao, Liming ; Wang, 
Jian J ; Ni, Ruiyu ; Zeng, Star 

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 ; Heyi Guo ; 
edk2-devel@lists.01.org; Kinney, Michael D 
Cc: Yi Li ; Renhao Liang ; 
Dong, Eric ; Gao, Liming ; Wang, 
Jian J ; Ni, Ruiyu 
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 ; edk2- de...@lists.01.org
> Cc: Yi Li ; Renhao Liang 
> ; Dong, Eric ; Kinney, 
> Michael D ; Gao, Liming 
> ; Wang, Jian J ; Ni, 
> Ruiyu ; Zeng, Star 
> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 
> conversion
> 
> Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.
> 
> Thanks,
> Star
> -Original Message-
> From: Heyi Guo [mailto:heyi@linaro.org]
> Sent: Thursday, March 29, 2018 4:20 PM
> To: edk2-devel@lists.01.org
> Cc: Heyi Guo ; Yi Li ; 
> Renhao Liang ; Zeng, Star 
> ; Dong, Eric ; Kinney, 
> Michael D ; Gao, Liming 
> ; Wang, Jian J ; Ni, 
> Ruiyu 
> 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 
> Signed-off-by: Yi Li 
> Signed-off-by: Renhao Liang 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Jian J Wang 
> Cc: Ruiyu Ni

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

2018-04-02 Thread Zeng, Star 
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 ; Heyi Guo ; 
edk2-devel@lists.01.org; Kinney, Michael D 
Cc: Yi Li ; Renhao Liang ; 
Dong, Eric ; Gao, Liming ; Wang, 
Jian J ; Ni, Ruiyu 
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 ; edk2- de...@lists.01.org
> Cc: Yi Li ; Renhao Liang 
> ; Dong, Eric ; Kinney, 
> Michael D ; Gao, Liming 
> ; Wang, Jian J ; Ni, 
> Ruiyu ; Zeng, Star 
> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 
> conversion
> 
> Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.
> 
> Thanks,
> Star
> -Original Message-
> From: Heyi Guo [mailto:heyi@linaro.org]
> Sent: Thursday, March 29, 2018 4:20 PM
> To: edk2-devel@lists.01.org
> Cc: Heyi Guo ; Yi Li ; 
> Renhao Liang ; Zeng, Star 
> ; Dong, Eric ; Kinney, 
> Michael D ; Gao, Liming 
> ; Wang, Jian J ; Ni, 
> Ruiyu 
> 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 
> Signed-off-by: Yi Li 
> Signed-off-by: Renhao Liang 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Jian J Wang 
> Cc: Ruiyu Ni 
> ---
>  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

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

2018-04-02 Thread Kinney, Michael D 
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 ; edk2-
> de...@lists.01.org
> Cc: Yi Li ; Renhao Liang
> ; Dong, Eric
> ; Kinney, Michael D
> ; Gao, Liming
> ; Wang, Jian J
> ; Ni, Ruiyu ;
> Zeng, Star 
> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of
> attribute conversion
> 
> Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.
> 
> Thanks,
> Star
> -Original Message-
> From: Heyi Guo [mailto:heyi@linaro.org]
> Sent: Thursday, March 29, 2018 4:20 PM
> To: edk2-devel@lists.01.org
> Cc: Heyi Guo ; Yi Li
> ; Renhao Liang
> ; Zeng, Star
> ; Dong, Eric ;
> Kinney, Michael D ; Gao,
> Liming ; Wang, Jian J
> ; Ni, Ruiyu 
> 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 
> Signed-off-by: Yi Li 
> Signed-off-by: Renhao Liang 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Jian J Wang 
> Cc: Ruiyu Ni 
> ---
>  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

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

2018-04-01 Thread Zeng, Star 
Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

Thanks,
Star
-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org] 
Sent: Thursday, March 29, 2018 4:20 PM
To: edk2-devel@lists.01.org
Cc: Heyi Guo ; Yi Li ; Renhao 
Liang ; Zeng, Star ; Dong, Eric 
; Kinney, Michael D ; Gao, 
Liming ; Wang, Jian J ; Ni, Ruiyu 

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 
Signed-off-by: Yi Li 
Signed-off-by: Renhao Liang 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jian J Wang 
Cc: Ruiyu Ni 
---
 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

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

2018-03-29 Thread Zeng, Star 
Reviewed-by: Star Zeng 

Thanks,
Star
-Original Message-
From: Heyi Guo [mailto:heyi@linaro.org] 
Sent: Thursday, March 29, 2018 4:20 PM
To: edk2-devel@lists.01.org
Cc: Heyi Guo ; Yi Li ; Renhao 
Liang ; Zeng, Star ; Dong, Eric 
; Kinney, Michael D ; Gao, 
Liming ; Wang, Jian J ; Ni, Ruiyu 

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 
Signed-off-by: Yi Li 
Signed-off-by: Renhao Liang 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jian J Wang 
Cc: Ruiyu Ni 
---
 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

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

2018-03-29 Thread Heyi Guo 
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 
Signed-off-by: Yi Li 
Signed-off-by: Renhao Liang 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Jian J Wang 
Cc: Ruiyu Ni 
---
 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