* [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
@ 2018-03-29 8:19 Heyi Guo
2018-03-30 1:59 ` Zeng, Star
2018-04-02 5:58 ` Zeng, Star
0 siblings, 2 replies; 15+ messages in thread
From: Heyi Guo @ 2018-03-29 8:19 UTC (permalink / raw)
To: edk2-devel
Cc: Heyi Guo, Yi Li, Renhao Liang, Star Zeng, Eric Dong,
Michael D Kinney, Liming Gao, Jian J Wang, Ruiyu Ni
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-03-29 8:19 [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion Heyi Guo
@ 2018-03-30 1:59 ` Zeng, Star
2018-04-02 5:58 ` Zeng, Star
1 sibling, 0 replies; 15+ messages in thread
From: Zeng, Star @ 2018-03-30 1:59 UTC (permalink / raw)
To: Heyi Guo, edk2-devel@lists.01.org
Cc: Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming,
Wang, Jian J, Ni, Ruiyu, Zeng, Star
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-03-29 8:19 [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion Heyi Guo
2018-03-30 1:59 ` Zeng, Star
@ 2018-04-02 5:58 ` Zeng, Star
2018-04-02 21:43 ` Kinney, Michael D
1 sibling, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2018-04-02 5:58 UTC (permalink / raw)
To: Heyi Guo, edk2-devel@lists.01.org
Cc: Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming,
Wang, Jian J, Ni, Ruiyu, Zeng, Star
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-02 5:58 ` Zeng, Star
@ 2018-04-02 21:43 ` Kinney, Michael D
2018-04-03 0:59 ` Zeng, Star
0 siblings, 1 reply; 15+ messages in thread
From: Kinney, Michael D @ 2018-04-02 21:43 UTC (permalink / raw)
To: Zeng, Star, Heyi Guo, edk2-devel@lists.01.org, Kinney, Michael D
Cc: Yi Li, Renhao Liang, Dong, Eric, Gao, Liming, Wang, Jian J,
Ni, Ruiyu
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-02 21:43 ` Kinney, Michael D
@ 2018-04-03 0:59 ` Zeng, Star
2018-04-03 1:15 ` Zeng, Star
0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2018-04-03 0:59 UTC (permalink / raw)
To: Kinney, Michael D, Heyi Guo, edk2-devel@lists.01.org
Cc: Yi Li, Renhao Liang, Dong, Eric, Gao, Liming, Wang, Jian J,
Ni, Ruiyu, Zeng, Star
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 0:59 ` Zeng, Star
@ 2018-04-03 1:15 ` Zeng, Star
2018-04-03 1:24 ` Wang, Jian J
0 siblings, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2018-04-03 1:15 UTC (permalink / raw)
To: Kinney, Michael D, Heyi Guo, edk2-devel@lists.01.org
Cc: Yi Li, Renhao Liang, Dong, Eric, Gao, Liming, Wang, Jian J,
Ni, Ruiyu, Zeng, Star
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 1:15 ` Zeng, Star
@ 2018-04-03 1:24 ` Wang, Jian J
2018-04-03 2:25 ` Yao, Jiewen
0 siblings, 1 reply; 15+ messages in thread
From: Wang, Jian J @ 2018-04-03 1:24 UTC (permalink / raw)
To: Zeng, Star, Kinney, Michael D, Heyi Guo, edk2-devel@lists.01.org
Cc: Yi Li, Renhao Liang, Dong, Eric, Gao, Liming, Ni, Ruiyu
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 1:24 ` Wang, Jian J
@ 2018-04-03 2:25 ` Yao, Jiewen
2018-04-03 2:33 ` Zeng, Star
2018-04-04 1:12 ` Guo Heyi
0 siblings, 2 replies; 15+ messages in thread
From: Yao, Jiewen @ 2018-04-03 2:25 UTC (permalink / raw)
To: Wang, Jian J, Zeng, Star, Kinney, Michael D, Heyi Guo,
edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Yi Li, Gao, Liming, Dong, Eric, Renhao Liang
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 2:25 ` Yao, Jiewen
@ 2018-04-03 2:33 ` Zeng, Star
2018-04-03 2:34 ` Yao, Jiewen
2018-04-04 1:12 ` Guo Heyi
1 sibling, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2018-04-03 2:33 UTC (permalink / raw)
To: Yao, Jiewen, Wang, Jian J, Kinney, Michael D, Heyi Guo,
edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Yi Li, Gao, Liming, Dong, Eric, Renhao Liang,
Zeng, Star
Agree.
And I agree we need the fix at https://lists.01.org/pipermail/edk2-devel/2018-April/023364.html from Mike for compatibility.
And I think more precious and precise information need to be added into the commit log or code comments since we have been crazy about handling attributes in the GCD service. :)
Thanks,
Star
-----Original Message-----
From: Yao, Jiewen
Sent: Tuesday, April 3, 2018 10:26 AM
To: Wang, Jian J <jian.j.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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 2:33 ` Zeng, Star
@ 2018-04-03 2:34 ` Yao, Jiewen
2018-04-03 2:41 ` Zeng, Star
2018-04-03 21:45 ` Kinney, Michael D
0 siblings, 2 replies; 15+ messages in thread
From: Yao, Jiewen @ 2018-04-03 2:34 UTC (permalink / raw)
To: Zeng, Star, Wang, Jian J, Kinney, Michael D, Heyi Guo,
edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Yi Li, Gao, Liming, Dong, Eric, Renhao Liang
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 2:34 ` Yao, Jiewen
@ 2018-04-03 2:41 ` Zeng, Star
2018-04-03 6:06 ` Yao, Jiewen
2018-04-03 21:45 ` Kinney, Michael D
1 sibling, 1 reply; 15+ messages in thread
From: Zeng, Star @ 2018-04-03 2:41 UTC (permalink / raw)
To: Yao, Jiewen, Wang, Jian J, Kinney, Michael D, Heyi Guo,
edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Yi Li, Gao, Liming, Dong, Eric, Renhao Liang,
Zeng, Star
Just thought more about the case.
If a memory range has WB attribute, and code is to set RUNTIME attribute, then the WB attribute will be clear in GCD database, it seems wrong. :(
For this case, need have OR operation ( | RUNTIME) to original attribute in GCD database?
Thanks,
Star
-----Original Message-----
From: Yao, Jiewen
Sent: Tuesday, April 3, 2018 10:34 AM
To: Zeng, Star <star.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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 2:41 ` Zeng, Star
@ 2018-04-03 6:06 ` Yao, Jiewen
0 siblings, 0 replies; 15+ messages in thread
From: Yao, Jiewen @ 2018-04-03 6:06 UTC (permalink / raw)
To: Zeng, Star, Wang, Jian J, Kinney, Michael D, Heyi Guo,
edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Yi Li, Gao, Liming, Dong, Eric, Renhao Liang
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 2:34 ` Yao, Jiewen
2018-04-03 2:41 ` Zeng, Star
@ 2018-04-03 21:45 ` Kinney, Michael D
2018-04-04 0:38 ` Zeng, Star
1 sibling, 1 reply; 15+ messages in thread
From: Kinney, Michael D @ 2018-04-03 21:45 UTC (permalink / raw)
To: Yao, Jiewen, Zeng, Star, Wang, Jian J, Heyi Guo,
edk2-devel@lists.01.org, Kinney, Michael D
Cc: Ni, Ruiyu, Yi Li, Gao, Liming, Dong, Eric, Renhao Liang
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 21:45 ` Kinney, Michael D
@ 2018-04-04 0:38 ` Zeng, Star
0 siblings, 0 replies; 15+ messages in thread
From: Zeng, Star @ 2018-04-04 0:38 UTC (permalink / raw)
To: Kinney, Michael D, Yao, Jiewen, Wang, Jian J, Heyi Guo,
edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Yi Li, Gao, Liming, Dong, Eric, Renhao Liang,
Zeng, Star
Sure, I can do that.
Thanks,
Star
-----Original Message-----
From: Kinney, Michael D
Sent: Wednesday, April 4, 2018 5:45 AM
To: Yao, Jiewen <jiewen.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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
2018-04-03 2:25 ` Yao, Jiewen
2018-04-03 2:33 ` Zeng, Star
@ 2018-04-04 1:12 ` Guo Heyi
1 sibling, 0 replies; 15+ messages in thread
From: Guo Heyi @ 2018-04-04 1:12 UTC (permalink / raw)
To: Yao, Jiewen
Cc: Wang, Jian J, Zeng, Star, Kinney, Michael D, Heyi Guo,
edk2-devel@lists.01.org, Ni, Ruiyu, Yi Li, Gao, Liming,
Dong, Eric, Renhao Liang
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-04-04 1:12 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-29 8:19 [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion Heyi Guo
2018-03-30 1:59 ` Zeng, Star
2018-04-02 5:58 ` Zeng, Star
2018-04-02 21:43 ` Kinney, Michael D
2018-04-03 0:59 ` Zeng, Star
2018-04-03 1:15 ` Zeng, Star
2018-04-03 1:24 ` Wang, Jian J
2018-04-03 2:25 ` Yao, Jiewen
2018-04-03 2:33 ` Zeng, Star
2018-04-03 2:34 ` Yao, Jiewen
2018-04-03 2:41 ` Zeng, Star
2018-04-03 6:06 ` Yao, Jiewen
2018-04-03 21:45 ` Kinney, Michael D
2018-04-04 0:38 ` Zeng, Star
2018-04-04 1:12 ` Guo Heyi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox