diff mbox series

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

Message ID 1522311590-104218-1-git-send-email-heyi.guo@linaro.org
State Accepted
Commit 5b91bf82c67b586b9588cbe4bbffa1588f6b5926
Headers show
Series [edk2,1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion | expand

Commit Message

gary guo March 29, 2018, 8:19 a.m. UTC
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(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Zeng, Star March 30, 2018, 1:59 a.m. UTC | #1
Reviewed-by: Star Zeng <star.zeng@intel.com>


Thanks,
Star
-----Original Message-----
From: Heyi Guo [mailto:heyi.guo@linaro.org] 

Sent: Thursday, March 29, 2018 4:20 PM
To: edk2-devel@lists.01.org
Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li <phoenix.liyi@huawei.com>; Renhao Liang <liangrenhao@huawei.com>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

For gDS->SetMemorySpaceAttributes(), when user passes a combined memory attribute including CPU arch attribute and other attributes, like EFI_MEMORY_RUNTIME, ConverToCpuArchAttributes() will return INVALID_CPU_ARCH_ATTRIBUTES and skip setting page/cache attribute for the specified memory space.

We don't see any reason to forbid combining CPU arch attributes and non-CPU-arch attributes when calling gDS->SetMemorySpaceAttributes(), so we remove the check code in ConverToCpuArchAttributes(); the remaining code is enough to grab the interested bits for
Cpu->SetMemoryAttributes().

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

Signed-off-by: Renhao Liang <liangrenhao@huawei.com>

Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index 77f4adb4bc01..907245a3f512 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {
   UINT64      CpuArchAttributes;
 
-  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
-                      NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
-    return INVALID_CPU_ARCH_ATTRIBUTES;
-  }
-
   CpuArchAttributes = Attributes & NONEXCLUSIVE_MEMORY_ATTRIBUTES;
 
   if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
--
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Zeng, Star April 2, 2018, 5:58 a.m. UTC | #2
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
Kinney, Michael D April 2, 2018, 9:43 p.m. UTC | #3
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
Zeng, Star April 3, 2018, 12:59 a.m. UTC | #4
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
Zeng, Star April 3, 2018, 1:15 a.m. UTC | #5
Current gCpu->SetMemoryAttributes does not support getting memory attributes, and has no clear description about clearing memory attributes.

I noticed we introduced SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribute.h) protocol for heap guard feature in SMM environment.
Seemingly, we also need introduce MemoryAttribute protocol for DXE?


Thanks,
Star
-----Original Message-----
From: Zeng, Star 

Sent: Tuesday, April 3, 2018 8:59 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org
Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

Mike,

Agree the problem.
We need support only RUNTIME attribute.
We need support only cache attributes.
We need support only page attributes.
We need support RUNTIME + cache + page attributes.
Do we need support the 0 Attributes case to clear page attributes?
There was discussion about the 0 Attributes case at https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.
It came to the question that should the 0 Attributes case be handled by SetMemoryAttributes() to clear the page attributes?


Thanks,
Star
-----Original Message-----
From: Kinney, Michael D

Sent: Tuesday, April 3, 2018 5:43 AM
To: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

Star,

This commit breaks Vlv2TbltDevicePkg.

On this platform, there are 2 calls to 
gDS->SetMemorySpaceAttributes() for an MMIO
range to set only the EFI_MEMORY_RUNTIME bit.

With this commit, ConverToCpuArchAttributes()returns 0, and 0 is passed to gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on Vlv2TbltDevicePkg.

Before this commit, ConverToCpuArchAttributes() returns INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu->SetMemoryAttributes()to be skipped so no error is generated.

I think the right fix is to skip the call to 
gCpu->SetMemoryAttributes() if CpuArchAttributes
is 0 so a call that only sets the RUNTIME attribute can be supported along with call the set both the RUNTIME bit and other cache related bits.

I will send a patch soon with the proposed fix.

Mike

> -----Original Message-----

> From: Zeng, Star

> Sent: Sunday, April 1, 2018 10:59 PM

> To: Heyi Guo <heyi.guo@linaro.org>; edk2- devel@lists.01.org

> Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Kinney, 

> Michael D <michael.d.kinney@intel.com>; Gao, Liming 

> <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, 

> Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> conversion

> 

> Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> 

> Thanks,

> Star

> -----Original Message-----

> From: Heyi Guo [mailto:heyi.guo@linaro.org]

> Sent: Thursday, March 29, 2018 4:20 PM

> To: edk2-devel@lists.01.org

> Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li <phoenix.liyi@huawei.com>; 

> Renhao Liang <liangrenhao@huawei.com>; Zeng, Star 

> <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, 

> Michael D <michael.d.kinney@intel.com>; Gao, Liming 

> <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, 

> Ruiyu <ruiyu.ni@intel.com>

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

> 

> For gDS->SetMemorySpaceAttributes(), when user passes a combined 

> memory attribute including CPU arch attribute and other attributes, 

> like EFI_MEMORY_RUNTIME,

> ConverToCpuArchAttributes() will return INVALID_CPU_ARCH_ATTRIBUTES 

> and skip setting page/cache attribute for the specified memory space.

> 

> We don't see any reason to forbid combining CPU arch attributes and 

> non-CPU-arch attributes when calling gDS-

> >SetMemorySpaceAttributes(), so we remove the check code

> in ConverToCpuArchAttributes(); the remaining code is enough to grab 

> the interested bits for

> Cpu->SetMemoryAttributes().

> 

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> Signed-off-by: Renhao Liang <liangrenhao@huawei.com>

> Cc: Star Zeng <star.zeng@intel.com>

> Cc: Eric Dong <eric.dong@intel.com>

> Cc: Michael D Kinney <michael.d.kinney@intel.com>

> Cc: Liming Gao <liming.gao@intel.com>

> Cc: Jian J Wang <jian.j.wang@intel.com>

> Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> ---

>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

>  1 file changed, 5 deletions(-)

> 

> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c 

> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> 77f4adb4bc01..907245a3f512 100644

> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {

>    UINT64      CpuArchAttributes;

> 

> -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> != 0) {

> -    return INVALID_CPU_ARCH_ATTRIBUTES;

> -  }

> -

>    CpuArchAttributes = Attributes &

> NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> 

>    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {

> --

> 2.7.4


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Wang, Jian J April 3, 2018, 1:24 a.m. UTC | #6
The NX Memory Protection and Heap Guard features need to clear paging attributes.
Allowing 0 attribute is the current only choice without changing arch protocol. Maybe
It's time to introduce a new protocol.

Regards,
Jian


> -----Original Message-----

> From: Zeng, Star

> Sent: Tuesday, April 03, 2018 9:16 AM

> To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming

> <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> 

> Current gCpu->SetMemoryAttributes does not support getting memory

> attributes, and has no clear description about clearing memory attributes.

> 

> I noticed we introduced

> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribute.

> h) protocol for heap guard feature in SMM environment.

> Seemingly, we also need introduce MemoryAttribute protocol for DXE?

> 

> 

> Thanks,

> Star

> -----Original Message-----

> From: Zeng, Star

> Sent: Tuesday, April 3, 2018 8:59 AM

> To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming

> <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> 

> Mike,

> 

> Agree the problem.

> We need support only RUNTIME attribute.

> We need support only cache attributes.

> We need support only page attributes.

> We need support RUNTIME + cache + page attributes.

> Do we need support the 0 Attributes case to clear page attributes?

> There was discussion about the 0 Attributes case at

> https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.

> It came to the question that should the 0 Attributes case be handled by

> SetMemoryAttributes() to clear the page attributes?

> 

> 

> Thanks,

> Star

> -----Original Message-----

> From: Kinney, Michael D

> Sent: Tuesday, April 3, 2018 5:43 AM

> To: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-

> devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>

> Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming

> <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> <ruiyu.ni@intel.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> 

> Star,

> 

> This commit breaks Vlv2TbltDevicePkg.

> 

> On this platform, there are 2 calls to

> gDS->SetMemorySpaceAttributes() for an MMIO

> range to set only the EFI_MEMORY_RUNTIME bit.

> 

> With this commit, ConverToCpuArchAttributes()returns 0, and 0 is passed to

> gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on

> Vlv2TbltDevicePkg.

> 

> Before this commit, ConverToCpuArchAttributes() returns

> INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu-

> >SetMemoryAttributes()to be skipped so no error is generated.

> 

> I think the right fix is to skip the call to

> gCpu->SetMemoryAttributes() if CpuArchAttributes

> is 0 so a call that only sets the RUNTIME attribute can be supported along with

> call the set both the RUNTIME bit and other cache related bits.

> 

> I will send a patch soon with the proposed fix.

> 

> Mike

> 

> > -----Original Message-----

> > From: Zeng, Star

> > Sent: Sunday, April 1, 2018 10:59 PM

> > To: Heyi Guo <heyi.guo@linaro.org>; edk2- devel@lists.01.org

> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Kinney,

> > Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni,

> > Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > conversion

> >

> > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> >

> > Thanks,

> > Star

> > -----Original Message-----

> > From: Heyi Guo [mailto:heyi.guo@linaro.org]

> > Sent: Thursday, March 29, 2018 4:20 PM

> > To: edk2-devel@lists.01.org

> > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li <phoenix.liyi@huawei.com>;

> > Renhao Liang <liangrenhao@huawei.com>; Zeng, Star

> > <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney,

> > Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni,

> > Ruiyu <ruiyu.ni@intel.com>

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

> >

> > For gDS->SetMemorySpaceAttributes(), when user passes a combined

> > memory attribute including CPU arch attribute and other attributes,

> > like EFI_MEMORY_RUNTIME,

> > ConverToCpuArchAttributes() will return INVALID_CPU_ARCH_ATTRIBUTES

> > and skip setting page/cache attribute for the specified memory space.

> >

> > We don't see any reason to forbid combining CPU arch attributes and

> > non-CPU-arch attributes when calling gDS-

> > >SetMemorySpaceAttributes(), so we remove the check code

> > in ConverToCpuArchAttributes(); the remaining code is enough to grab

> > the interested bits for

> > Cpu->SetMemoryAttributes().

> >

> > Contributed-under: TianoCore Contribution Agreement 1.1

> > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > Signed-off-by: Renhao Liang <liangrenhao@huawei.com>

> > Cc: Star Zeng <star.zeng@intel.com>

> > Cc: Eric Dong <eric.dong@intel.com>

> > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > Cc: Liming Gao <liming.gao@intel.com>

> > Cc: Jian J Wang <jian.j.wang@intel.com>

> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > ---

> >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

> >  1 file changed, 5 deletions(-)

> >

> > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> > 77f4adb4bc01..907245a3f512 100644

> > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {

> >    UINT64      CpuArchAttributes;

> >

> > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> > != 0) {

> > -    return INVALID_CPU_ARCH_ATTRIBUTES;

> > -  }

> > -

> >    CpuArchAttributes = Attributes &

> > NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> >

> >    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {

> > --

> > 2.7.4


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Yao, Jiewen April 3, 2018, 2:25 a.m. UTC | #7
I have a discussion with Star/Jian.

The expectation for the CPU driver is to handle PageAttribute.
The expectation for the platform driver is to use GetAttribute(), AND/OR attribute, then call SetAttribute().
Because the DRAM always has a cache attribute (no matter UC or WB), 0 should not happen. (0 might happen for a GCD reserved memory, but there is no need to set page attribute)

If all drivers use above way, there won't be any issue. There is no need to introduce a new protocol so far.

We did encounter some error, because the platform/silicon/CPU code is not updated yet. (For example, the MinnowMax which is using binary)
The fix to filter 0 seems a workable way to resolve the compatibility issue.

Later, I suggest we update MinnowMax binary - Add paging handling for CPU driver, and use GetAttribute()/SetAttribute() for platform/silicon code.


Thank you
Yao Jiewen

> -----Original Message-----

> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,

> Jian J

> Sent: Tuesday, April 3, 2018 9:24 AM

> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D

> <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>;

> edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao,

> Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao

> Liang <liangrenhao@huawei.com>

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

> conversion

> 

> The NX Memory Protection and Heap Guard features need to clear paging

> attributes.

> Allowing 0 attribute is the current only choice without changing arch protocol.

> Maybe

> It's time to introduce a new protocol.

> 

> Regards,

> Jian

> 

> 

> > -----Original Message-----

> > From: Zeng, Star

> > Sent: Tuesday, April 03, 2018 9:16 AM

> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming

> > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> > <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> >

> > Current gCpu->SetMemoryAttributes does not support getting memory

> > attributes, and has no clear description about clearing memory attributes.

> >

> > I noticed we introduced

> >

> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribut

> e.

> > h) protocol for heap guard feature in SMM environment.

> > Seemingly, we also need introduce MemoryAttribute protocol for DXE?

> >

> >

> > Thanks,

> > Star

> > -----Original Message-----

> > From: Zeng, Star

> > Sent: Tuesday, April 3, 2018 8:59 AM

> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming

> > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> > <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> >

> > Mike,

> >

> > Agree the problem.

> > We need support only RUNTIME attribute.

> > We need support only cache attributes.

> > We need support only page attributes.

> > We need support RUNTIME + cache + page attributes.

> > Do we need support the 0 Attributes case to clear page attributes?

> > There was discussion about the 0 Attributes case at

> > https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.

> > It came to the question that should the 0 Attributes case be handled by

> > SetMemoryAttributes() to clear the page attributes?

> >

> >

> > Thanks,

> > Star

> > -----Original Message-----

> > From: Kinney, Michael D

> > Sent: Tuesday, April 3, 2018 5:43 AM

> > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-

> > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>

> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming

> > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> > <ruiyu.ni@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> >

> > Star,

> >

> > This commit breaks Vlv2TbltDevicePkg.

> >

> > On this platform, there are 2 calls to

> > gDS->SetMemorySpaceAttributes() for an MMIO

> > range to set only the EFI_MEMORY_RUNTIME bit.

> >

> > With this commit, ConverToCpuArchAttributes()returns 0, and 0 is passed to

> > gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on

> > Vlv2TbltDevicePkg.

> >

> > Before this commit, ConverToCpuArchAttributes() returns

> > INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu-

> > >SetMemoryAttributes()to be skipped so no error is generated.

> >

> > I think the right fix is to skip the call to

> > gCpu->SetMemoryAttributes() if CpuArchAttributes

> > is 0 so a call that only sets the RUNTIME attribute can be supported along with

> > call the set both the RUNTIME bit and other cache related bits.

> >

> > I will send a patch soon with the proposed fix.

> >

> > Mike

> >

> > > -----Original Message-----

> > > From: Zeng, Star

> > > Sent: Sunday, April 1, 2018 10:59 PM

> > > To: Heyi Guo <heyi.guo@linaro.org>; edk2- devel@lists.01.org

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Kinney,

> > > Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni,

> > > Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > conversion

> > >

> > > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> > >

> > > Thanks,

> > > Star

> > > -----Original Message-----

> > > From: Heyi Guo [mailto:heyi.guo@linaro.org]

> > > Sent: Thursday, March 29, 2018 4:20 PM

> > > To: edk2-devel@lists.01.org

> > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li <phoenix.liyi@huawei.com>;

> > > Renhao Liang <liangrenhao@huawei.com>; Zeng, Star

> > > <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney,

> > > Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni,

> > > Ruiyu <ruiyu.ni@intel.com>

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

> > >

> > > For gDS->SetMemorySpaceAttributes(), when user passes a combined

> > > memory attribute including CPU arch attribute and other attributes,

> > > like EFI_MEMORY_RUNTIME,

> > > ConverToCpuArchAttributes() will return INVALID_CPU_ARCH_ATTRIBUTES

> > > and skip setting page/cache attribute for the specified memory space.

> > >

> > > We don't see any reason to forbid combining CPU arch attributes and

> > > non-CPU-arch attributes when calling gDS-

> > > >SetMemorySpaceAttributes(), so we remove the check code

> > > in ConverToCpuArchAttributes(); the remaining code is enough to grab

> > > the interested bits for

> > > Cpu->SetMemoryAttributes().

> > >

> > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > > Signed-off-by: Renhao Liang <liangrenhao@huawei.com>

> > > Cc: Star Zeng <star.zeng@intel.com>

> > > Cc: Eric Dong <eric.dong@intel.com>

> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > > Cc: Liming Gao <liming.gao@intel.com>

> > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > > ---

> > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

> > >  1 file changed, 5 deletions(-)

> > >

> > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> > > 77f4adb4bc01..907245a3f512 100644

> > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {

> > >    UINT64      CpuArchAttributes;

> > >

> > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> > > != 0) {

> > > -    return INVALID_CPU_ARCH_ATTRIBUTES;

> > > -  }

> > > -

> > >    CpuArchAttributes = Attributes &

> > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> > >

> > >    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {

> > > --

> > > 2.7.4

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Zeng, Star April 3, 2018, 2:33 a.m. UTC | #8
Agree.

And I agree we need the fix at https://lists.01.org/pipermail/edk2-devel/2018-April/023364.html from Mike for compatibility.
And I think more precious and precise information need to be added into the commit log or code comments since we have been crazy about handling attributes in the GCD service. :)


Thanks,
Star
-----Original Message-----
From: Yao, Jiewen 

Sent: Tuesday, April 3, 2018 10:26 AM
To: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao Liang <liangrenhao@huawei.com>
Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

I have a discussion with Star/Jian.

The expectation for the CPU driver is to handle PageAttribute.
The expectation for the platform driver is to use GetAttribute(), AND/OR attribute, then call SetAttribute().
Because the DRAM always has a cache attribute (no matter UC or WB), 0 should not happen. (0 might happen for a GCD reserved memory, but there is no need to set page attribute)

If all drivers use above way, there won't be any issue. There is no need to introduce a new protocol so far.

We did encounter some error, because the platform/silicon/CPU code is not updated yet. (For example, the MinnowMax which is using binary) The fix to filter 0 seems a workable way to resolve the compatibility issue.

Later, I suggest we update MinnowMax binary - Add paging handling for CPU driver, and use GetAttribute()/SetAttribute() for platform/silicon code.


Thank you
Yao Jiewen

> -----Original Message-----

> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 

> Wang, Jian J

> Sent: Tuesday, April 3, 2018 9:24 AM

> To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D 

> <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>; 

> edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; 

> Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; 

> Renhao Liang <liangrenhao@huawei.com>

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

> conversion

> 

> The NX Memory Protection and Heap Guard features need to clear paging 

> attributes.

> Allowing 0 attribute is the current only choice without changing arch protocol.

> Maybe

> It's time to introduce a new protocol.

> 

> Regards,

> Jian

> 

> 

> > -----Original Message-----

> > From: Zeng, Star

> > Sent: Tuesday, April 03, 2018 9:16 AM

> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo 

> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, 

> > Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; 

> > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > conversion

> >

> > Current gCpu->SetMemoryAttributes does not support getting memory 

> > attributes, and has no clear description about clearing memory attributes.

> >

> > I noticed we introduced

> >

> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribut

> e.

> > h) protocol for heap guard feature in SMM environment.

> > Seemingly, we also need introduce MemoryAttribute protocol for DXE?

> >

> >

> > Thanks,

> > Star

> > -----Original Message-----

> > From: Zeng, Star

> > Sent: Tuesday, April 3, 2018 8:59 AM

> > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo 

> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, 

> > Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; 

> > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > conversion

> >

> > Mike,

> >

> > Agree the problem.

> > We need support only RUNTIME attribute.

> > We need support only cache attributes.

> > We need support only page attributes.

> > We need support RUNTIME + cache + page attributes.

> > Do we need support the 0 Attributes case to clear page attributes?

> > There was discussion about the 0 Attributes case at 

> > https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.

> > It came to the question that should the 0 Attributes case be handled 

> > by

> > SetMemoryAttributes() to clear the page attributes?

> >

> >

> > Thanks,

> > Star

> > -----Original Message-----

> > From: Kinney, Michael D

> > Sent: Tuesday, April 3, 2018 5:43 AM

> > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo 

> > <heyi.guo@linaro.org>; edk2- devel@lists.01.org; Kinney, Michael D 

> > <michael.d.kinney@intel.com>

> > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, 

> > Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; 

> > Ni, Ruiyu <ruiyu.ni@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > conversion

> >

> > Star,

> >

> > This commit breaks Vlv2TbltDevicePkg.

> >

> > On this platform, there are 2 calls to

> > gDS->SetMemorySpaceAttributes() for an MMIO

