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

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

[Heyi Guo]

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

We don't see any reason to forbid combining CPU arch attributes and
non-CPU-arch attributes when calling gDS->SetMemorySpaceAttributes(),
so we change ConverToCpuArchAttributes() to only check if there is
valid CPU arch attributes in the input "Attribute" parameter and just
ignore other attributes.

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>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Zeng, Star]

Cc Jian, Jiewen and Ruiyu.

Thanks,
Star
-----Original Message-----
From: Heyi Guo [mailto:heyi.guo@linaro.org] 
Sent: Thursday, March 29, 2018 10:32 AM
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>
Subject: [RFC 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 change ConverToCpuArchAttributes() to only check if there is valid CPU arch attributes in the input "Attribute" parameter and just ignore other attributes.

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>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Zeng, Star]

The code before 14dde9e903bb9a719ebb8f3381da72b19509bc36 can allow the attribute to be a combination of CPU arch attribute and other attributes, for example, UC + RUNTIME.
But current code could not allow the case, that seems a regression in the code.
So, I agree the statement.

Jian, will you please provide the special case you are awared?


Thanks,
Star
-----Original Message-----
From: Heyi Guo [mailto:heyi.guo@linaro.org] 
Sent: Thursday, March 29, 2018 10:32 AM
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>
Subject: [RFC 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 change ConverToCpuArchAttributes() to only check if there is valid CPU arch attributes in the input "Attribute" parameter and just ignore other attributes.

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>
---
 MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Wang, Jian J]

I agree. The only issue here is that case "Attributes == 0" is also excluded in this patch.
I think 0 should be CPU arch supported attributes.

