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

[Laszlo Ersek <lersek@redhat.com>]

Hi Jian,

I'm CC'ing Ard and Matt, and commenting at the bottom.

On 11/10/17 02:02, Jian J Wang wrote:
>> v5:
>>    Coding style clean-up
> 
>> v4:
>> a. Remove DoUpdate and check attributes mismatch all the time to avoid
>>    a logic hole
>> b. Add warning message if failed to update capability
>> c. Add local variable to hold new attributes to make code cleaner
> 
>> v3:
>> a. Add comment to explain more on updating memory capabilities
>> b. Fix logic hole in updating attributes
>> c. Instead of checking illegal memory space address and size, use return
>>    status of gDS->SetMemorySpaceCapabilities() to skip memory block which
>>    cannot be updated with new capabilities.
> 
>> 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 | 69 +++++++++++++++++++++++++++++-----------
>  1 file changed, 50 insertions(+), 19 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..61537838b7 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -789,8 +789,7 @@ RefreshGcdMemoryAttributesFromPaging (
>    UINT64                              BaseAddress;
>    UINT64                              PageStartAddress;
>    UINT64                              Attributes;
> -  UINT64                              Capabilities;
> -  BOOLEAN                             DoUpdate;
> +  UINT64                              NewAttributes;
>    UINTN                               Index;
>  
>    //
> @@ -802,9 +801,8 @@ RefreshGcdMemoryAttributesFromPaging (
>  
>    GetCurrentPagingContext (&PagingContext);
>  
> -  DoUpdate      = FALSE;
> -  Capabilities  = 0;
>    Attributes    = 0;
> +  NewAttributes = 0;
>    BaseAddress   = 0;
>    PageLength    = 0;
>  
> @@ -813,6 +811,34 @@ RefreshGcdMemoryAttributesFromPaging (
>        continue;
>      }
>  
> +    //
> +    // Sync the actual paging related capabilities back to GCD service first.
> +    // As a side effect (good one), this can also help to avoid unnecessary
> +    // memory map entries due to the different capabilities of the same type
> +    // memory, such as multiple RT_CODE and RT_DATA entries in memory map,
> +    // which could cause boot failure of some old Linux distro (before v4.3).
> +    //
> +    Status = gDS->SetMemorySpaceCapabilities (
> +                    MemorySpaceMap[Index].BaseAddress,
> +                    MemorySpaceMap[Index].Length,
> +                    MemorySpaceMap[Index].Capabilities |
> +                    EFI_MEMORY_PAGETYPE_MASK
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      //
> +      // If we cannot udpate the capabilities, we cannot update its
> +      // attributes either. So just simply skip current block of memory.
> +      //
> +      DEBUG ((
> +        DEBUG_WARN,
> +        "Failed to update capability: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
> +        (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> +        MemorySpaceMap[Index].Capabilities,
> +        MemorySpaceMap[Index].Capabilities | EFI_MEMORY_PAGETYPE_MASK
> +        ));
> +      continue;
> +    }
> +
>      if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
>        //
>        // Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +852,9 @@ RefreshGcdMemoryAttributesFromPaging (
>        PageLength -= (MemorySpaceMap[Index].BaseAddress - BaseAddress);
>      }
>  
> -    // Sync real page attributes to GCD
> +    //
> +    // Sync actual page attributes to GCD
> +    //
>      BaseAddress       = MemorySpaceMap[Index].BaseAddress;
>      MemorySpaceLength = MemorySpaceMap[Index].Length;
>      while (MemorySpaceLength > 0) {
> @@ -842,23 +870,26 @@ RefreshGcdMemoryAttributesFromPaging (
>          PageStartAddress  = (*PageEntry) & (UINT64)PageAttributeToMask(PageAttribute);
>          PageLength        = PageAttributeToLength (PageAttribute) - (BaseAddress - PageStartAddress);
>          Attributes        = GetAttributesFromPageEntry (PageEntry);
> -
> -        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;
> -        }
>        }
>  
>        Length = MIN (PageLength, MemorySpaceLength);
> -      if (DoUpdate) {
> -        gDS->SetMemorySpaceCapabilities (BaseAddress, Length, Capabilities);
> -        gDS->SetMemorySpaceAttributes (BaseAddress, Length, Attributes);
> -        DEBUG ((DEBUG_INFO, "Update memory space attribute: [%02d] %016lx - %016lx (%08lx -> %08lx)\r\n",
> -                             Index, BaseAddress, BaseAddress + Length - 1,
> -                             MemorySpaceMap[Index].Attributes, Attributes));
> +      if (Attributes != (MemorySpaceMap[Index].Attributes &
> +                         EFI_MEMORY_PAGETYPE_MASK)) {
> +        NewAttributes = (MemorySpaceMap[Index].Attributes &
> +                         ~EFI_MEMORY_PAGETYPE_MASK) | Attributes;
> +        Status = gDS->SetMemorySpaceAttributes (
> +                        BaseAddress,
> +                        Length,
> +                        NewAttributes
> +                        );
> +        ASSERT_EFI_ERROR (Status);
> +        DEBUG ((
> +          DEBUG_INFO,
> +          "Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
> +          (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> +          MemorySpaceMap[Index].Attributes,
> +          NewAttributes
> +          ));
>        }
>  
>        PageLength        -= Length;
> 

So, I was ready to give my R-b for this patch, but then I also wanted to
test it. I applied the patch on current edk2 master (7e2a8dfe8a9a,
"ArmPlatformPkg/PrePeiCore: seed temporary stack before entering PEI
core", 2017-10-20), and built OVMF like this:

$ build \
  -a IA32 \
  -a X64 \
  -p OvmfPkg/OvmfPkgIa32X64.dsc \
  -t GCC48 \
  -b NOOPT \
  -D SMM_REQUIRE \
  -D SECURE_BOOT_ENABLE \
  -D E1000_ENABLE \
  -D HTTP_BOOT_ENABLE

For testing I used a recent-ish upstream QEMU development build
(ae49fbbcd8e4, "Merge remote-tracking branch
'remotes/rth/tags/pull-tcg-20171025' into staging", 2017-10-25), with
the Q35 machine type (which is required by SMM anyway).

The results vary across guest OSes:

(1) Up-to-date Fedora 26 guest crashes during boot, with the following
call stack:

BUG: unable to handle kernel paging request at fffffffefe893018
Call Trace:
 ? __change_page_attr_set_clr+0xaa6/0xd70
 ? kernel_map_pages_in_pgd+0xbc/0xd0
 ? efi_call+0x58/0x90
 ? virt_efi_set_variable.part.7+0x66/0x120
 ? virt_efi_set_variable+0x4f/0x60
 ? efi_delete_dummy_variable+0x62/0x90
 ? efi_enter_virtual_mode+0x4d4/0x4e8
 ? efi_enter_virtual_mode+0x4d4/0x4e8
 ? start_kernel+0x442/0x4e6
 ? early_idt_handler_array+0x120/0x120
 ? x86_64_start_reservations+0x24/0x26
 ? x86_64_start_kernel+0x13e/0x161
 ? secondary_startup_64+0x9f/0x9f

(2) The following Windows OSes all boot successfully:

- Windows 7
- Windows Server 2008 R2
- Windows 8.1
- Windows Server 2012 R2
- Windows 10

(3) Windows Server 2016 crashes with a BSOD; reporting "ATTEMPTED WRITE
TO READONLY MEMORY".

(Without the patch, all OSes boot OK.)


I'm attaching a ZIP file with the following contents (note that I'll
attach the same file to TianoCore BZ#753 as well, because the mailing
list archive(s) don't seem to preserve attachments):

- "ovmf.pre.txt", "shell.memmap.pre.txt", "kernel.pre.txt": OVMF log,
MEMMAP command output in the UEFI shell, and Fedora 26 kernel boot log
(successful) *before* applying your patch. The kernel log is detailed
(the cmdline had "ignore_loglevel" and "efi=debug").

- "ovmf.post.txt", "shell.memmap.post.txt", "kernel.post.txt": same
files as above, but saved *after* applying your patch. This is when the
F26 kernel crashes.

- "win2016.post.png": screenshot of the Windows Server 2016 boot failure
(after the patch was applied).

Thanks,
Laszlo