Re: [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

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

https://bugzilla.tianocore.org/show_bug.cgi?id=763 is submitted to track this.


Thanks,
Star
-----Original Message-----
From: Wang, Jian J 
Sent: Tuesday, November 7, 2017 8:55 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of RT_CODE in memory map

Thanks for the review. And I agree that GCD.SetMemoryAttributes should be used all the time in DxeCore. Let's fix it in another patch.

> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, November 06, 2017 5:16 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star 
> <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries 
> of RT_CODE in memory map
> 
> I am ok to this code approach.
> 
> Acknowledged-by: Star Zeng <star.zeng@intel.com>
> 
> Besides, I think except GCD services, DxeCore should not call gCpu-
> >SetMemoryAttributes() directly, for example 
> >ApplyMemoryProtectionPolicy(), it
> should have no assumption of the CPU code (to set capabilities), and 
> call GCD setcapabilities first, and then call GCD setattributes since
> 14dde9e903bb9a719ebb8f3381da72b19509bc36 "MdeModulePkg/Core: Fix out- 
> of-sync issue in GCD", otherwise there may be mismatch of page 
> attributes between GCD and gCPU after 
> RefreshGcdMemoryAttributesFromPaging() by the calling gCpu->SetMemoryAttributes() in ApplyMemoryProtectionPolicy().
> 
> Anyway, that could be in a separated patch. :)
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Jian J Wang
> Sent: Friday, November 3, 2017 8:57 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Yao, Jiewen 
> <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2] [PATCH v2] UefiCpuPkg/CpuDxe: Fix multiple entries of 
> RT_CODE in memory map
> 
> > v2
> > a. Fix an issue which will cause setting capability failure if size is smaller
> >    than a page.
> 
> More than one entry of RT_CODE memory might cause boot problem for 
> some old OSs. This patch will fix this issue to keep OS compatibility as much as possible.
> 
> More detailed information, please refer to
>     https://bugzilla.tianocore.org/show_bug.cgi?id=753
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..4a7827ebc9 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -809,7 +809,9 @@ RefreshGcdMemoryAttributesFromPaging (
>    PageLength    = 0;
> 
>    for (Index = 0; Index < NumberOfDescriptors; Index++) {
> -    if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent) {
> +    if (MemorySpaceMap[Index].GcdMemoryType ==
> EfiGcdMemoryTypeNonExistent
> +        || (MemorySpaceMap[Index].BaseAddress & EFI_PAGE_MASK) != 0
> +        || (MemorySpaceMap[Index].Length & EFI_PAGE_MASK) != 0) {
>        continue;
>      }
> 
> @@ -829,6 +831,15 @@ RefreshGcdMemoryAttributesFromPaging (
>      // Sync real page attributes to GCD
>      BaseAddress       = MemorySpaceMap[Index].BaseAddress;
>      MemorySpaceLength = MemorySpaceMap[Index].Length;
> +    Capabilities      = MemorySpaceMap[Index].Capabilities |
> +                        EFI_MEMORY_PAGETYPE_MASK;
> +    Status = gDS->SetMemorySpaceCapabilities (
> +                    BaseAddress,
> +                    MemorySpaceLength,
> +                    Capabilities
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +
>      while (MemorySpaceLength > 0) {
>        if (PageLength == 0) {
>          PageEntry = GetPageTableEntry (&PagingContext, BaseAddress, 
> &PageAttribute); @@ -846,7 +857,6 @@ 
> RefreshGcdMemoryAttributesFromPaging (
>          if (Attributes != (MemorySpaceMap[Index].Attributes &
> EFI_MEMORY_PAGETYPE_MASK)) {
>            DoUpdate = TRUE;
>            Attributes |= (MemorySpaceMap[Index].Attributes & 
> ~EFI_MEMORY_PAGETYPE_MASK);
> -          Capabilities = Attributes | MemorySpaceMap[Index].Capabilities;
>          } else {
>            DoUpdate = FALSE;
>          }
> @@ -854,8 +864,8 @@ RefreshGcdMemoryAttributesFromPaging (
> 
>        Length = MIN (PageLength, MemorySpaceLength);
>        if (DoUpdate) {
> -        gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> -        gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> +        Status = gDS->SetMemorySpaceAttributes (BaseAddress, Length,
> Attributes);
> +        ASSERT_EFI_ERROR (Status);
>          DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] 
> %016lx -  %016lx (%08lx -> %08lx)\r\n",
>                               Index, BaseAddress, BaseAddress + Length - 1,
>                               MemorySpaceMap[Index].Attributes, 
> Attributes));
> --
> 2.14.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel