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

[Laszlo Ersek <lersek@redhat.com>]

On 11/08/17 11:52, Jian J Wang wrote:
>> 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 | 48 +++++++++++++++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index d312eb66f8..455c713dfc 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -789,7 +789,6 @@ RefreshGcdMemoryAttributesFromPaging (
>    UINT64                              BaseAddress;
>    UINT64                              PageStartAddress;
>    UINT64                              Attributes;
> -  UINT64                              Capabilities;
>    BOOLEAN                             DoUpdate;
>    UINTN                               Index;
>  
> @@ -803,7 +802,6 @@ RefreshGcdMemoryAttributesFromPaging (
>    GetCurrentPagingContext (&PagingContext);
>  
>    DoUpdate      = FALSE;
> -  Capabilities  = 0;
>    Attributes    = 0;
>    BaseAddress   = 0;
>    PageLength    = 0;
> @@ -813,6 +811,27 @@ 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.
> +      //

(1) Can you perhaps add a DEBUG_WARN here?

> +      continue;
> +    }
> +
>      if (MemorySpaceMap[Index].BaseAddress >= (BaseAddress + PageLength)) {
>        //
>        // Current memory space starts at a new page. Resetting PageLength will
> @@ -826,7 +845,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) {
> @@ -845,8 +866,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;
>          }

(2) To me it seems like we can remove the "DoUpdate" local variable
completely. Below, we can replace the DoUpdate check with the actual

  (Attributes != (MemorySpaceMap[Index].Attributes &
                  EFI_MEMORY_PAGETYPE_MASK))

check.

The point is that we check the EFI_MEMORY_PAGETYPE_MASK bit-field of the
entry's attributes. If they do not match Attributes, we clear the full
bit-field, and then add Attributes back in. I.e., we set the bit-field
to the desired Attributes.

> @@ -854,11 +873,20 @@ RefreshGcdMemoryAttributesFromPaging (
>  
>        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));
> +        Status = gDS->SetMemorySpaceAttributes (
> +                        BaseAddress,
> +                        Length,
> +                        (MemorySpaceMap[Index].Attributes
> +                         & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
> +                        );
> +        ASSERT_EFI_ERROR (Status);
> +        DEBUG ((
> +          DEBUG_INFO,
> +          "Update memory space attribute: [%02d] %016lx - %016lx (%016lx -> %016lx)\r\n",
> +          Index, BaseAddress, BaseAddress + Length - 1,
> +          MemorySpaceMap[Index].Attributes,
> +          (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_PAGETYPE_MASK) | Attributes
> +          ));
>        }

(3) I suggest introducing a new variable called
"NewMemorySpaceAttributes", and using that for both the debug message
and the SetMemorySpaceAttributes() call.

(4) Not closely related to this patch, but I'll mention it: the "%d"
format specifier is not right for printing UINTN values. The
32-bit/64-bit clean way to print UINTN is:

- cast the variable to UINT64 explicitly,
- print it with "%lu".

Thanks!
Laszlo

>  
>        PageLength        -= Length;
>