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

Message ID 1522290692-99585-1-git-send-email-heyi.guo@linaro.org
State New
Headers show
Series
  • [edk2,RFC,1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion
Related show

Commit Message

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

-- 
2.7.4

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

Comments

Zeng, Star March 29, 2018, 2:53 a.m. | #1
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Zeng, Star March 29, 2018, 3:19 a.m. | #2
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Wang, Jian J March 29, 2018, 4:40 a.m. | #3
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


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 29, 2018, 5:09 a.m. | #4
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

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Wang, Jian J March 29, 2018, 5:44 a.m. | #5
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

> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 29, 2018, 5:54 a.m. | #6
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

> > >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Wang, Jian J March 29, 2018, 6:13 a.m. | #7
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

> > > >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ni, Ruiyu March 29, 2018, 7:12 a.m. | #8
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

> > > > >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo March 29, 2018, 7:53 a.m. | #9
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

> > > > > >

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

Patch

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;
   }