> > range to set only the EFI_MEMORY_RUNTIME bit.

> >

> > With this commit, ConverToCpuArchAttributes()returns 0, and 0 is 

> > passed to

> > gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on

> > Vlv2TbltDevicePkg.

> >

> > Before this commit, ConverToCpuArchAttributes() returns 

> > INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu-

> > >SetMemoryAttributes()to be skipped so no error is generated.

> >

> > I think the right fix is to skip the call to

> > gCpu->SetMemoryAttributes() if CpuArchAttributes

> > is 0 so a call that only sets the RUNTIME attribute can be supported 

> > along with call the set both the RUNTIME bit and other cache related bits.

> >

> > I will send a patch soon with the proposed fix.

> >

> > Mike

> >

> > > -----Original Message-----

> > > From: Zeng, Star

> > > Sent: Sunday, April 1, 2018 10:59 PM

> > > To: Heyi Guo <heyi.guo@linaro.org>; edk2- devel@lists.01.org

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; 

> > > Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming 

> > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, 

> > > Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > > conversion

> > >

> > > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> > >

> > > Thanks,

> > > Star

> > > -----Original Message-----

> > > From: Heyi Guo [mailto:heyi.guo@linaro.org]

> > > Sent: Thursday, March 29, 2018 4:20 PM

> > > To: edk2-devel@lists.01.org

> > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li 

> > > <phoenix.liyi@huawei.com>; Renhao Liang <liangrenhao@huawei.com>; 

> > > Zeng, Star <star.zeng@intel.com>; Dong, Eric 

> > > <eric.dong@intel.com>; Kinney, Michael D 

> > > <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; 

> > > Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu 

> > > <ruiyu.ni@intel.com>

> > > Subject: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > > conversion

> > >

> > > For gDS->SetMemorySpaceAttributes(), when user passes a combined 

> > > memory attribute including CPU arch attribute and other 

> > > attributes, like EFI_MEMORY_RUNTIME,

> > > ConverToCpuArchAttributes() will return 

> > > INVALID_CPU_ARCH_ATTRIBUTES and skip setting page/cache attribute for the specified memory space.

> > >

> > > We don't see any reason to forbid combining CPU arch attributes 

> > > and non-CPU-arch attributes when calling gDS-

> > > >SetMemorySpaceAttributes(), so we remove the check code

> > > in ConverToCpuArchAttributes(); the remaining code is enough to 

> > > grab the interested bits for

> > > Cpu->SetMemoryAttributes().

> > >

> > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > > Signed-off-by: Renhao Liang <liangrenhao@huawei.com>

> > > Cc: Star Zeng <star.zeng@intel.com>

> > > Cc: Eric Dong <eric.dong@intel.com>

> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > > Cc: Liming Gao <liming.gao@intel.com>

> > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > > ---

> > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

> > >  1 file changed, 5 deletions(-)

> > >

> > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c 

> > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> > > 77f4adb4bc01..907245a3f512 100644

> > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {

> > >    UINT64      CpuArchAttributes;

> > >

> > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> > > != 0) {

> > > -    return INVALID_CPU_ARCH_ATTRIBUTES;

> > > -  }

> > > -

> > >    CpuArchAttributes = Attributes & 

> > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> > >

> > >    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {

> > > --

> > > 2.7.4

> 

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Yao, Jiewen April 3, 2018, 2:34 a.m. UTC | #9
That is good idea.

Just add more description as the code comment, please.

It is easy for code review later.

Thank you
Yao Jiewen


> -----Original Message-----

> From: Zeng, Star

> Sent: Tuesday, April 3, 2018 10:33 AM

> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;

> Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao,

> Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao

> Liang <liangrenhao@huawei.com>; Zeng, Star <star.zeng@intel.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> 

> Agree.

> 

> And I agree we need the fix at

> https://lists.01.org/pipermail/edk2-devel/2018-April/023364.html from Mike

> for compatibility.

> And I think more precious and precise information need to be added into the

> commit log or code comments since we have been crazy about handling

> attributes in the GCD service. :)

> 

> 

> Thanks,

> Star

> -----Original Message-----

> From: Yao, Jiewen

> Sent: Tuesday, April 3, 2018 10:26 AM

> To: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>;

> Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao,

> Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao

> Liang <liangrenhao@huawei.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> 

> I have a discussion with Star/Jian.

> 

> The expectation for the CPU driver is to handle PageAttribute.

> The expectation for the platform driver is to use GetAttribute(), AND/OR

> attribute, then call SetAttribute().

> Because the DRAM always has a cache attribute (no matter UC or WB), 0 should

> not happen. (0 might happen for a GCD reserved memory, but there is no need

> to set page attribute)

> 

> If all drivers use above way, there won't be any issue. There is no need to

> introduce a new protocol so far.

> 

> We did encounter some error, because the platform/silicon/CPU code is not

> updated yet. (For example, the MinnowMax which is using binary) The fix to

> filter 0 seems a workable way to resolve the compatibility issue.

> 

> Later, I suggest we update MinnowMax binary - Add paging handling for CPU

> driver, and use GetAttribute()/SetAttribute() for platform/silicon code.

> 

> 

> Thank you

> Yao Jiewen

> 

> > -----Original Message-----

> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of

> > Wang, Jian J

> > Sent: Tuesday, April 3, 2018 9:24 AM

> > To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D

> > <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>;

> > edk2-devel@lists.01.org

> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>;

> > Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;

> > Renhao Liang <liangrenhao@huawei.com>

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

> > conversion

> >

> > The NX Memory Protection and Heap Guard features need to clear paging

> > attributes.

> > Allowing 0 attribute is the current only choice without changing arch protocol.

> > Maybe

> > It's time to introduce a new protocol.

> >

> > Regards,

> > Jian

> >

> >

> > > -----Original Message-----

> > > From: Zeng, Star

> > > Sent: Tuesday, April 03, 2018 9:16 AM

> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao,

> > > Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;

> > > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > conversion

> > >

> > > Current gCpu->SetMemoryAttributes does not support getting memory

> > > attributes, and has no clear description about clearing memory attributes.

> > >

> > > I noticed we introduced

> > >

> >

> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribut

> > e.

> > > h) protocol for heap guard feature in SMM environment.

> > > Seemingly, we also need introduce MemoryAttribute protocol for DXE?

> > >

> > >

> > > Thanks,

> > > Star

> > > -----Original Message-----

> > > From: Zeng, Star

> > > Sent: Tuesday, April 3, 2018 8:59 AM

> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao,

> > > Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;

> > > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > conversion

> > >

> > > Mike,

> > >

> > > Agree the problem.

> > > We need support only RUNTIME attribute.

> > > We need support only cache attributes.

> > > We need support only page attributes.

> > > We need support RUNTIME + cache + page attributes.

> > > Do we need support the 0 Attributes case to clear page attributes?

> > > There was discussion about the 0 Attributes case at

> > > https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.

> > > It came to the question that should the 0 Attributes case be handled

> > > by

> > > SetMemoryAttributes() to clear the page attributes?

> > >

> > >

> > > Thanks,

> > > Star

> > > -----Original Message-----

> > > From: Kinney, Michael D

> > > Sent: Tuesday, April 3, 2018 5:43 AM

> > > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo

> > > <heyi.guo@linaro.org>; edk2- devel@lists.01.org; Kinney, Michael D

> > > <michael.d.kinney@intel.com>

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao,

> > > Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;

> > > Ni, Ruiyu <ruiyu.ni@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > conversion

> > >

> > > Star,

> > >

> > > This commit breaks Vlv2TbltDevicePkg.

> > >

> > > On this platform, there are 2 calls to

> > > gDS->SetMemorySpaceAttributes() for an MMIO

> > > range to set only the EFI_MEMORY_RUNTIME bit.

> > >

> > > With this commit, ConverToCpuArchAttributes()returns 0, and 0 is

> > > passed to

> > > gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on

> > > Vlv2TbltDevicePkg.

> > >

> > > Before this commit, ConverToCpuArchAttributes() returns

> > > INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu-

> > > >SetMemoryAttributes()to be skipped so no error is generated.

> > >

> > > I think the right fix is to skip the call to

> > > gCpu->SetMemoryAttributes() if CpuArchAttributes

> > > is 0 so a call that only sets the RUNTIME attribute can be supported

> > > along with call the set both the RUNTIME bit and other cache related bits.

> > >

> > > I will send a patch soon with the proposed fix.

> > >

> > > Mike

> > >

> > > > -----Original Message-----

> > > > From: Zeng, Star

> > > > Sent: Sunday, April 1, 2018 10:59 PM

> > > > To: Heyi Guo <heyi.guo@linaro.org>; edk2- devel@lists.01.org

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>;

> > > > Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni,

> > > > Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > > conversion

> > > >

> > > > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> > > >

> > > > Thanks,

> > > > Star

> > > > -----Original Message-----

> > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]

> > > > Sent: Thursday, March 29, 2018 4:20 PM

> > > > To: edk2-devel@lists.01.org

> > > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li

> > > > <phoenix.liyi@huawei.com>; Renhao Liang <liangrenhao@huawei.com>;

> > > > Zeng, Star <star.zeng@intel.com>; Dong, Eric

> > > > <eric.dong@intel.com>; Kinney, Michael D

> > > > <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>;

> > > > Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> > > > <ruiyu.ni@intel.com>

> > > > Subject: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > > conversion

> > > >

> > > > For gDS->SetMemorySpaceAttributes(), when user passes a combined

> > > > memory attribute including CPU arch attribute and other

> > > > attributes, like EFI_MEMORY_RUNTIME,

> > > > ConverToCpuArchAttributes() will return

> > > > INVALID_CPU_ARCH_ATTRIBUTES and skip setting page/cache attribute for

> the specified memory space.

> > > >

> > > > We don't see any reason to forbid combining CPU arch attributes

> > > > and non-CPU-arch attributes when calling gDS-

> > > > >SetMemorySpaceAttributes(), so we remove the check code

> > > > in ConverToCpuArchAttributes(); the remaining code is enough to

> > > > grab the interested bits for

> > > > Cpu->SetMemoryAttributes().

> > > >

> > > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > > > Signed-off-by: Renhao Liang <liangrenhao@huawei.com>

> > > > Cc: Star Zeng <star.zeng@intel.com>

> > > > Cc: Eric Dong <eric.dong@intel.com>

> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > > > Cc: Liming Gao <liming.gao@intel.com>

> > > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > > > ---

> > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

> > > >  1 file changed, 5 deletions(-)

> > > >

> > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> > > > 77f4adb4bc01..907245a3f512 100644

> > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {

> > > >    UINT64      CpuArchAttributes;

> > > >

> > > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> > > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> > > > != 0) {

> > > > -    return INVALID_CPU_ARCH_ATTRIBUTES;

> > > > -  }

> > > > -

> > > >    CpuArchAttributes = Attributes &

> > > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> > > >

> > > >    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {

> > > > --

> > > > 2.7.4

> >

> > _______________________________________________

> > edk2-devel mailing list

> > edk2-devel@lists.01.org

> > https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Zeng, Star April 3, 2018, 2:41 a.m. UTC | #10
Just thought more about the case.

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


Thanks,
Star
-----Original Message-----
From: Yao, Jiewen 

Sent: Tuesday, April 3, 2018 10:34 AM
To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao Liang <liangrenhao@huawei.com>
Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

That is good idea.

Just add more description as the code comment, please.

It is easy for code review later.

Thank you
Yao Jiewen


> -----Original Message-----

> From: Zeng, Star

> Sent: Tuesday, April 3, 2018 10:33 AM

> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J 

> <jian.j.wang@intel.com>; Kinney, Michael D 

> <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>; 

> edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; 

> Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; 

> Renhao Liang <liangrenhao@huawei.com>; Zeng, Star 

> <star.zeng@intel.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> conversion

> 

> Agree.

> 

> And I agree we need the fix at

> https://lists.01.org/pipermail/edk2-devel/2018-April/023364.html from 

> Mike for compatibility.

> And I think more precious and precise information need to be added 

> into the commit log or code comments since we have been crazy about 

> handling attributes in the GCD service. :)

> 

> 

> Thanks,

> Star

> -----Original Message-----

> From: Yao, Jiewen

> Sent: Tuesday, April 3, 2018 10:26 AM

> To: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star 

> <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; 

> Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; 

> Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; 

> Renhao Liang <liangrenhao@huawei.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> conversion

> 

> I have a discussion with Star/Jian.

> 

> The expectation for the CPU driver is to handle PageAttribute.

> The expectation for the platform driver is to use GetAttribute(), 

> AND/OR attribute, then call SetAttribute().

> Because the DRAM always has a cache attribute (no matter UC or WB), 0 

> should not happen. (0 might happen for a GCD reserved memory, but 

> there is no need to set page attribute)

> 

> If all drivers use above way, there won't be any issue. There is no 

> need to introduce a new protocol so far.

> 

> We did encounter some error, because the platform/silicon/CPU code is 

> not updated yet. (For example, the MinnowMax which is using binary) 

> The fix to filter 0 seems a workable way to resolve the compatibility issue.

> 

> Later, I suggest we update MinnowMax binary - Add paging handling for 

> CPU driver, and use GetAttribute()/SetAttribute() for platform/silicon code.

> 

> 

> Thank you

> Yao Jiewen

> 

> > -----Original Message-----

> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf 

> > Of Wang, Jian J

> > Sent: Tuesday, April 3, 2018 9:24 AM

> > To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D 

> > <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>; 

> > edk2-devel@lists.01.org

> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; 

> > Gao, Liming <liming.gao@intel.com>; Dong, Eric 

> > <eric.dong@intel.com>; Renhao Liang <liangrenhao@huawei.com>

> > Subject: Re: [edk2] [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of 

> > attribute conversion

> >

> > The NX Memory Protection and Heap Guard features need to clear 

> > paging attributes.

> > Allowing 0 attribute is the current only choice without changing arch protocol.

> > Maybe

> > It's time to introduce a new protocol.

> >

> > Regards,

> > Jian

> >

> >

> > > -----Original Message-----

> > > From: Zeng, Star

> > > Sent: Tuesday, April 03, 2018 9:16 AM

> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo 

> > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, 

> > > Liming <liming.gao@intel.com>; Wang, Jian J 

> > > <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, 

> > > Star <star.zeng@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > > conversion

> > >

> > > Current gCpu->SetMemoryAttributes does not support getting memory 

> > > attributes, and has no clear description about clearing memory attributes.

> > >

> > > I noticed we introduced

> > >

> >

> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribut

> > e.

> > > h) protocol for heap guard feature in SMM environment.

> > > Seemingly, we also need introduce MemoryAttribute protocol for DXE?

> > >

> > >

> > > Thanks,

> > > Star

> > > -----Original Message-----

> > > From: Zeng, Star

> > > Sent: Tuesday, April 3, 2018 8:59 AM

> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo 

> > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, 

> > > Liming <liming.gao@intel.com>; Wang, Jian J 

> > > <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, 

> > > Star <star.zeng@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > > conversion

> > >

> > > Mike,

> > >

> > > Agree the problem.

> > > We need support only RUNTIME attribute.

> > > We need support only cache attributes.

> > > We need support only page attributes.

> > > We need support RUNTIME + cache + page attributes.

> > > Do we need support the 0 Attributes case to clear page attributes?

> > > There was discussion about the 0 Attributes case at 

> > > https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.

> > > It came to the question that should the 0 Attributes case be 

> > > handled by

> > > SetMemoryAttributes() to clear the page attributes?

> > >

> > >

> > > Thanks,

> > > Star

> > > -----Original Message-----

> > > From: Kinney, Michael D

> > > Sent: Tuesday, April 3, 2018 5:43 AM

> > > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo 

> > > <heyi.guo@linaro.org>; edk2- devel@lists.01.org; Kinney, Michael D 

> > > <michael.d.kinney@intel.com>

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, 

> > > Liming <liming.gao@intel.com>; Wang, Jian J 

> > > <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > > conversion

> > >

> > > Star,

> > >

> > > This commit breaks Vlv2TbltDevicePkg.

> > >

> > > On this platform, there are 2 calls to

> > > gDS->SetMemorySpaceAttributes() for an MMIO

> > > range to set only the EFI_MEMORY_RUNTIME bit.

> > >

> > > With this commit, ConverToCpuArchAttributes()returns 0, and 0 is 

> > > passed to

> > > gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on

> > > Vlv2TbltDevicePkg.

> > >

> > > Before this commit, ConverToCpuArchAttributes() returns 

> > > INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu-

> > > >SetMemoryAttributes()to be skipped so no error is generated.

> > >

> > > I think the right fix is to skip the call to

> > > gCpu->SetMemoryAttributes() if CpuArchAttributes

> > > is 0 so a call that only sets the RUNTIME attribute can be 

> > > supported along with call the set both the RUNTIME bit and other cache related bits.

> > >

> > > I will send a patch soon with the proposed fix.

> > >

> > > Mike

> > >

> > > > -----Original Message-----

> > > > From: Zeng, Star

> > > > Sent: Sunday, April 1, 2018 10:59 PM

> > > > To: Heyi Guo <heyi.guo@linaro.org>; edk2- devel@lists.01.org

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; 

> > > > Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming 

> > > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; 

> > > > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > > > conversion

> > > >

> > > > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> > > >

> > > > Thanks,

> > > > Star

> > > > -----Original Message-----

> > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]

> > > > Sent: Thursday, March 29, 2018 4:20 PM

> > > > To: edk2-devel@lists.01.org

> > > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li 

> > > > <phoenix.liyi@huawei.com>; Renhao Liang 

> > > > <liangrenhao@huawei.com>; Zeng, Star <star.zeng@intel.com>; 

> > > > Dong, Eric <eric.dong@intel.com>; Kinney, Michael D 

> > > > <michael.d.kinney@intel.com>; Gao, Liming 

> > > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; 

> > > > Ni, Ruiyu <ruiyu.ni@intel.com>

> > > > Subject: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> > > > conversion

> > > >

> > > > For gDS->SetMemorySpaceAttributes(), when user passes a combined 

> > > > memory attribute including CPU arch attribute and other 

> > > > attributes, like EFI_MEMORY_RUNTIME,

> > > > ConverToCpuArchAttributes() will return 

> > > > INVALID_CPU_ARCH_ATTRIBUTES and skip setting page/cache 

> > > > attribute for

> the specified memory space.

> > > >

> > > > We don't see any reason to forbid combining CPU arch attributes 

> > > > and non-CPU-arch attributes when calling gDS-

> > > > >SetMemorySpaceAttributes(), so we remove the check code

> > > > in ConverToCpuArchAttributes(); the remaining code is enough to 

> > > > grab the interested bits for

> > > > Cpu->SetMemoryAttributes().

> > > >

> > > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > > > Signed-off-by: Renhao Liang <liangrenhao@huawei.com>

> > > > Cc: Star Zeng <star.zeng@intel.com>

> > > > Cc: Eric Dong <eric.dong@intel.com>

> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > > > Cc: Liming Gao <liming.gao@intel.com>

> > > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > > > ---

> > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

> > > >  1 file changed, 5 deletions(-)

> > > >

> > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c 

> > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> > > > 77f4adb4bc01..907245a3f512 100644

> > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {

> > > >    UINT64      CpuArchAttributes;

> > > >

> > > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> > > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> > > > != 0) {

> > > > -    return INVALID_CPU_ARCH_ATTRIBUTES;

> > > > -  }

> > > > -

> > > >    CpuArchAttributes = Attributes & 

> > > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> > > >

