Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update

["Laszlo Ersek" <lersek@redhat.com>]

On 08/13/19 01:22, Kinney, Michael D wrote:
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io]
>> On Behalf Of Laszlo Ersek
>> Sent: Monday, August 12, 2019 9:24 AM
>> To: devel@edk2.groups.io; Gao, Liming
>> <liming.gao@intel.com>
>> Cc: Mike Turner <miketur@microsoft.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
>> Bi, Dandan <dandan.bi@intel.com>
>> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore:
>> Fix for missing MAT update
>>
>> On 08/10/19 16:10, Liming Gao wrote:
>>> From: Mike Turner <miketur@microsoft.com>
>>>
>>> The Fpdt driver (FirmwarePerformanceDxe) saves a memory
>> address across
>>> reboots, and then does an AllocatePage for that memory
>> address.
>>> If, on this boot, that memory comes from a Runtime
>> memory bucket, the
>>> MAT table is not updated. This causes Windows to boot
>> into Recovery.
>>
>> (1) What is "MAT"?
> 
> Memory Attributes Table (EFI_MEMORY_ATTRIBUTES_TABLE)
> 
>>
>>> This patch blocks the memory manager from changing the
>> page from a
>>> special bucket to a different memory type.  Once the
>> buckets are
>>> allocated, we freeze the memory ranges for the OS, and
>> fragmenting the
>>> special buckets will cause errors resuming from
>> hibernate.
>>
>> (2) My understanding is that CoreConvertPages() will only
>> hand out the requested pages if those pages are currently
>> free. I suggest clarifying the commit message that the
>> intent is to prevent the allocation of otherwise *free*
>> pages, if the allocation would fragment special buckets.
>>
>> (3) I don't understand the conjunction "and". I would
>> understand if the statement were:
>>
>>     Once the buckets are allocated, we freeze the memory
>> ranges for the
>>     OS, *because* fragmenting the special buckets *would*
>> cause errors
>>     resuming from hibernate.
>>
>> Is this the intent?
>>
>>>
>>> This patch is cherry pick from Project Mu:
>>>
>> https://github.com/microsoft/mu_basecore/commit/a9be767d9
>> be96af94016eb
>>> d391ea6f340920735a
>>> With the minor change,
>>> 1. Update commit message format to keep the message in
>> 80 characters one line.
>>> 2. Remove // MU_CHANGE comments in source code.
>>>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Hao A Wu <hao.a.wu@intel.com>
>>> Cc: Dandan Bi <dandan.bi@intel.com>
>>> Signed-off-by: Liming Gao <liming.gao@intel.com>
>>> ---
>>>  MdeModulePkg/Core/Dxe/Mem/Page.c | 43
>>> ++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 37 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> b/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> index bd9e116aa5..637518889d 100644
>>> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
>>> @@ -1265,12 +1265,13 @@ CoreInternalAllocatePages (
>>>    IN BOOLEAN                NeedGuard
>>>    )
>>>  {
>>> -  EFI_STATUS      Status;
>>> -  UINT64          Start;
>>> -  UINT64          NumberOfBytes;
>>> -  UINT64          End;
>>> -  UINT64          MaxAddress;
>>> -  UINTN           Alignment;
>>> +  EFI_STATUS       Status;
>>> +  UINT64           Start;
>>> +  UINT64           NumberOfBytes;
>>> +  UINT64           End;
>>> +  UINT64           MaxAddress;
>>> +  UINTN            Alignment;
>>> +  EFI_MEMORY_TYPE  CheckType;
>>>
>>>    if ((UINT32)Type >= MaxAllocateType) {
>>>      return EFI_INVALID_PARAMETER;
>>> @@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
>>>    // if (Start + NumberOfBytes) rolls over 0 or
>>>    // if Start is above MAX_ALLOC_ADDRESS or
>>>    // if End is above MAX_ALLOC_ADDRESS,
>>> +  // if Start..End overlaps any tracked
>> MemoryTypeStatistics range
>>>    // return EFI_NOT_FOUND.
>>>    //
>>>    if (Type == AllocateAddress) {
>>> @@ -1336,6 +1338,35 @@ CoreInternalAllocatePages (
>>>          (End > MaxAddress)) {
>>>        return EFI_NOT_FOUND;
>>>      }
>>> +
>>> +    // Problem summary
>>> +
>>> +    /*
>>> +    A driver is allowed to call AllocatePages using an
>> AllocateAddress type.  This type of
>>> +    AllocatePage request the exact physical address if
>> it is not used.  The existing code
>>> +    will allow this request even in 'special' pages.
>> The problem with this is that the
>>> +    reason to have 'special' pages for OS
>> hibernate/resume is defeated as memory is
>>> +    fragmented.
>>> +    */
>>
>> (4) This comment style is not native to edk2.
>>
>> I think the "problem summary" line should be removed, and
>> the actual problem statement should use the following
>> comment style:
>>
>>   //
>>   // blah
>>   //
>>
>>
>>> +
>>> +    for (CheckType = (EFI_MEMORY_TYPE) 0; CheckType <
>> EfiMaxMemoryType; CheckType++) {
>>> +      if (MemoryType != CheckType &&
>>> +          mMemoryTypeStatistics[CheckType].Special &&
>>> +
>> mMemoryTypeStatistics[CheckType].NumberOfPages > 0) {
>>> +        if (Start >=
>> mMemoryTypeStatistics[CheckType].BaseAddress &&
>>> +            Start <=
>> mMemoryTypeStatistics[CheckType].MaximumAddress) {
>>> +          return EFI_NOT_FOUND;
>>> +        }
>>> +        if (End >=
>> mMemoryTypeStatistics[CheckType].BaseAddress &&
>>> +            End <=
>> mMemoryTypeStatistics[CheckType].MaximumAddress) {
>>> +          return EFI_NOT_FOUND;
>>> +        }
>>> +        if (Start <
>> mMemoryTypeStatistics[CheckType].BaseAddress &&
>>> +            End   >
>> mMemoryTypeStatistics[CheckType].MaximumAddress) {
>>> +          return EFI_NOT_FOUND;
>>> +        }
>>> +      }
>>> +    }
>>>    }
>>
>> (5) Checking for overlap (i.e., whether the intersection
>> is non-empty) can be done more simply (i.e., with fewer
>> comparisons in the worst case, and with less code):
>>
>>   if (MAX (Start,
>> mMemoryTypeStatistics[CheckType].BaseAddress) <=
>>       MIN (End,
>> mMemoryTypeStatistics[CheckType].MaximumAddress)) {
>>     return EFI_NOT_FOUND;
>>   }
>>
>> but the proposed intersection check is technically right
>> already, IMO, so there's no strong need to update it.
>>
>> (Somewhat unusually for this kind of comparison, all four
>> boundaries are inclusive here.)
>>
>> (6) Both the commit message and the code comment state
>> that this problem is specific to S4. Therefore, we can
>> distinguish three cases:
>>
>> (6a) Platform doesn't support (or doesn't enable) S4 at
>> all.
>>
>> (6b) Platform supports & enables S4, and this is a normal
>> boot.
>>
>> (6c) Platform supports & enables S4, and this is actually
>> an S4 resume.
>>
>> The code being proposed applies to all three cases. Is
>> that the intent?
>> Shouldn't the new check be made conditional on (6c) --
>> from the boot mode HOB --, or at least on (6b)||(6c) --
>> i.e. the check should be disabled if S4 is absent
>> entirely?
> 
> Hi Laszlo,
> 
> I think this check should be added for all cases. Without
> this change, memory allocations using type AllocateAddress
> Is allowed to allocate in the unused portion of a bin.  This
> means the memory allocations are not consist with the memory
> map returned by GetMemoryMap() that shows the entire bin as
> allocated.  The only exception that is allowed is if an
> AllocateAddress request is made to the unused portion of a
> bin where the request and the bin have the same MemoryType.

Thanks for the explanation. It helps! I understand now.

> The references to S4 here are the use case that fails.  This
> failure is root caused to an inconsistent behavior of the 
> core memory services themselves when type AllocateAddress is
> used.  

Can the commit message be framed accordingly, please?

The main issue is apparently with the UEFI memory map -- the UEFI memory
map reflects the pre-allocated bins, but the actual allocations at fixed
addresses may go out of sync with that. Everything else, such as:

- EFI_MEMORY_ATTRIBUTES_TABLE (page protections) being out of sync,

- S4 failing

are just symptoms / consequences.

> The only time these types of check can be disabled is if the
> Memory Type Information feature is disabled.

The gEfiMemoryTypeInformationGuid HOB is supposed to be built -- if it
is built at all -- no later than in the DXE IPL PEIM (if VariablePei is
included in the platform, and the underlying UEFI variable exists). Is
that correct?

Becase if it is correct, then I think the check could be based (in the
DXE core) on the presence of this HOB.

Thank you!
Laszlo