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

["Zeng, Star" <star.zeng@intel.com>]

Current gCpu->SetMemoryAttributes does not support getting memory attributes, and has no clear description about clearing memory attributes.

I noticed we introduced SmmMemoryAttribute(MdeModulePkg\Include\Protocol\SmmMemoryAttribute.h) protocol for heap guard feature in SMM environment.
Seemingly, we also need introduce MemoryAttribute protocol for DXE?


Thanks,
Star
-----Original Message-----
From: Zeng, Star 
Sent: Tuesday, April 3, 2018 8:59 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; 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>; Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

Mike,

Agree the problem.
We need support only RUNTIME attribute.
We need support only cache attributes.
We need support only page attributes.
We need support RUNTIME + cache + page attributes.
Do we need support the 0 Attributes case to clear page attributes?
There was discussion about the 0 Attributes case at https://lists.01.org/pipermail/edk2-devel/2018-March/023315.html.
It came to the question that should the 0 Attributes case be handled by SetMemoryAttributes() to clear the page attributes?


Thanks,
Star
-----Original Message-----
From: Kinney, Michael D
Sent: Tuesday, April 3, 2018 5:43 AM
To: Zeng, Star <star.zeng@intel.com>; Heyi Guo <heyi.guo@linaro.org>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Yi Li <phoenix.liyi@huawei.com>; Renhao Liang <liangrenhao@huawei.com>; Dong, Eric <eric.dong@intel.com>; Gao, Liming <liming.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute conversion

Star,

This commit breaks Vlv2TbltDevicePkg.

On this platform, there are 2 calls to 
gDS->SetMemorySpaceAttributes() for an MMIO
range to set only the EFI_MEMORY_RUNTIME bit.

With this commit, ConverToCpuArchAttributes()returns 0, and 0 is passed to gCpu->SetMemoryAttributes()that returns EFI_INVALID_PARAMETER on Vlv2TbltDevicePkg.

Before this commit, ConverToCpuArchAttributes() returns INVALID_CPU_ARCH_ATTRIBUTES which causes the call to gCpu->SetMemoryAttributes()to be skipped so no error is generated.

I think the right fix is to skip the call to 
gCpu->SetMemoryAttributes() if CpuArchAttributes
is 0 so a call that only sets the RUNTIME attribute can be supported along with call the set both the RUNTIME bit and other cache related bits.

I will send a patch soon with the proposed fix.

Mike

> -----Original Message-----
> From: Zeng, Star
> Sent: Sunday, April 1, 2018 10:59 PM
> 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>; Ni, 
> Ruiyu <ruiyu.ni@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH 1/1] MdeModulePkg/Gcd: Fix bug of attribute 
> conversion
> 
> Pushed at 5b91bf82c67b586b9588cbe4bbffa1588f6b5926.
> 
> Thanks,
> Star
> -----Original Message-----
> From: Heyi Guo [mailto:heyi.guo@linaro.org]
> Sent: Thursday, March 29, 2018 4:20 PM
> 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>; Wang, Jian J <jian.j.wang@intel.com>; Ni, 
> Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH 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 remove the check code
> in ConverToCpuArchAttributes(); the remaining code is enough to grab 
> the interested bits for
> Cpu->SetMemoryAttributes().
> 
> 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>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c 
> b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c index
> 77f4adb4bc01..907245a3f512 100644
> --- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> +++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
> @@ -673,11 +673,6 @@ ConverToCpuArchAttributes (  {
>    UINT64      CpuArchAttributes;
> 
> -  if ((Attributes & ~(EXCLUSIVE_MEMORY_ATTRIBUTES |
> -                      NONEXCLUSIVE_MEMORY_ATTRIBUTES))
> != 0) {
> -    return INVALID_CPU_ARCH_ATTRIBUTES;
> -  }
> -
>    CpuArchAttributes = Attributes &
> NONEXCLUSIVE_MEMORY_ATTRIBUTES;
> 
>    if ( (Attributes & EFI_MEMORY_UC) == EFI_MEMORY_UC) {
> --
> 2.7.4