[edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
Laszlo Ersek
lersek at redhat.com
Tue Aug 13 09:47:55 UTC 2019
On 08/13/19 01:22, Kinney, Michael D wrote:
>
>
>> -----Original Message-----
>> From: devel at edk2.groups.io [mailto:devel at edk2.groups.io]
>> On Behalf Of Laszlo Ersek
>> Sent: Monday, August 12, 2019 9:24 AM
>> To: devel at edk2.groups.io; Gao, Liming
>> <liming.gao at intel.com>
>> Cc: Mike Turner <miketur at microsoft.com>; Wang, Jian J
>> <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>;
>> Bi, Dandan <dandan.bi at 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 at 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 at intel.com>
>>> Cc: Hao A Wu <hao.a.wu at intel.com>
>>> Cc: Dandan Bi <dandan.bi at intel.com>
>>> Signed-off-by: Liming Gao <liming.gao at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45516): https://edk2.groups.io/g/devel/message/45516
Mute This Topic: https://groups.io/mt/32821535/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list