> > > >    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {

> > > > --

> > > > 2.7.4

> >

> > _______________________________________________

> > edk2-devel mailing list

> > edk2-devel@lists.01.org

> > https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Yao, Jiewen April 3, 2018, 6:06 a.m. UTC | #11
Right.
I think we only support clear the page attribute, and we don't support the cache attribute.
Using OR for RUNTIME is good.

Thank you
Yao Jiewen

> -----Original Message-----

> From: Zeng, Star

> Sent: Tuesday, April 3, 2018 10:41 AM

> To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;

> Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao,

> Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao

> Liang <liangrenhao@huawei.com>; Zeng, Star <star.zeng@intel.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> 

> Just thought more about the case.

> 

> If a memory range has WB attribute, and code is to set RUNTIME attribute, then

> the WB attribute will be clear in GCD database, it seems wrong. :(

> For this case, need have OR operation ( | RUNTIME) to original attribute in GCD

> database?

> 

> 

> Thanks,

> Star

> -----Original Message-----

> From: Yao, Jiewen

> Sent: Tuesday, April 3, 2018 10:34 AM

> To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;

> Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao,

> Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao

> Liang <liangrenhao@huawei.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> 

> That is good idea.

> 

> Just add more description as the code comment, please.

> 

> It is easy for code review later.

> 

> Thank you

> Yao Jiewen

> 

> 

> > -----Original Message-----

> > From: Zeng, Star

> > Sent: Tuesday, April 3, 2018 10:33 AM

> > To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J

> > <jian.j.wang@intel.com>; Kinney, Michael D

> > <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>;

> > edk2-devel@lists.01.org

> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>;

> > Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;

> > Renhao Liang <liangrenhao@huawei.com>; Zeng, Star

> > <star.zeng@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > conversion

> >

> > Agree.

> >

> > And I agree we need the fix at

> > https://lists.01.org/pipermail/edk2-devel/2018-April/023364.html from

> > Mike for compatibility.

> > And I think more precious and precise information need to be added

> > into the commit log or code comments since we have been crazy about

> > handling attributes in the GCD service. :)

> >

> >

> > Thanks,

> > Star

> > -----Original Message-----

> > From: Yao, Jiewen

> > Sent: Tuesday, April 3, 2018 10:26 AM

> > To: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star

> > <star.zeng@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;

> > Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>;

> > Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>;

> > Renhao Liang <liangrenhao@huawei.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > conversion

> >

> > I have a discussion with Star/Jian.

> >

> > The expectation for the CPU driver is to handle PageAttribute.

> > The expectation for the platform driver is to use GetAttribute(),

> > AND/OR attribute, then call SetAttribute().

> > Because the DRAM always has a cache attribute (no matter UC or WB), 0

> > should not happen. (0 might happen for a GCD reserved memory, but

> > there is no need to set page attribute)

> >

> > If all drivers use above way, there won't be any issue. There is no

> > need to introduce a new protocol so far.

> >

> > We did encounter some error, because the platform/silicon/CPU code is

> > not updated yet. (For example, the MinnowMax which is using binary)

> > The fix to filter 0 seems a workable way to resolve the compatibility issue.

> >

> > Later, I suggest we update MinnowMax binary - Add paging handling for

> > CPU driver, and use GetAttribute()/SetAttribute() for platform/silicon code.

> >

> >

> > Thank you

> > Yao Jiewen

> >

> > > -----Original Message-----

> > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf

> > > Of Wang, Jian J

> > > Sent: Tuesday, April 3, 2018 9:24 AM

> > > To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D

> > > <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>;

> > > edk2-devel@lists.01.org

> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>;

> > > Gao, Liming <liming.gao@intel.com>; Dong, Eric

> > > <eric.dong@intel.com>; Renhao Liang <liangrenhao@huawei.com>

> > > Subject: Re: [edk2] [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of

> > > attribute conversion

> > >

> > > The NX Memory Protection and Heap Guard features need to clear

> > > paging attributes.

> > > Allowing 0 attribute is the current only choice without changing arch

> protocol.

> > > Maybe

> > > It's time to introduce a new protocol.

> > >

> > > Regards,

> > > Jian

> > >

> > >

> > > > -----Original Message-----

> > > > From: Zeng, Star

> > > > Sent: Tuesday, April 03, 2018 9:16 AM

> > > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> > > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao,

> > > > Liming <liming.gao@intel.com>; Wang, Jian J

> > > > <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng,

> > > > Star <star.zeng@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > > conversion

> > > >

> > > > Current gCpu->SetMemoryAttributes does not support getting memory

> > > > attributes, and has no clear description about clearing memory attributes.

> > > >

> > > > I noticed we introduced

> > > >

> > >

> >

> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribut

> > > e.

> > > > h) protocol for heap guard feature in SMM environment.

> > > > Seemingly, we also need introduce MemoryAttribute protocol for DXE?

> > > >

> > > >

> > > > Thanks,

> > > > Star

> > > > -----Original Message-----

> > > > From: Zeng, Star

> > > > Sent: Tuesday, April 3, 2018 8:59 AM

> > > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> > > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao,

> > > > Liming <liming.gao@intel.com>; Wang, Jian J

> > > > <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng,

> > > > Star <star.zeng@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > > conversion

> > > >

> > > > Mike,

> > > >

> > > > Agree the problem.

> > > > We need support only RUNTIME attribute.

> > > > We need support only cache attributes.

> > > > We need support only page attributes.

> > > > We need support RUNTIME + cache + page attributes.

> > > > Do we need support the 0 Attributes case to clear page attributes?

> > > > There was discussion about the 0 Attributes case at

> > > > https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.

> > > > It came to the question that should the 0 Attributes case be

> > > > handled by

> > > > SetMemoryAttributes() to clear the page attributes?

> > > >

> > > >

> > > > Thanks,

> > > > Star

> > > > -----Original Message-----

> > > > From: Kinney, Michael D

> > > > Sent: Tuesday, April 3, 2018 5:43 AM

> > > > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo

> > > > <heyi.guo@linaro.org>; edk2- devel@lists.01.org; Kinney, Michael D

> > > > <michael.d.kinney@intel.com>

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao,

> > > > Liming <liming.gao@intel.com>; Wang, Jian J

> > > > <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > > conversion

> > > >

> > > > Star,

> > > >

> > > > This commit breaks Vlv2TbltDevicePkg.

> > > >

> > > > On this platform, there are 2 calls to

> > > > gDS->SetMemorySpaceAttributes() for an MMIO

> > > > range to set only the EFI_MEMORY_RUNTIME bit.

> > > >

> > > > With this commit, ConverToCpuArchAttributes()returns 0, and 0 is

> > > > passed to

> > > > gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on

> > > > Vlv2TbltDevicePkg.

> > > >

> > > > Before this commit, ConverToCpuArchAttributes() returns

> > > > INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu-

> > > > >SetMemoryAttributes()to be skipped so no error is generated.

> > > >

> > > > I think the right fix is to skip the call to

> > > > gCpu->SetMemoryAttributes() if CpuArchAttributes

> > > > is 0 so a call that only sets the RUNTIME attribute can be

> > > > supported along with call the set both the RUNTIME bit and other cache

> related bits.

> > > >

> > > > I will send a patch soon with the proposed fix.

> > > >

> > > > Mike

> > > >

> > > > > -----Original Message-----

> > > > > From: Zeng, Star

> > > > > Sent: Sunday, April 1, 2018 10:59 PM

> > > > > To: Heyi Guo <heyi.guo@linaro.org>; edk2- devel@lists.01.org

> > > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>;

> > > > > Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > > > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;

> > > > > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > > > conversion

> > > > >

> > > > > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> > > > >

> > > > > Thanks,

> > > > > Star

> > > > > -----Original Message-----

> > > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]

> > > > > Sent: Thursday, March 29, 2018 4:20 PM

> > > > > To: edk2-devel@lists.01.org

> > > > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li

> > > > > <phoenix.liyi@huawei.com>; Renhao Liang

> > > > > <liangrenhao@huawei.com>; Zeng, Star <star.zeng@intel.com>;

> > > > > Dong, Eric <eric.dong@intel.com>; Kinney, Michael D

> > > > > <michael.d.kinney@intel.com>; Gao, Liming

> > > > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;

> > > > > Ni, Ruiyu <ruiyu.ni@intel.com>

> > > > > Subject: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > > > conversion

> > > > >

> > > > > For gDS->SetMemorySpaceAttributes(), when user passes a combined

> > > > > memory attribute including CPU arch attribute and other

> > > > > attributes, like EFI_MEMORY_RUNTIME,

> > > > > ConverToCpuArchAttributes() will return

> > > > > INVALID_CPU_ARCH_ATTRIBUTES and skip setting page/cache

> > > > > attribute for

> > the specified memory space.

> > > > >

> > > > > We don't see any reason to forbid combining CPU arch attributes

> > > > > and non-CPU-arch attributes when calling gDS-

> > > > > >SetMemorySpaceAttributes(), so we remove the check code

> > > > > in ConverToCpuArchAttributes(); the remaining code is enough to

> > > > > grab the interested bits for

> > > > > Cpu->SetMemoryAttributes().

> > > > >

> > > > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > > > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > > > > Signed-off-by: Renhao Liang <liangrenhao@huawei.com>

> > > > > Cc: Star Zeng <star.zeng@intel.com>

> > > > > Cc: Eric Dong <eric.dong@intel.com>

> > > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > > > > Cc: Liming Gao <liming.gao@intel.com>

> > > > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > > > > ---

> > > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

> > > > >  1 file changed, 5 deletions(-)

> > > > >

> > > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> > > > > 77f4adb4bc01..907245a3f512 100644

> > > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {

> > > > >    UINT64      CpuArchAttributes;

> > > > >

> > > > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> > > > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> > > > > != 0) {

> > > > > -    return INVALID_CPU_ARCH_ATTRIBUTES;

> > > > > -  }

> > > > > -

> > > > >    CpuArchAttributes = Attributes &

> > > > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> > > > >

> > > > >    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {

> > > > > --

> > > > > 2.7.4

> > >

> > > _______________________________________________

> > > edk2-devel mailing list

> > > edk2-devel@lists.01.org

> > > https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Kinney, Michael D April 3, 2018, 9:45 p.m. UTC | #12
Star,

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

Thanks,

Mike

> -----Original Message-----

> From: Yao, Jiewen

> Sent: Monday, April 2, 2018 7:34 PM

> To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>; Kinney, Michael D

> <michael.d.kinney@intel.com>; Heyi Guo

> <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li

> <phoenix.liyi@huawei.com>; Gao, Liming

> <liming.gao@intel.com>; Dong, Eric

> <eric.dong@intel.com>; Renhao Liang

> <liangrenhao@huawei.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of

> attribute conversion

> 

> That is good idea.

> 

> Just add more description as the code comment, please.

> 

> It is easy for code review later.

> 

> Thank you

> Yao Jiewen

> 

> 

> > -----Original Message-----

> > From: Zeng, Star

> > Sent: Tuesday, April 3, 2018 10:33 AM

> > To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>;

> > Kinney, Michael D <michael.d.kinney@intel.com>; Heyi

> Guo

> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li

> <phoenix.liyi@huawei.com>; Gao,

> > Liming <liming.gao@intel.com>; Dong, Eric

> <eric.dong@intel.com>; Renhao

> > Liang <liangrenhao@huawei.com>; Zeng, Star

> <star.zeng@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of

> attribute conversion

> >

> > Agree.

> >

> > And I agree we need the fix at

> > https://lists.01.org/pipermail/edk2-devel/2018-

> April/023364.html from Mike

> > for compatibility.

> > And I think more precious and precise information need

> to be added into the

> > commit log or code comments since we have been crazy

> about handling

> > attributes in the GCD service. :)

> >

> >

> > Thanks,

> > Star

> > -----Original Message-----

> > From: Yao, Jiewen

> > Sent: Tuesday, April 3, 2018 10:26 AM

> > To: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star

> <star.zeng@intel.com>;

> > Kinney, Michael D <michael.d.kinney@intel.com>; Heyi

> Guo

> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li

> <phoenix.liyi@huawei.com>; Gao,

> > Liming <liming.gao@intel.com>; Dong, Eric

> <eric.dong@intel.com>; Renhao

> > Liang <liangrenhao@huawei.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of

> attribute conversion

> >

> > I have a discussion with Star/Jian.

> >

> > The expectation for the CPU driver is to handle

> PageAttribute.

> > The expectation for the platform driver is to use

> GetAttribute(), AND/OR

> > attribute, then call SetAttribute().

> > Because the DRAM always has a cache attribute (no

> matter UC or WB), 0 should

> > not happen. (0 might happen for a GCD reserved memory,

> but there is no need

> > to set page attribute)

> >

> > If all drivers use above way, there won't be any

> issue. There is no need to

> > introduce a new protocol so far.

> >

> > We did encounter some error, because the

> platform/silicon/CPU code is not

> > updated yet. (For example, the MinnowMax which is

> using binary) The fix to

> > filter 0 seems a workable way to resolve the

> compatibility issue.

> >

> > Later, I suggest we update MinnowMax binary - Add

> paging handling for CPU

> > driver, and use GetAttribute()/SetAttribute() for

> platform/silicon code.

> >

> >

> > Thank you

> > Yao Jiewen

> >

> > > -----Original Message-----

> > > From: edk2-devel [mailto:edk2-devel-

> bounces@lists.01.org] On Behalf Of

> > > Wang, Jian J

> > > Sent: Tuesday, April 3, 2018 9:24 AM

> > > To: Zeng, Star <star.zeng@intel.com>; Kinney,

> Michael D

> > > <michael.d.kinney@intel.com>; Heyi Guo

> <heyi.guo@linaro.org>;

> > > edk2-devel@lists.01.org

> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li

> <phoenix.liyi@huawei.com>;

> > > Gao, Liming <liming.gao@intel.com>; Dong, Eric

> <eric.dong@intel.com>;

> > > Renhao Liang <liangrenhao@huawei.com>

> > > Subject: Re: [edk2] [PATCH 1/1] MdeModulePkg/Gcd:

> Fix bug of attribute

> > > conversion

> > >

> > > The NX Memory Protection and Heap Guard features

> need to clear paging

> > > attributes.

> > > Allowing 0 attribute is the current only choice

> without changing arch protocol.

> > > Maybe

> > > It's time to introduce a new protocol.

> > >

> > > Regards,

> > > Jian

> > >

> > >

> > > > -----Original Message-----

> > > > From: Zeng, Star

> > > > Sent: Tuesday, April 03, 2018 9:16 AM

> > > > To: Kinney, Michael D

> <michael.d.kinney@intel.com>; Heyi Guo

> > > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > > <liangrenhao@huawei.com>; Dong, Eric

> <eric.dong@intel.com>; Gao,

> > > > Liming <liming.gao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>;

> > > > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star

> <star.zeng@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug

> of attribute

> > > > conversion

> > > >

> > > > Current gCpu->SetMemoryAttributes does not support

> getting memory

> > > > attributes, and has no clear description about

> clearing memory attributes.

> > > >

> > > > I noticed we introduced

> > > >

> > >

> >

> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemo

> ryAttribut

> > > e.

> > > > h) protocol for heap guard feature in SMM

> environment.

> > > > Seemingly, we also need introduce MemoryAttribute

> protocol for DXE?

> > > >

> > > >

> > > > Thanks,

> > > > Star

> > > > -----Original Message-----

> > > > From: Zeng, Star

> > > > Sent: Tuesday, April 3, 2018 8:59 AM

> > > > To: Kinney, Michael D

> <michael.d.kinney@intel.com>; Heyi Guo

> > > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > > <liangrenhao@huawei.com>; Dong, Eric

> <eric.dong@intel.com>; Gao,

> > > > Liming <liming.gao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>;

> > > > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star

> <star.zeng@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug

> of attribute

> > > > conversion

> > > >

> > > > Mike,

> > > >

> > > > Agree the problem.

> > > > We need support only RUNTIME attribute.

> > > > We need support only cache attributes.

> > > > We need support only page attributes.

> > > > We need support RUNTIME + cache + page attributes.

> > > > Do we need support the 0 Attributes case to clear

> page attributes?

> > > > There was discussion about the 0 Attributes case

> at

> > > > https://lists.01.org/pipermail/edk2-devel/2018-

> March/023315.html.

> > > > It came to the question that should the 0

> Attributes case be handled

> > > > by

> > > > SetMemoryAttributes() to clear the page

> attributes?

> > > >

> > > >

> > > > Thanks,

> > > > Star

> > > > -----Original Message-----

> > > > From: Kinney, Michael D

> > > > Sent: Tuesday, April 3, 2018 5:43 AM

> > > > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo

> > > > <heyi.guo@linaro.org>; edk2- devel@lists.01.org;

> Kinney, Michael D

> > > > <michael.d.kinney@intel.com>

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > > <liangrenhao@huawei.com>; Dong, Eric

> <eric.dong@intel.com>; Gao,

> > > > Liming <liming.gao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>;

> > > > Ni, Ruiyu <ruiyu.ni@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug

> of attribute

> > > > conversion

> > > >

> > > > Star,

> > > >

> > > > This commit breaks Vlv2TbltDevicePkg.

> > > >

> > > > On this platform, there are 2 calls to

> > > > gDS->SetMemorySpaceAttributes() for an MMIO

> > > > range to set only the EFI_MEMORY_RUNTIME bit.

> > > >

> > > > With this commit,

> ConverToCpuArchAttributes()returns 0, and 0 is

> > > > passed to

> > > > gCpu->SetMemoryAttributes()that returns

> EFI_INVALID_PARAMETER on

> > > > Vlv2TbltDevicePkg.

> > > >

> > > > Before this commit, ConverToCpuArchAttributes()

> returns

> > > > INVALID_CPU_ARCH_ATTRIBUTES which causes the call

> to gCpu-

> > > > >SetMemoryAttributes()to be skipped so no error is

> generated.

> > > >

> > > > I think the right fix is to skip the call to

> > > > gCpu->SetMemoryAttributes() if CpuArchAttributes

> > > > is 0 so a call that only sets the RUNTIME

> attribute can be supported

> > > > along with call the set both the RUNTIME bit and

> other cache related bits.

> > > >

> > > > I will send a patch soon with the proposed fix.

> > > >

> > > > Mike

> > > >

> > > > > -----Original Message-----

> > > > > From: Zeng, Star

> > > > > Sent: Sunday, April 1, 2018 10:59 PM

> > > > > To: Heyi Guo <heyi.guo@linaro.org>; edk2-

> devel@lists.01.org

> > > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao

> Liang

> > > > > <liangrenhao@huawei.com>; Dong, Eric

> <eric.dong@intel.com>;

> > > > > Kinney, Michael D <michael.d.kinney@intel.com>;

> Gao, Liming

> > > > > <liming.gao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>; Ni,

> > > > > Ruiyu <ruiyu.ni@intel.com>; Zeng, Star

> <star.zeng@intel.com>

> > > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix

> bug of attribute

> > > > > conversion

> > > > >

> > > > > Pushed at

> 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> > > > >

> > > > > Thanks,

> > > > > Star

> > > > > -----Original Message-----

> > > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]

> > > > > Sent: Thursday, March 29, 2018 4:20 PM

> > > > > To: edk2-devel@lists.01.org

> > > > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li

> > > > > <phoenix.liyi@huawei.com>; Renhao Liang

> <liangrenhao@huawei.com>;

> > > > > Zeng, Star <star.zeng@intel.com>; Dong, Eric

> > > > > <eric.dong@intel.com>; Kinney, Michael D

> > > > > <michael.d.kinney@intel.com>; Gao, Liming

> <liming.gao@intel.com>;

> > > > > Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> > > > > <ruiyu.ni@intel.com>

> > > > > Subject: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug

> of attribute

> > > > > conversion

> > > > >

> > > > > For gDS->SetMemorySpaceAttributes(), when user

> passes a combined

> > > > > memory attribute including CPU arch attribute

> and other

> > > > > attributes, like EFI_MEMORY_RUNTIME,

> > > > > ConverToCpuArchAttributes() will return

> > > > > INVALID_CPU_ARCH_ATTRIBUTES and skip setting

> page/cache attribute for

> > the specified memory space.

> > > > >

> > > > > We don't see any reason to forbid combining CPU

> arch attributes

> > > > > and non-CPU-arch attributes when calling gDS-

> > > > > >SetMemorySpaceAttributes(), so we remove the

> check code

> > > > > in ConverToCpuArchAttributes(); the remaining

> code is enough to

> > > > > grab the interested bits for

> > > > > Cpu->SetMemoryAttributes().

> > > > >

> > > > > Contributed-under: TianoCore Contribution

> Agreement 1.1

> > > > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > > > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > > > > Signed-off-by: Renhao Liang

> <liangrenhao@huawei.com>

> > > > > Cc: Star Zeng <star.zeng@intel.com>

> > > > > Cc: Eric Dong <eric.dong@intel.com>

> > > > > Cc: Michael D Kinney

> <michael.d.kinney@intel.com>

> > > > > Cc: Liming Gao <liming.gao@intel.com>

> > > > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > > > > ---

> > > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

> > > > >  1 file changed, 5 deletions(-)

> > > > >

> > > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> > > > > 77f4adb4bc01..907245a3f512 100644

> > > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (

> {

> > > > >    UINT64      CpuArchAttributes;

> > > > >

> > > > > -  if ((Attributes &

> ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> > > > > -

> NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> > > > > != 0) {

> > > > > -    return INVALID_CPU_ARCH_ATTRIBUTES;

> > > > > -  }

> > > > > -

> > > > >    CpuArchAttributes = Attributes &

> > > > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> > > > >

> > > > >    if ( (Attributes & EFI_MEMORY_UC) ==

> EFI_MEMORY_UC) {

> > > > > --

> > > > > 2.7.4

> > >

> > > _______________________________________________

> > > edk2-devel mailing list

> > > edk2-devel@lists.01.org

> > > https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Zeng, Star April 4, 2018, 12:38 a.m. UTC | #13
Sure, I can do that.

Thanks,
Star
-----Original Message-----
From: Kinney, Michael D 

Sent: Wednesday, April 4, 2018 5:45 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao Liang <liangrenhao@huawei.com>
Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

Star,

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

Thanks,

Mike

> -----Original Message-----

> From: Yao, Jiewen

> Sent: Monday, April 2, 2018 7:34 PM

> To: Zeng, Star <star.zeng@intel.com>; Wang, Jian J 

> <jian.j.wang@intel.com>; Kinney, Michael D 

> <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>; 

> edk2-devel@lists.01.org

> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; 

> Gao, Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; 

> Renhao Liang <liangrenhao@huawei.com>

> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 

> conversion

> 

> That is good idea.

> 

> Just add more description as the code comment, please.

> 

> It is easy for code review later.

> 

> Thank you

> Yao Jiewen

> 

> 

> > -----Original Message-----

> > From: Zeng, Star

> > Sent: Tuesday, April 3, 2018 10:33 AM

> > To: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>;

> > Kinney, Michael D <michael.d.kinney@intel.com>; Heyi

> Guo

> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li

> <phoenix.liyi@huawei.com>; Gao,

> > Liming <liming.gao@intel.com>; Dong, Eric

> <eric.dong@intel.com>; Renhao

> > Liang <liangrenhao@huawei.com>; Zeng, Star

> <star.zeng@intel.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of

> attribute conversion

> >

> > Agree.

> >

> > And I agree we need the fix at

> > https://lists.01.org/pipermail/edk2-devel/2018-

> April/023364.html from Mike

> > for compatibility.

> > And I think more precious and precise information need

> to be added into the

> > commit log or code comments since we have been crazy

> about handling

> > attributes in the GCD service. :)

> >

> >

> > Thanks,

> > Star

> > -----Original Message-----

> > From: Yao, Jiewen

> > Sent: Tuesday, April 3, 2018 10:26 AM

> > To: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star

> <star.zeng@intel.com>;

> > Kinney, Michael D <michael.d.kinney@intel.com>; Heyi

> Guo

> > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li

> <phoenix.liyi@huawei.com>; Gao,

> > Liming <liming.gao@intel.com>; Dong, Eric

> <eric.dong@intel.com>; Renhao

> > Liang <liangrenhao@huawei.com>

> > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of

> attribute conversion

> >

> > I have a discussion with Star/Jian.

> >

> > The expectation for the CPU driver is to handle

> PageAttribute.

> > The expectation for the platform driver is to use

> GetAttribute(), AND/OR

> > attribute, then call SetAttribute().

> > Because the DRAM always has a cache attribute (no

> matter UC or WB), 0 should

> > not happen. (0 might happen for a GCD reserved memory,

> but there is no need

> > to set page attribute)

> >

> > If all drivers use above way, there won't be any

> issue. There is no need to

> > introduce a new protocol so far.

> >

> > We did encounter some error, because the

> platform/silicon/CPU code is not

> > updated yet. (For example, the MinnowMax which is

> using binary) The fix to

> > filter 0 seems a workable way to resolve the

> compatibility issue.

> >

> > Later, I suggest we update MinnowMax binary - Add

> paging handling for CPU

> > driver, and use GetAttribute()/SetAttribute() for

> platform/silicon code.

> >

> >

> > Thank you

> > Yao Jiewen

> >

> > > -----Original Message-----

> > > From: edk2-devel [mailto:edk2-devel-

> bounces@lists.01.org] On Behalf Of

> > > Wang, Jian J

> > > Sent: Tuesday, April 3, 2018 9:24 AM

> > > To: Zeng, Star <star.zeng@intel.com>; Kinney,

> Michael D

> > > <michael.d.kinney@intel.com>; Heyi Guo

> <heyi.guo@linaro.org>;

> > > edk2-devel@lists.01.org

> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li

> <phoenix.liyi@huawei.com>;

> > > Gao, Liming <liming.gao@intel.com>; Dong, Eric

> <eric.dong@intel.com>;

> > > Renhao Liang <liangrenhao@huawei.com>

> > > Subject: Re: [edk2] [PATCH 1/1] MdeModulePkg/Gcd:

> Fix bug of attribute

> > > conversion

> > >

> > > The NX Memory Protection and Heap Guard features

> need to clear paging

> > > attributes.

> > > Allowing 0 attribute is the current only choice

> without changing arch protocol.

> > > Maybe

> > > It's time to introduce a new protocol.

> > >

> > > Regards,

> > > Jian

> > >

> > >

> > > > -----Original Message-----

> > > > From: Zeng, Star

> > > > Sent: Tuesday, April 03, 2018 9:16 AM

> > > > To: Kinney, Michael D

> <michael.d.kinney@intel.com>; Heyi Guo

> > > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > > > <liangrenhao@huawei.com>; Dong, Eric

> <eric.dong@intel.com>; Gao,

> > > > Liming <liming.gao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>;

> > > > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star

> <star.zeng@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug

> of attribute

> > > > conversion

> > > >

> > > > Current gCpu->SetMemoryAttributes does not support

> getting memory

> > > > attributes, and has no clear description about

> clearing memory attributes.

> > > >

> > > > I noticed we introduced

> > > >

> > >

> >

> SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemo

> ryAttribut

> > > e.

> > > > h) protocol for heap guard feature in SMM

> environment.

> > > > Seemingly, we also need introduce MemoryAttribute

> protocol for DXE?

> > > >

> > > >

> > > > Thanks,

> > > > Star

> > > > -----Original Message-----

> > > > From: Zeng, Star

> > > > Sent: Tuesday, April 3, 2018 8:59 AM

> > > > To: Kinney, Michael D

> <michael.d.kinney@intel.com>; Heyi Guo

> > > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > > > <liangrenhao@huawei.com>; Dong, Eric

> <eric.dong@intel.com>; Gao,

> > > > Liming <liming.gao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>;

> > > > Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star

> <star.zeng@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug

> of attribute

> > > > conversion

> > > >

> > > > Mike,

> > > >

> > > > Agree the problem.

> > > > We need support only RUNTIME attribute.

> > > > We need support only cache attributes.

> > > > We need support only page attributes.

> > > > We need support RUNTIME + cache + page attributes.

> > > > Do we need support the 0 Attributes case to clear

> page attributes?

> > > > There was discussion about the 0 Attributes case

> at

> > > > https://lists.01.org/pipermail/edk2-devel/2018-

> March/023315.html.

> > > > It came to the question that should the 0

> Attributes case be handled

> > > > by

> > > > SetMemoryAttributes() to clear the page

> attributes?

> > > >

> > > >

> > > > Thanks,

> > > > Star

> > > > -----Original Message-----

> > > > From: Kinney, Michael D

> > > > Sent: Tuesday, April 3, 2018 5:43 AM

> > > > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo 

> > > > <heyi.guo@linaro.org>; edk2- devel@lists.01.org;

> Kinney, Michael D

> > > > <michael.d.kinney@intel.com>

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang 

> > > > <liangrenhao@huawei.com>; Dong, Eric

> <eric.dong@intel.com>; Gao,

> > > > Liming <liming.gao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>;

> > > > Ni, Ruiyu <ruiyu.ni@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug

> of attribute

> > > > conversion

> > > >

> > > > Star,

> > > >

> > > > This commit breaks Vlv2TbltDevicePkg.

> > > >

> > > > On this platform, there are 2 calls to

> > > > gDS->SetMemorySpaceAttributes() for an MMIO

> > > > range to set only the EFI_MEMORY_RUNTIME bit.

> > > >

> > > > With this commit,

> ConverToCpuArchAttributes()returns 0, and 0 is

> > > > passed to

> > > > gCpu->SetMemoryAttributes()that returns

> EFI_INVALID_PARAMETER on

> > > > Vlv2TbltDevicePkg.

> > > >

> > > > Before this commit, ConverToCpuArchAttributes()

> returns

> > > > INVALID_CPU_ARCH_ATTRIBUTES which causes the call

> to gCpu-

> > > > >SetMemoryAttributes()to be skipped so no error is

> generated.

> > > >

> > > > I think the right fix is to skip the call to

> > > > gCpu->SetMemoryAttributes() if CpuArchAttributes

> > > > is 0 so a call that only sets the RUNTIME

> attribute can be supported

> > > > along with call the set both the RUNTIME bit and

> other cache related bits.

> > > >

> > > > I will send a patch soon with the proposed fix.

> > > >

> > > > Mike

> > > >

> > > > > -----Original Message-----

> > > > > From: Zeng, Star

> > > > > Sent: Sunday, April 1, 2018 10:59 PM

> > > > > To: Heyi Guo <heyi.guo@linaro.org>; edk2-

> devel@lists.01.org

> > > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao

> Liang

> > > > > <liangrenhao@huawei.com>; Dong, Eric

> <eric.dong@intel.com>;

> > > > > Kinney, Michael D <michael.d.kinney@intel.com>;

> Gao, Liming

> > > > > <liming.gao@intel.com>; Wang, Jian J

> <jian.j.wang@intel.com>; Ni,

> > > > > Ruiyu <ruiyu.ni@intel.com>; Zeng, Star

> <star.zeng@intel.com>

> > > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix

> bug of attribute

> > > > > conversion

> > > > >

> > > > > Pushed at

> 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> > > > >

> > > > > Thanks,

> > > > > Star

> > > > > -----Original Message-----

> > > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]

> > > > > Sent: Thursday, March 29, 2018 4:20 PM

> > > > > To: edk2-devel@lists.01.org

> > > > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li 

> > > > > <phoenix.liyi@huawei.com>; Renhao Liang

> <liangrenhao@huawei.com>;

> > > > > Zeng, Star <star.zeng@intel.com>; Dong, Eric 

> > > > > <eric.dong@intel.com>; Kinney, Michael D 

> > > > > <michael.d.kinney@intel.com>; Gao, Liming

> <liming.gao@intel.com>;

> > > > > Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu 

> > > > > <ruiyu.ni@intel.com>

> > > > > Subject: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug

> of attribute

> > > > > conversion

> > > > >

> > > > > For gDS->SetMemorySpaceAttributes(), when user

> passes a combined

> > > > > memory attribute including CPU arch attribute

> and other

> > > > > attributes, like EFI_MEMORY_RUNTIME,

> > > > > ConverToCpuArchAttributes() will return 

> > > > > INVALID_CPU_ARCH_ATTRIBUTES and skip setting

> page/cache attribute for

> > the specified memory space.

> > > > >

> > > > > We don't see any reason to forbid combining CPU

> arch attributes

> > > > > and non-CPU-arch attributes when calling gDS-

> > > > > >SetMemorySpaceAttributes(), so we remove the

> check code

> > > > > in ConverToCpuArchAttributes(); the remaining

> code is enough to

> > > > > grab the interested bits for

> > > > > Cpu->SetMemoryAttributes().

> > > > >

> > > > > Contributed-under: TianoCore Contribution

> Agreement 1.1

> > > > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > > > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > > > > Signed-off-by: Renhao Liang

> <liangrenhao@huawei.com>

> > > > > Cc: Star Zeng <star.zeng@intel.com>

> > > > > Cc: Eric Dong <eric.dong@intel.com>

> > > > > Cc: Michael D Kinney

> <michael.d.kinney@intel.com>

> > > > > Cc: Liming Gao <liming.gao@intel.com>

> > > > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > > > > ---

> > > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

> > > > >  1 file changed, 5 deletions(-)

> > > > >

> > > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c 

> > > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> > > > > 77f4adb4bc01..907245a3f512 100644

> > > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (

> {

> > > > >    UINT64      CpuArchAttributes;

> > > > >

> > > > > -  if ((Attributes &

> ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> > > > > -

> NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> > > > > != 0) {

> > > > > -    return INVALID_CPU_ARCH_ATTRIBUTES;

> > > > > -  }

> > > > > -

> > > > >    CpuArchAttributes = Attributes & 

> > > > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> > > > >

> > > > >    if ( (Attributes & EFI_MEMORY_UC) ==

> EFI_MEMORY_UC) {

> > > > > --

> > > > > 2.7.4

> > >

> > > _______________________________________________

> > > edk2-devel mailing list

> > > edk2-devel@lists.01.org

> > > https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo April 4, 2018, 1:12 a.m. UTC | #14
On Tue, Apr 03, 2018 at 02:25:30AM +0000, Yao, Jiewen wrote:
> I have a discussion with Star/Jian.

> 

> The expectation for the CPU driver is to handle PageAttribute.

> The expectation for the platform driver is to use GetAttribute(), AND/OR attribute, then call SetAttribute().


Agree, "Get and then Set" seems to be the clearest definition for
gDS->SetMemoryAttributes().

Thanks,

Heyi

> Because the DRAM always has a cache attribute (no matter UC or WB), 0 should not happen. (0 might happen for a GCD reserved memory, but there is no need to set page attribute)

> 

> If all drivers use above way, there won't be any issue. There is no need to introduce a new protocol so far.

> 

> We did encounter some error, because the platform/silicon/CPU code is not updated yet. (For example, the MinnowMax which is using binary)

> The fix to filter 0 seems a workable way to resolve the compatibility issue.

> 

> Later, I suggest we update MinnowMax binary - Add paging handling for CPU driver, and use GetAttribute()/SetAttribute() for platform/silicon code.

> 

> 

> Thank you

> Yao Jiewen

> 

> > -----Original Message-----

> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,

> > Jian J

> > Sent: Tuesday, April 3, 2018 9:24 AM

> > To: Zeng, Star <star.zeng@intel.com>; Kinney, Michael D

> > <michael.d.kinney@intel.com>; Heyi Guo <heyi.guo@linaro.org>;

> > edk2-devel@lists.01.org

> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yi Li <phoenix.liyi@huawei.com>; Gao,

> > Liming <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>; Renhao

> > Liang <liangrenhao@huawei.com>

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

> > conversion

> > 

> > The NX Memory Protection and Heap Guard features need to clear paging

> > attributes.

> > Allowing 0 attribute is the current only choice without changing arch protocol.

> > Maybe

> > It's time to introduce a new protocol.

> > 

> > Regards,

> > Jian

> > 

> > 

> > > -----Original Message-----

> > > From: Zeng, Star

> > > Sent: Tuesday, April 03, 2018 9:16 AM

> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming

> > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> > > <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> > >

> > > Current gCpu->SetMemoryAttributes does not support getting memory

> > > attributes, and has no clear description about clearing memory attributes.

> > >

> > > I noticed we introduced

> > >

> > SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribut

> > e.

> > > h) protocol for heap guard feature in SMM environment.

> > > Seemingly, we also need introduce MemoryAttribute protocol for DXE?

> > >

> > >

> > > Thanks,

> > > Star

> > > -----Original Message-----

> > > From: Zeng, Star

> > > Sent: Tuesday, April 3, 2018 8:59 AM

> > > To: Kinney, Michael D <michael.d.kinney@intel.com>; Heyi Guo

> > > <heyi.guo@linaro.org>; edk2-devel@lists.01.org

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming

> > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> > > <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> > >

> > > Mike,

> > >

> > > Agree the problem.

> > > We need support only RUNTIME attribute.

> > > We need support only cache attributes.

> > > We need support only page attributes.

> > > We need support RUNTIME + cache + page attributes.

> > > Do we need support the 0 Attributes case to clear page attributes?

> > > There was discussion about the 0 Attributes case at

> > > https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.

> > > It came to the question that should the 0 Attributes case be handled by

> > > SetMemoryAttributes() to clear the page attributes?

> > >

> > >

> > > Thanks,

> > > Star

> > > -----Original Message-----

> > > From: Kinney, Michael D

> > > Sent: Tuesday, April 3, 2018 5:43 AM

> > > To: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-

> > > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>

> > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming

> > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu

> > > <ruiyu.ni@intel.com>

> > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

> > >

> > > Star,

> > >

> > > This commit breaks Vlv2TbltDevicePkg.

> > >

> > > On this platform, there are 2 calls to

> > > gDS->SetMemorySpaceAttributes() for an MMIO

> > > range to set only the EFI_MEMORY_RUNTIME bit.

> > >

> > > With this commit, ConverToCpuArchAttributes()returns 0, and 0 is passed to

> > > gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on

> > > Vlv2TbltDevicePkg.

> > >

> > > Before this commit, ConverToCpuArchAttributes() returns

> > > INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu-

> > > >SetMemoryAttributes()to be skipped so no error is generated.

> > >

> > > I think the right fix is to skip the call to

> > > gCpu->SetMemoryAttributes() if CpuArchAttributes

> > > is 0 so a call that only sets the RUNTIME attribute can be supported along with

> > > call the set both the RUNTIME bit and other cache related bits.

> > >

> > > I will send a patch soon with the proposed fix.

> > >

> > > Mike

> > >

> > > > -----Original Message-----

> > > > From: Zeng, Star

> > > > Sent: Sunday, April 1, 2018 10:59 PM

> > > > To: Heyi Guo <heyi.guo@linaro.org>; edk2- devel@lists.01.org

> > > > Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang

> > > > <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Kinney,

> > > > Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni,

> > > > Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>

> > > > Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute

> > > > conversion

> > > >

> > > > Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.

> > > >

> > > > Thanks,

> > > > Star

> > > > -----Original Message-----

> > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]

> > > > Sent: Thursday, March 29, 2018 4:20 PM

> > > > To: edk2-devel@lists.01.org

> > > > Cc: Heyi Guo <heyi.guo@linaro.org>; Yi Li <phoenix.liyi@huawei.com>;

> > > > Renhao Liang <liangrenhao@huawei.com>; Zeng, Star

> > > > <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney,

> > > > Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > > > <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni,

> > > > Ruiyu <ruiyu.ni@intel.com>

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

> > > >

> > > > For gDS->SetMemorySpaceAttributes(), when user passes a combined

> > > > memory attribute including CPU arch attribute and other attributes,

> > > > like EFI_MEMORY_RUNTIME,

> > > > ConverToCpuArchAttributes() will return INVALID_CPU_ARCH_ATTRIBUTES

> > > > and skip setting page/cache attribute for the specified memory space.

> > > >

> > > > We don't see any reason to forbid combining CPU arch attributes and

> > > > non-CPU-arch attributes when calling gDS-

> > > > >SetMemorySpaceAttributes(), so we remove the check code

> > > > in ConverToCpuArchAttributes(); the remaining code is enough to grab

> > > > the interested bits for

> > > > Cpu->SetMemoryAttributes().

> > > >

> > > > Contributed-under: TianoCore Contribution Agreement 1.1

> > > > Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> > > > Signed-off-by: Yi Li <phoenix.liyi@huawei.com>

> > > > Signed-off-by: Renhao Liang <liangrenhao@huawei.com>

> > > > Cc: Star Zeng <star.zeng@intel.com>

> > > > Cc: Eric Dong <eric.dong@intel.com>

> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>

> > > > Cc: Liming Gao <liming.gao@intel.com>

> > > > Cc: Jian J Wang <jian.j.wang@intel.com>

> > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> > > > ---

> > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----

> > > >  1 file changed, 5 deletions(-)

> > > >

> > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index

> > > > 77f4adb4bc01..907245a3f512 100644

> > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c

> > > > @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {

> > > >    UINT64      CpuArchAttributes;

> > > >

> > > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |

> > > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))

> > > > != 0) {

> > > > -    return INVALID_CPU_ARCH_ATTRIBUTES;

> > > > -  }

> > > > -

> > > >    CpuArchAttributes = Attributes &

> > > > NONEXCLUSIVE_MEMORY_ATTRIBUTES;

> > > >

> > > >    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {

> > > > --

> > > > 2.7.4

> > 

> > _______________________________________________

> > edk2-devel mailing list

> > edk2-devel@lists.01.org

> > https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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) {