Regards,
Jian

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, March 29, 2018 11:20 AM
> 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>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> 
> The code before 14dde9e903bb9a719ebb8f3381da72b19509bc36 can allow the
> attribute to be a combination of CPU arch attribute and other attributes, for
> example, UC + RUNTIME.
> But current code could not allow the case, that seems a regression in the code.
> So, I agree the statement.
> 
> Jian, will you please provide the special case you are awared?
> 
> 
> Thanks,
> Star
> -----Original Message-----
> From: Heyi Guo [mailto:heyi.guo@linaro.org]
> Sent: Thursday, March 29, 2018 10:32 AM
> 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>
> Subject: [RFC 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 change
> ConverToCpuArchAttributes() to only check if there is valid CPU arch attributes in
> the input "Attribute" parameter and just ignore other attributes.
> 
> 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>
> ---
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index 77f4adb4bc01..2ababdd14cc6
> 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -673,8 +673,8 @@ ConverToCpuArchAttributes (  {
>    UINT64      CpuArchAttributes;
> 
> -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
> +  if ((Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> +                     NONEXCLUSIVE_MEMORY_ATTRIBUTES)) == 0) {
>      return INVALID_CPU_ARCH_ATTRIBUTES;
>    }
> 
> --
> 2.7.4

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

[Guo Heyi]

Hi Jian,

I excluded CpuArchAttributes == 0 on purpose, for I don't know the expected
behaviour of gCpu->SetMemoryAttributes() if CpuArchAttributes == 0. Does that
mean we need to keep the cache attribute and remove memory protection
attributes?

If so, then it seems we don't need the check at all.

Thanks,

Heyi

On Thu, Mar 29, 2018 at 04:40:33AM +0000, Wang, Jian J wrote:
> I agree. The only issue here is that case "Attributes == 0" is also excluded in this patch.
> I think 0 should be CPU arch supported attributes.
> 
> Regards,
> Jian
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Thursday, March 29, 2018 11:20 AM
> > 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>; Zeng, Star <star.zeng@intel.com>
> > Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> > 
> > The code before 14dde9e903bb9a719ebb8f3381da72b19509bc36 can allow the
> > attribute to be a combination of CPU arch attribute and other attributes, for
> > example, UC + RUNTIME.
> > But current code could not allow the case, that seems a regression in the code.
> > So, I agree the statement.
> > 
> > Jian, will you please provide the special case you are awared?
> > 
> > 
> > Thanks,
> > Star
> > -----Original Message-----
> > From: Heyi Guo [mailto:heyi.guo@linaro.org]
> > Sent: Thursday, March 29, 2018 10:32 AM
> > 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>
> > Subject: [RFC 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 change
> > ConverToCpuArchAttributes() to only check if there is valid CPU arch attributes in
> > the input "Attribute" parameter and just ignore other attributes.
> > 
> > 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>
> > ---
> >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index 77f4adb4bc01..2ababdd14cc6
> > 100644
> > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > @@ -673,8 +673,8 @@ ConverToCpuArchAttributes (  {
> >    UINT64      CpuArchAttributes;
> > 
> > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
> > +  if ((Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> > +                     NONEXCLUSIVE_MEMORY_ATTRIBUTES)) == 0) {
> >      return INVALID_CPU_ARCH_ATTRIBUTES;
> >    }
> > 
> > --
> > 2.7.4
> 

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

[Wang, Jian J]

Hi Heyi,

Yeah, you're probably right. Page attributes are allowed to be cleared but
we have no separate parameter or interface to differentiate such situation.
I think there's flaw in the interface design. But it's hard to change it now.

Regards,
Jian


> -----Original Message-----
> From: Guo Heyi [mailto:heyi.guo@linaro.org]
> Sent: Thursday, March 29, 2018 1:09 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-
> devel@lists.01.org; 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>
> Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> 
> Hi Jian,
> 
> I excluded CpuArchAttributes == 0 on purpose, for I don't know the expected
> behaviour of gCpu->SetMemoryAttributes() if CpuArchAttributes == 0. Does that
> mean we need to keep the cache attribute and remove memory protection
> attributes?
> 
> If so, then it seems we don't need the check at all.
> 
> Thanks,
> 
> Heyi
> 
> On Thu, Mar 29, 2018 at 04:40:33AM +0000, Wang, Jian J wrote:
> > I agree. The only issue here is that case "Attributes == 0" is also excluded in this
> patch.
> > I think 0 should be CPU arch supported attributes.
> >
> > Regards,
> > Jian
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Thursday, March 29, 2018 11:20 AM
> > > 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>; Zeng, Star <star.zeng@intel.com>
> > > Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> > >
> > > The code before 14dde9e903bb9a719ebb8f3381da72b19509bc36 can allow
> the
> > > attribute to be a combination of CPU arch attribute and other attributes, for
> > > example, UC + RUNTIME.
> > > But current code could not allow the case, that seems a regression in the
> code.
> > > So, I agree the statement.
> > >
> > > Jian, will you please provide the special case you are awared?
> > >
> > >
> > > Thanks,
> > > Star
> > > -----Original Message-----
> > > From: Heyi Guo [mailto:heyi.guo@linaro.org]
> > > Sent: Thursday, March 29, 2018 10:32 AM
> > > 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>
> > > Subject: [RFC 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
> change
> > > ConverToCpuArchAttributes() to only check if there is valid CPU arch
> attributes in
> > > the input "Attribute" parameter and just ignore other attributes.
> > >
> > > 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>
> > > ---
> > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index 77f4adb4bc01..2ababdd14cc6
> > > 100644
> > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > @@ -673,8 +673,8 @@ ConverToCpuArchAttributes (  {
> > >    UINT64      CpuArchAttributes;
> > >
> > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
> > > +  if ((Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > +                     NONEXCLUSIVE_MEMORY_ATTRIBUTES)) == 0) {
> > >      return INVALID_CPU_ARCH_ATTRIBUTES;
> > >    }
> > >
> > > --
> > > 2.7.4
> >

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

[Guo Heyi]

Agree.

And what's your suggestion to fix the current issue?

Regards,
Heyi


On Thu, Mar 29, 2018 at 05:44:16AM +0000, Wang, Jian J wrote:
> Hi Heyi,
> 
> Yeah, you're probably right. Page attributes are allowed to be cleared but
> we have no separate parameter or interface to differentiate such situation.
> I think there's flaw in the interface design. But it's hard to change it now.
> 
> Regards,
> Jian
> 
> 
> > -----Original Message-----
> > From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > Sent: Thursday, March 29, 2018 1:09 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-
> > devel@lists.01.org; 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>
> > Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> > 
> > Hi Jian,
> > 
> > I excluded CpuArchAttributes == 0 on purpose, for I don't know the expected
> > behaviour of gCpu->SetMemoryAttributes() if CpuArchAttributes == 0. Does that
> > mean we need to keep the cache attribute and remove memory protection
> > attributes?
> > 
> > If so, then it seems we don't need the check at all.
> > 
> > Thanks,
> > 
> > Heyi
> > 
> > On Thu, Mar 29, 2018 at 04:40:33AM +0000, Wang, Jian J wrote:
> > > I agree. The only issue here is that case "Attributes == 0" is also excluded in this
> > patch.
> > > I think 0 should be CPU arch supported attributes.
> > >
> > > Regards,
> > > Jian
> > >
> > > > -----Original Message-----
> > > > From: Zeng, Star
> > > > Sent: Thursday, March 29, 2018 11:20 AM
> > > > 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>; Zeng, Star <star.zeng@intel.com>
> > > > Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> > > >
> > > > The code before 14dde9e903bb9a719ebb8f3381da72b19509bc36 can allow
> > the
> > > > attribute to be a combination of CPU arch attribute and other attributes, for
> > > > example, UC + RUNTIME.
> > > > But current code could not allow the case, that seems a regression in the
> > code.
> > > > So, I agree the statement.
> > > >
> > > > Jian, will you please provide the special case you are awared?
> > > >
> > > >
> > > > Thanks,
> > > > Star
> > > > -----Original Message-----
> > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]
> > > > Sent: Thursday, March 29, 2018 10:32 AM
> > > > 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>
> > > > Subject: [RFC 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
> > change
> > > > ConverToCpuArchAttributes() to only check if there is valid CPU arch
> > attributes in
> > > > the input "Attribute" parameter and just ignore other attributes.
> > > >
> > > > 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>
> > > > ---
> > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index 77f4adb4bc01..2ababdd14cc6
> > > > 100644
> > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > @@ -673,8 +673,8 @@ ConverToCpuArchAttributes (  {
> > > >    UINT64      CpuArchAttributes;
> > > >
> > > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
> > > > +  if ((Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > +                     NONEXCLUSIVE_MEMORY_ATTRIBUTES)) == 0) {
> > > >      return INVALID_CPU_ARCH_ATTRIBUTES;
> > > >    }
> > > >
> > > > --
> > > > 2.7.4
> > >

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

[Wang, Jian J]

I think you can simply remove the check.

Jiewen and Ruiyu, do you have any different thoughts?

Regards,
Jian


> -----Original Message-----
> From: Guo Heyi [mailto:heyi.guo@linaro.org]
> Sent: Thursday, March 29, 2018 1:55 PM
> To: Wang, Jian J <jian.j.wang@intel.com>
> Cc: Guo Heyi <heyi.guo@linaro.org>; Zeng, Star <star.zeng@intel.com>; edk2-
> devel@lists.01.org; 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>
> Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> 
> Agree.
> 
> And what's your suggestion to fix the current issue?
> 
> Regards,
> Heyi
> 
> 
> On Thu, Mar 29, 2018 at 05:44:16AM +0000, Wang, Jian J wrote:
> > Hi Heyi,
> >
> > Yeah, you're probably right. Page attributes are allowed to be cleared but
> > we have no separate parameter or interface to differentiate such situation.
> > I think there's flaw in the interface design. But it's hard to change it now.
> >
> > Regards,
> > Jian
> >
> >
> > > -----Original Message-----
> > > From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > > Sent: Thursday, March 29, 2018 1:09 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>;
> edk2-
> > > devel@lists.01.org; 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>
> > > Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> > >
> > > Hi Jian,
> > >
> > > I excluded CpuArchAttributes == 0 on purpose, for I don't know the expected
> > > behaviour of gCpu->SetMemoryAttributes() if CpuArchAttributes == 0. Does
> that
> > > mean we need to keep the cache attribute and remove memory protection
> > > attributes?
> > >
> > > If so, then it seems we don't need the check at all.
> > >
> > > Thanks,
> > >
> > > Heyi
> > >
> > > On Thu, Mar 29, 2018 at 04:40:33AM +0000, Wang, Jian J wrote:
> > > > I agree. The only issue here is that case "Attributes == 0" is also excluded in
> this
> > > patch.
> > > > I think 0 should be CPU arch supported attributes.
> > > >
> > > > Regards,
> > > > Jian
> > > >
> > > > > -----Original Message-----
> > > > > From: Zeng, Star
> > > > > Sent: Thursday, March 29, 2018 11:20 AM
> > > > > 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>; Zeng, Star <star.zeng@intel.com>
> > > > > Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> conversion
> > > > >
> > > > > The code before 14dde9e903bb9a719ebb8f3381da72b19509bc36 can
> allow
> > > the
> > > > > attribute to be a combination of CPU arch attribute and other attributes,
> for
> > > > > example, UC + RUNTIME.
> > > > > But current code could not allow the case, that seems a regression in the
> > > code.
> > > > > So, I agree the statement.
> > > > >
> > > > > Jian, will you please provide the special case you are awared?
> > > > >
> > > > >
> > > > > Thanks,
> > > > > Star
> > > > > -----Original Message-----
> > > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]
> > > > > Sent: Thursday, March 29, 2018 10:32 AM
> > > > > 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>
> > > > > Subject: [RFC 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
> > > change
> > > > > ConverToCpuArchAttributes() to only check if there is valid CPU arch
> > > attributes in
> > > > > the input "Attribute" parameter and just ignore other attributes.
> > > > >
> > > > > 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>
> > > > > ---
> > > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index
> 77f4adb4bc01..2ababdd14cc6
> > > > > 100644
> > > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > @@ -673,8 +673,8 @@ ConverToCpuArchAttributes (  {
> > > > >    UINT64      CpuArchAttributes;
> > > > >
> > > > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
> > > > > +  if ((Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > > +                     NONEXCLUSIVE_MEMORY_ATTRIBUTES)) == 0) {
> > > > >      return INVALID_CPU_ARCH_ATTRIBUTES;
> > > > >    }
> > > > >
> > > > > --
> > > > > 2.7.4
> > > >

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

[Ni, Ruiyu]

I agree to remove the check.
Code only grabs the interested bit and calls Cpu->SetMemoryAttributes().

Thanks/Ray

> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, March 29, 2018 2:13 PM
> To: Guo Heyi <heyi.guo@linaro.org>
> Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; 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>; Yao,
> Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> 
> I think you can simply remove the check.
> 
> Jiewen and Ruiyu, do you have any different thoughts?
> 
> Regards,
> Jian
> 
> 
> > -----Original Message-----
> > From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > Sent: Thursday, March 29, 2018 1:55 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>
> > Cc: Guo Heyi <heyi.guo@linaro.org>; Zeng, Star <star.zeng@intel.com>;
> > edk2- devel@lists.01.org; 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>
> > Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > conversion
> >
> > Agree.
> >
> > And what's your suggestion to fix the current issue?
> >
> > Regards,
> > Heyi
> >
> >
> > On Thu, Mar 29, 2018 at 05:44:16AM +0000, Wang, Jian J wrote:
> > > Hi Heyi,
> > >
> > > Yeah, you're probably right. Page attributes are allowed to be
> > > cleared but we have no separate parameter or interface to differentiate
> such situation.
> > > I think there's flaw in the interface design. But it's hard to change it now.
> > >
> > > Regards,
> > > Jian
> > >
> > >
> > > > -----Original Message-----
> > > > From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > > > Sent: Thursday, March 29, 2018 1:09 PM
> > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo
> > > > <heyi.guo@linaro.org>;
> > edk2-
> > > > devel@lists.01.org; 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>
> > > > Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > > > conversion
> > > >
> > > > Hi Jian,
> > > >
> > > > I excluded CpuArchAttributes == 0 on purpose, for I don't know the
> > > > expected behaviour of gCpu->SetMemoryAttributes() if
> > > > CpuArchAttributes == 0. Does
> > that
> > > > mean we need to keep the cache attribute and remove memory
> > > > protection attributes?
> > > >
> > > > If so, then it seems we don't need the check at all.
> > > >
> > > > Thanks,
> > > >
> > > > Heyi
> > > >
> > > > On Thu, Mar 29, 2018 at 04:40:33AM +0000, Wang, Jian J wrote:
> > > > > I agree. The only issue here is that case "Attributes == 0" is
> > > > > also excluded in
> > this
> > > > patch.
> > > > > I think 0 should be CPU arch supported attributes.
> > > > >
> > > > > Regards,
> > > > > Jian
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zeng, Star
> > > > > > Sent: Thursday, March 29, 2018 11:20 AM
> > > > > > 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>; Zeng, Star
> > > > > > <star.zeng@intel.com>
> > > > > > Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > conversion
> > > > > >
> > > > > > The code before 14dde9e903bb9a719ebb8f3381da72b19509bc36
> can
> > allow
> > > > the
> > > > > > attribute to be a combination of CPU arch attribute and other
> > > > > > attributes,
> > for
> > > > > > example, UC + RUNTIME.
> > > > > > But current code could not allow the case, that seems a
> > > > > > regression in the
> > > > code.
> > > > > > So, I agree the statement.
> > > > > >
> > > > > > Jian, will you please provide the special case you are awared?
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Star
> > > > > > -----Original Message-----
> > > > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]
> > > > > > Sent: Thursday, March 29, 2018 10:32 AM
> > > > > > 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>
> > > > > > Subject: [RFC 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
> > > > change
> > > > > > ConverToCpuArchAttributes() to only check if there is valid
> > > > > > CPU arch
> > > > attributes in
> > > > > > the input "Attribute" parameter and just ignore other attributes.
> > > > > >
> > > > > > 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>
> > > > > > ---
> > > > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
> > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index
> > 77f4adb4bc01..2ababdd14cc6
> > > > > > 100644
> > > > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > > @@ -673,8 +673,8 @@ ConverToCpuArchAttributes (  {
> > > > > >    UINT64      CpuArchAttributes;
> > > > > >
> > > > > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
> > > > > > +  if ((Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > > > +                     NONEXCLUSIVE_MEMORY_ATTRIBUTES)) == 0) {
> > > > > >      return INVALID_CPU_ARCH_ATTRIBUTES;
> > > > > >    }
> > > > > >
> > > > > > --
> > > > > > 2.7.4
> > > > >

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

[Guo Heyi]

That makes sense to me; I'll send a patch to make the change.

Thanks,
Heyi

On Thu, Mar 29, 2018 at 07:12:16AM +0000, Ni, Ruiyu wrote:
> I agree to remove the check.
> Code only grabs the interested bit and calls Cpu->SetMemoryAttributes().
> 
> Thanks/Ray
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Thursday, March 29, 2018 2:13 PM
> > To: Guo Heyi <heyi.guo@linaro.org>
> > Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; 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>; Yao,
> > Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> > Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
> > 
> > I think you can simply remove the check.
> > 
> > Jiewen and Ruiyu, do you have any different thoughts?
> > 
> > Regards,
> > Jian
> > 
> > 
> > > -----Original Message-----
> > > From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > > Sent: Thursday, March 29, 2018 1:55 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > Cc: Guo Heyi <heyi.guo@linaro.org>; Zeng, Star <star.zeng@intel.com>;
> > > edk2- devel@lists.01.org; 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>
> > > Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > > conversion
> > >
> > > Agree.
> > >
> > > And what's your suggestion to fix the current issue?
> > >
> > > Regards,
> > > Heyi
> > >
> > >
> > > On Thu, Mar 29, 2018 at 05:44:16AM +0000, Wang, Jian J wrote:
> > > > Hi Heyi,
> > > >
> > > > Yeah, you're probably right. Page attributes are allowed to be
> > > > cleared but we have no separate parameter or interface to differentiate
> > such situation.
> > > > I think there's flaw in the interface design. But it's hard to change it now.
> > > >
> > > > Regards,
> > > > Jian
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Guo Heyi [mailto:heyi.guo@linaro.org]
> > > > > Sent: Thursday, March 29, 2018 1:09 PM
> > > > > To: Wang, Jian J <jian.j.wang@intel.com>
> > > > > Cc: Zeng, Star <star.zeng@intel.com>; Heyi Guo
> > > > > <heyi.guo@linaro.org>;
> > > edk2-
> > > > > devel@lists.01.org; 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>
> > > > > Subject: Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > > > > conversion
> > > > >
> > > > > Hi Jian,
> > > > >
> > > > > I excluded CpuArchAttributes == 0 on purpose, for I don't know the
> > > > > expected behaviour of gCpu->SetMemoryAttributes() if
> > > > > CpuArchAttributes == 0. Does
> > > that
> > > > > mean we need to keep the cache attribute and remove memory
> > > > > protection attributes?
> > > > >
> > > > > If so, then it seems we don't need the check at all.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Heyi
> > > > >
> > > > > On Thu, Mar 29, 2018 at 04:40:33AM +0000, Wang, Jian J wrote:
> > > > > > I agree. The only issue here is that case "Attributes == 0" is
> > > > > > also excluded in
> > > this
> > > > > patch.
> > > > > > I think 0 should be CPU arch supported attributes.
> > > > > >
> > > > > > Regards,
> > > > > > Jian
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Zeng, Star
> > > > > > > Sent: Thursday, March 29, 2018 11:20 AM
> > > > > > > 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>; Zeng, Star
> > > > > > > <star.zeng@intel.com>
> > > > > > > Subject: RE: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute
> > > conversion
> > > > > > >
> > > > > > > The code before 14dde9e903bb9a719ebb8f3381da72b19509bc36
> > can
> > > allow
> > > > > the
> > > > > > > attribute to be a combination of CPU arch attribute and other
> > > > > > > attributes,
> > > for
> > > > > > > example, UC + RUNTIME.
> > > > > > > But current code could not allow the case, that seems a
> > > > > > > regression in the
> > > > > code.
> > > > > > > So, I agree the statement.
> > > > > > >
> > > > > > > Jian, will you please provide the special case you are awared?
> > > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Star
> > > > > > > -----Original Message-----
> > > > > > > From: Heyi Guo [mailto:heyi.guo@linaro.org]
> > > > > > > Sent: Thursday, March 29, 2018 10:32 AM
> > > > > > > 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>
> > > > > > > Subject: [RFC 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
> > > > > change
> > > > > > > ConverToCpuArchAttributes() to only check if there is valid
> > > > > > > CPU arch
> > > > > attributes in
> > > > > > > the input "Attribute" parameter and just ignore other attributes.
> > > > > > >
> > > > > > > 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>
> > > > > > > ---
> > > > > > >  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 4 ++--
> > > > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > > > b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index
> > > 77f4adb4bc01..2ababdd14cc6
> > > > > > > 100644
> > > > > > > --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > > > +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> > > > > > > @@ -673,8 +673,8 @@ ConverToCpuArchAttributes (  {
> > > > > > >    UINT64      CpuArchAttributes;
> > > > > > >
> > > > > > > -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > > > > -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES)) != 0) {
> > > > > > > +  if ((Attributes & (EXCLUSIVE_MEMORY_ATTRIBUTES |
> > > > > > > +                     NONEXCLUSIVE_MEMORY_ATTRIBUTES)) == 0) {
> > > > > > >      return INVALID_CPU_ARCH_ATTRIBUTES;
> > > > > > >    }
> > > > > > >
> > > > > > > --
> > > > > > > 2.7.4
> > > > > >