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

["Wang, Jian J" <jian.j.wang@intel.com>]

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