| Message ID | 1522311590-104218-1-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | Accepted |
| Commit | 5b91bf82c67b586b9588cbe4bbffa1588f6b5926 |
| Headers | show |
| Series | [edk2,1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion | expand |
Reviewed-by: Star Zeng <star.zeng@intel.com> 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
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
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
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
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 <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
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\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 <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
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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.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 > > 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> 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 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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) {