Re: [edk2] [PATCH 2/2] MdeModulePkg/Core: Fix out-of-sync issue in GCD

2017-09-20 Thread Wang, Jian J 
Sure. I'll submit a new patch after enough validation. Thanks for the review.

-Original Message-
From: Zeng, Star 
Sent: Wednesday, September 20, 2017 5:30 PM
To: Wang, Jian J <jian.j.w...@intel.com>; edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek 
<ler...@redhat.com>; Yao, Jiewen <jiewen@intel.com>; Dong, Eric 
<eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: RE: [edk2] [PATCH 2/2] MdeModulePkg/Core: Fix out-of-sync issue in GCD

Reviewed-by: Star Zeng <star.z...@intel.com> for this patch.

Please notice that the MemoryProtection.c is using gCpu->SetMemoryAttributes 
but not GCD SetMemorySpaceAttributes.
You should need update it to use GCD SetMemorySpaceAttributes, you can have 
separated patch to cover it.


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J 
Wang
Sent: Tuesday, September 19, 2017 2:10 PM
To: edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek 
<ler...@redhat.com>; Yao, Jiewen <jiewen@intel.com>; Dong, Eric 
<eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: [edk2] [PATCH 2/2] MdeModulePkg/Core: Fix out-of-sync issue in GCD

>From GCD perspective, its SetMemorySpaceAttributes() method doesn't accept 
>page related attributes. That means users cannot use it to change page 
>attributes, and have to turn to CPU arch protocol to do it, which is not be 
>allowed by PI spec.

Cc: Jiewen Yao <jiewen@intel.com>
Cc: Eric Dong <eric.d...@intel.com>
Cc: Star Zeng <star.z...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Michael Kinney <michael.d.kin...@intel.com>
Suggested-by: Jiewen Yao <jiewen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 45 -
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c 
index a06f8bb77c..e9d1d5b612 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -40,6 +40,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #define PRESENT_MEMORY_ATTRIBUTES (EFI_RESOURCE_ATTRIBUTE_PRESENT)
 
+#define EXCLUSIVE_MEMORY_ATTRIBUTES   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
+   EFI_MEMORY_WT | EFI_MEMORY_WB | \
+   EFI_MEMORY_WP | EFI_MEMORY_UCE)
+
+#define NONEXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | \
+EFI_MEMORY_RO)
+
 #define INVALID_CPU_ARCH_ATTRIBUTES   0x
 
 //
@@ -654,28 +661,30 @@ ConverToCpuArchAttributes (
   UINT64 Attributes
   )
 {
-  if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
-return EFI_MEMORY_UC;
-  }
+  UINT64  CpuArchAttributes;
 
-  if ( (Attributes & EFI_MEMORY_WC ) == EFI_MEMORY_WC) {
-return EFI_MEMORY_WC;
+  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
+  NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
+return INVALID_CPU_ARCH_ATTRIBUTES;
   }
 
-  if ( (Attributes & EFI_MEMORY_WT ) == EFI_MEMORY_WT) {
-return EFI_MEMORY_WT;
-  }
-
-  if ( (Attributes & EFI_MEMORY_WB) == EFI_MEMORY_WB) {
-return EFI_MEMORY_WB;
-  }
-
-  if ( (Attributes & EFI_MEMORY_WP) == EFI_MEMORY_WP) {
-return EFI_MEMORY_WP;
-  }
-
-  return INVALID_CPU_ARCH_ATTRIBUTES;
+  CpuArchAttributes = Attributes & NONEXCLUSIVE_MEMORY_ATTRIBUTES;
 
+  if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
+CpuArchAttributes |= EFI_MEMORY_UC;  } else if ( (Attributes & 
+ EFI_MEMORY_WC ) == EFI_MEMORY_WC) {
+CpuArchAttributes |= EFI_MEMORY_WC;  } else if ( (Attributes & 
+ EFI_MEMORY_WT ) == EFI_MEMORY_WT) {
+CpuArchAttributes |= EFI_MEMORY_WT;  } else if ( (Attributes & 
+ EFI_MEMORY_WB) == EFI_MEMORY_WB) {
+CpuArchAttributes |= EFI_MEMORY_WB;  } else if ( (Attributes & 
+ EFI_MEMORY_UCE) == EFI_MEMORY_UCE) {
+CpuArchAttributes |= EFI_MEMORY_UCE;  } else if ( (Attributes & 
+ EFI_MEMORY_WP) == EFI_MEMORY_WP) {
+CpuArchAttributes |= EFI_MEMORY_WP;  }
+
+  return CpuArchAttributes;
 }
 
 
--
2.14.1.windows.1

___
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

Re: [edk2] [PATCH 2/2] MdeModulePkg/Core: Fix out-of-sync issue in GCD

2017-09-20 Thread Zeng, Star 
Reviewed-by: Star Zeng <star.z...@intel.com> for this patch.

Please notice that the MemoryProtection.c is using gCpu->SetMemoryAttributes 
but not GCD SetMemorySpaceAttributes.
You should need update it to use GCD SetMemorySpaceAttributes, you can have 
separated patch to cover it.


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J 
Wang
Sent: Tuesday, September 19, 2017 2:10 PM
To: edk2-devel@lists.01.org
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek 
<ler...@redhat.com>; Yao, Jiewen <jiewen@intel.com>; Dong, Eric 
<eric.d...@intel.com>; Zeng, Star <star.z...@intel.com>
Subject: [edk2] [PATCH 2/2] MdeModulePkg/Core: Fix out-of-sync issue in GCD

>From GCD perspective, its SetMemorySpaceAttributes() method doesn't accept 
>page related attributes. That means users cannot use it to change page 
>attributes, and have to turn to CPU arch protocol to do it, which is not be 
>allowed by PI spec.

Cc: Jiewen Yao <jiewen@intel.com>
Cc: Eric Dong <eric.d...@intel.com>
Cc: Star Zeng <star.z...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Michael Kinney <michael.d.kin...@intel.com>
Suggested-by: Jiewen Yao <jiewen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.w...@intel.com>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 45 -
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c 
index a06f8bb77c..e9d1d5b612 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -40,6 +40,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #define PRESENT_MEMORY_ATTRIBUTES (EFI_RESOURCE_ATTRIBUTE_PRESENT)
 
+#define EXCLUSIVE_MEMORY_ATTRIBUTES   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
+   EFI_MEMORY_WT | EFI_MEMORY_WB | \
+   EFI_MEMORY_WP | EFI_MEMORY_UCE)
+
+#define NONEXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | \
+EFI_MEMORY_RO)
+
 #define INVALID_CPU_ARCH_ATTRIBUTES   0x
 
 //
@@ -654,28 +661,30 @@ ConverToCpuArchAttributes (
   UINT64 Attributes
   )
 {
-  if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
-return EFI_MEMORY_UC;
-  }
+  UINT64  CpuArchAttributes;
 
-  if ( (Attributes & EFI_MEMORY_WC ) == EFI_MEMORY_WC) {
-return EFI_MEMORY_WC;
+  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
+  NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
+return INVALID_CPU_ARCH_ATTRIBUTES;
   }
 
-  if ( (Attributes & EFI_MEMORY_WT ) == EFI_MEMORY_WT) {
-return EFI_MEMORY_WT;
-  }
-
-  if ( (Attributes & EFI_MEMORY_WB) == EFI_MEMORY_WB) {
-return EFI_MEMORY_WB;
-  }
-
-  if ( (Attributes & EFI_MEMORY_WP) == EFI_MEMORY_WP) {
-return EFI_MEMORY_WP;
-  }
-
-  return INVALID_CPU_ARCH_ATTRIBUTES;
+  CpuArchAttributes = Attributes & NONEXCLUSIVE_MEMORY_ATTRIBUTES;
 
+  if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
+CpuArchAttributes |= EFI_MEMORY_UC;  } else if ( (Attributes & 
+ EFI_MEMORY_WC ) == EFI_MEMORY_WC) {
+CpuArchAttributes |= EFI_MEMORY_WC;  } else if ( (Attributes & 
+ EFI_MEMORY_WT ) == EFI_MEMORY_WT) {
+CpuArchAttributes |= EFI_MEMORY_WT;  } else if ( (Attributes & 
+ EFI_MEMORY_WB) == EFI_MEMORY_WB) {
+CpuArchAttributes |= EFI_MEMORY_WB;  } else if ( (Attributes & 
+ EFI_MEMORY_UCE) == EFI_MEMORY_UCE) {
+CpuArchAttributes |= EFI_MEMORY_UCE;  } else if ( (Attributes & 
+ EFI_MEMORY_WP) == EFI_MEMORY_WP) {
+CpuArchAttributes |= EFI_MEMORY_WP;  }
+
+  return CpuArchAttributes;
 }
 
 
--
2.14.1.windows.1

___
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

[edk2] [PATCH 2/2] MdeModulePkg/Core: Fix out-of-sync issue in GCD

2017-09-19 Thread Jian J Wang 
>From GCD perspective, its SetMemorySpaceAttributes() method doesn't
accept page related attributes. That means users cannot use it to
change page attributes, and have to turn to CPU arch protocol to do
it, which is not be allowed by PI spec.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Star Zeng 
Cc: Laszlo Ersek 
Cc: Michael Kinney 
Suggested-by: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 45 -
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index a06f8bb77c..e9d1d5b612 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -40,6 +40,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 
 #define PRESENT_MEMORY_ATTRIBUTES (EFI_RESOURCE_ATTRIBUTE_PRESENT)
 
+#define EXCLUSIVE_MEMORY_ATTRIBUTES   (EFI_MEMORY_UC | EFI_MEMORY_WC | \
+   EFI_MEMORY_WT | EFI_MEMORY_WB | \
+   EFI_MEMORY_WP | EFI_MEMORY_UCE)
+
+#define NONEXCLUSIVE_MEMORY_ATTRIBUTES (EFI_MEMORY_XP | EFI_MEMORY_RP | \
+EFI_MEMORY_RO)
+
 #define INVALID_CPU_ARCH_ATTRIBUTES   0x
 
 //
@@ -654,28 +661,30 @@ ConverToCpuArchAttributes (
   UINT64 Attributes
   )
 {
-  if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
-return EFI_MEMORY_UC;
-  }
+  UINT64  CpuArchAttributes;
 
-  if ( (Attributes & EFI_MEMORY_WC ) == EFI_MEMORY_WC) {
-return EFI_MEMORY_WC;
+  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
+  NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
+return INVALID_CPU_ARCH_ATTRIBUTES;
   }
 
-  if ( (Attributes & EFI_MEMORY_WT ) == EFI_MEMORY_WT) {
-return EFI_MEMORY_WT;
-  }
-
-  if ( (Attributes & EFI_MEMORY_WB) == EFI_MEMORY_WB) {
-return EFI_MEMORY_WB;
-  }
-
-  if ( (Attributes & EFI_MEMORY_WP) == EFI_MEMORY_WP) {
-return EFI_MEMORY_WP;
-  }
-
-  return INVALID_CPU_ARCH_ATTRIBUTES;
+  CpuArchAttributes = Attributes & NONEXCLUSIVE_MEMORY_ATTRIBUTES;
 
+  if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
+CpuArchAttributes |= EFI_MEMORY_UC;
+  } else if ( (Attributes & EFI_MEMORY_WC ) == EFI_MEMORY_WC) {
+CpuArchAttributes |= EFI_MEMORY_WC;
+  } else if ( (Attributes & EFI_MEMORY_WT ) == EFI_MEMORY_WT) {
+CpuArchAttributes |= EFI_MEMORY_WT;
+  } else if ( (Attributes & EFI_MEMORY_WB) == EFI_MEMORY_WB) {
+CpuArchAttributes |= EFI_MEMORY_WB;
+  } else if ( (Attributes & EFI_MEMORY_UCE) == EFI_MEMORY_UCE) {
+CpuArchAttributes |= EFI_MEMORY_UCE;
+  } else if ( (Attributes & EFI_MEMORY_WP) == EFI_MEMORY_WP) {
+CpuArchAttributes |= EFI_MEMORY_WP;
+  }
+
+  return CpuArchAttributes;
 }
 
 
-- 
2.14.1.windows.1

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