* [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion @ 2018-03-29 2:31 Heyi Guo 2018-03-29 2:53 ` Zeng, Star 2018-03-29 3:19 ` Zeng, Star 0 siblings, 2 replies; 10+ messages in thread From: Heyi Guo @ 2018-03-29 2:31 UTC (permalink / raw) To: edk2-devel Cc: Heyi Guo, Yi Li, Renhao Liang, Star Zeng, Eric Dong, Michael D Kinney, Liming Gao 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion 2018-03-29 2:31 [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion Heyi Guo @ 2018-03-29 2:53 ` Zeng, Star 2018-03-29 3:19 ` Zeng, Star 1 sibling, 0 replies; 10+ messages in thread From: Zeng, Star @ 2018-03-29 2:53 UTC (permalink / raw) To: Heyi Guo, edk2-devel@lists.01.org Cc: Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming, Wang, Jian J, Yao, Jiewen, Ni, Ruiyu, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion 2018-03-29 2:31 [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion Heyi Guo 2018-03-29 2:53 ` Zeng, Star @ 2018-03-29 3:19 ` Zeng, Star 2018-03-29 4:40 ` Wang, Jian J 1 sibling, 1 reply; 10+ messages in thread From: Zeng, Star @ 2018-03-29 3:19 UTC (permalink / raw) To: Heyi Guo, edk2-devel@lists.01.org Cc: Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming, Wang, Jian J, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion 2018-03-29 3:19 ` Zeng, Star @ 2018-03-29 4:40 ` Wang, Jian J 2018-03-29 5:09 ` Guo Heyi 0 siblings, 1 reply; 10+ messages in thread From: Wang, Jian J @ 2018-03-29 4:40 UTC (permalink / raw) To: Zeng, Star, Heyi Guo, edk2-devel@lists.01.org Cc: Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion 2018-03-29 4:40 ` Wang, Jian J @ 2018-03-29 5:09 ` Guo Heyi 2018-03-29 5:44 ` Wang, Jian J 0 siblings, 1 reply; 10+ messages in thread From: Guo Heyi @ 2018-03-29 5:09 UTC (permalink / raw) To: Wang, Jian J Cc: Zeng, Star, Heyi Guo, edk2-devel@lists.01.org, Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion 2018-03-29 5:09 ` Guo Heyi @ 2018-03-29 5:44 ` Wang, Jian J 2018-03-29 5:54 ` Guo Heyi 0 siblings, 1 reply; 10+ messages in thread From: Wang, Jian J @ 2018-03-29 5:44 UTC (permalink / raw) To: Guo Heyi Cc: Zeng, Star, edk2-devel@lists.01.org, Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming 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 > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion 2018-03-29 5:44 ` Wang, Jian J @ 2018-03-29 5:54 ` Guo Heyi 2018-03-29 6:13 ` Wang, Jian J 0 siblings, 1 reply; 10+ messages in thread From: Guo Heyi @ 2018-03-29 5:54 UTC (permalink / raw) To: Wang, Jian J Cc: Guo Heyi, Zeng, Star, edk2-devel@lists.01.org, Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming 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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion 2018-03-29 5:54 ` Guo Heyi @ 2018-03-29 6:13 ` Wang, Jian J 2018-03-29 7:12 ` Ni, Ruiyu 0 siblings, 1 reply; 10+ messages in thread From: Wang, Jian J @ 2018-03-29 6:13 UTC (permalink / raw) To: Guo Heyi Cc: Zeng, Star, edk2-devel@lists.01.org, Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming, Yao, Jiewen, Ni, Ruiyu 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 > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion 2018-03-29 6:13 ` Wang, Jian J @ 2018-03-29 7:12 ` Ni, Ruiyu 2018-03-29 7:53 ` Guo Heyi 0 siblings, 1 reply; 10+ messages in thread From: Ni, Ruiyu @ 2018-03-29 7:12 UTC (permalink / raw) To: Wang, Jian J, Guo Heyi Cc: Zeng, Star, edk2-devel@lists.01.org, Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming, Yao, Jiewen 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 > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion 2018-03-29 7:12 ` Ni, Ruiyu @ 2018-03-29 7:53 ` Guo Heyi 0 siblings, 0 replies; 10+ messages in thread From: Guo Heyi @ 2018-03-29 7:53 UTC (permalink / raw) To: Ni, Ruiyu Cc: Wang, Jian J, Guo Heyi, Zeng, Star, edk2-devel@lists.01.org, Yi Li, Renhao Liang, Dong, Eric, Kinney, Michael D, Gao, Liming, Yao, Jiewen 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 > > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-29 7:47 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-03-29 2:31 [RFC 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion Heyi Guo 2018-03-29 2:53 ` Zeng, Star 2018-03-29 3:19 ` Zeng, Star 2018-03-29 4:40 ` Wang, Jian J 2018-03-29 5:09 ` Guo Heyi 2018-03-29 5:44 ` Wang, Jian J 2018-03-29 5:54 ` Guo Heyi 2018-03-29 6:13 ` Wang, Jian J 2018-03-29 7:12 ` Ni, Ruiyu 2018-03-29 7:53 ` Guo Heyi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox