* [Patch] MdeModulePkg DxeCore: Fix for missing MAT update @ 2019-08-10 14:10 Liming Gao 2019-08-12 5:10 ` [edk2-devel] " Wang, Jian J 2019-08-12 16:24 ` Laszlo Ersek 0 siblings, 2 replies; 18+ messages in thread From: Liming Gao @ 2019-08-10 14:10 UTC (permalink / raw) To: devel; +Cc: Mike Turner, Jian J Wang, Hao A Wu, Dandan Bi 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. 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. This patch is cherry pick from Project Mu: https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af94016ebd391ea6f340920735a 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. + */ + + 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; + } + } + } } if (Type == AllocateMaxAddress) { -- 2.13.0.windows.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-10 14:10 [Patch] MdeModulePkg DxeCore: Fix for missing MAT update Liming Gao @ 2019-08-12 5:10 ` Wang, Jian J 2019-08-12 16:24 ` Laszlo Ersek 1 sibling, 0 replies; 18+ messages in thread From: Wang, Jian J @ 2019-08-12 5:10 UTC (permalink / raw) To: devel@edk2.groups.io, Gao, Liming; +Cc: Mike Turner, Wu, Hao A, Bi, Dandan Reviewed-by: Jian J Wang <jian.j.wang@intel.com> > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Liming Gao > Sent: Saturday, August 10, 2019 10:10 PM > To: devel@edk2.groups.io > 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: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT > update > > 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. > > 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. > > This patch is cherry pick from Project Mu: > https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af94 > 016ebd391ea6f340920735a > 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. > + */ > + > + 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; > + } > + } > + } > } > > if (Type == AllocateMaxAddress) { > -- > 2.13.0.windows.1 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-10 14:10 [Patch] MdeModulePkg DxeCore: Fix for missing MAT update Liming Gao 2019-08-12 5:10 ` [edk2-devel] " Wang, Jian J @ 2019-08-12 16:24 ` Laszlo Ersek 2019-08-12 23:22 ` Michael D Kinney 1 sibling, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2019-08-12 16:24 UTC (permalink / raw) To: devel, liming.gao; +Cc: Mike Turner, Jian J Wang, Hao A Wu, Dandan Bi 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"? > 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/a9be767d9be96af94016ebd391ea6f340920735a > 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? Thanks, Laszlo > > if (Type == AllocateMaxAddress) { > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-12 16:24 ` Laszlo Ersek @ 2019-08-12 23:22 ` Michael D Kinney 2019-08-13 9:47 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Michael D Kinney @ 2019-08-12 23:22 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Gao, Liming, Kinney, Michael D Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan > -----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. 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. The only time these types of check can be disabled is if the Memory Type Information feature is disabled. Thanks, Mike > > Thanks, > Laszlo > > > > > > if (Type == AllocateMaxAddress) { > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-12 23:22 ` Michael D Kinney @ 2019-08-13 9:47 ` Laszlo Ersek 2019-08-14 14:00 ` Liming Gao 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2019-08-13 9:47 UTC (permalink / raw) To: Kinney, Michael D, devel@edk2.groups.io, Gao, Liming Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-13 9:47 ` Laszlo Ersek @ 2019-08-14 14:00 ` Liming Gao 2019-08-14 15:12 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Liming Gao @ 2019-08-14 14:00 UTC (permalink / raw) To: Laszlo Ersek, Kinney, Michael D, devel@edk2.groups.io Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan Laszlo: > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Tuesday, August 13, 2019 5:48 PM > To: Kinney, Michael D <michael.d.kinney@intel.com>; 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/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 > >> // I cherry pick this patch from Mu project with the minimal change. I can update the comment style. > >> > >> > >>> + > >>> + 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? > gEfiMemoryTypeInformationGuid HOB is installed by platform PEI. If the platform PEI doesn't install this HOB, it means this feature is disabled. Thanks Liming > 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-14 14:00 ` Liming Gao @ 2019-08-14 15:12 ` Laszlo Ersek 2019-08-14 15:55 ` Liming Gao 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2019-08-14 15:12 UTC (permalink / raw) To: Gao, Liming, Kinney, Michael D, devel@edk2.groups.io Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan On 08/14/19 16:00, Gao, Liming wrote: > Laszlo: > > I cherry pick this patch from Mu project with the minimal change. > I can update the comment style. Yes, please. Thanks! >> 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? >> > gEfiMemoryTypeInformationGuid HOB is installed by platform PEI. Yes, that's what I meant by "no later than in the DXE IPL PEIM". > If the platform PEI doesn't install this HOB, it means this feature is disabled. PlatformPei is supposed to build the HOB in the first place, yes. However, if it doesn't, then there still may be a feedback loop between the DXE IPL PEIM and BDS. The former builds the HOB from the UEFI variable, and the latter updates the variable (as I understand) for future boots. Thanks Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-14 15:12 ` Laszlo Ersek @ 2019-08-14 15:55 ` Liming Gao 2019-08-16 15:18 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Liming Gao @ 2019-08-14 15:55 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Kinney, Michael D Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan Laszlo: > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek > Sent: Wednesday, August 14, 2019 11:13 PM > To: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io > 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/14/19 16:00, Gao, Liming wrote: > > Laszlo: > > > > > I cherry pick this patch from Mu project with the minimal change. > > I can update the comment style. > > Yes, please. Thanks! > > >> 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? > >> > > gEfiMemoryTypeInformationGuid HOB is installed by platform PEI. > > Yes, that's what I meant by "no later than in the DXE IPL PEIM". > > > If the platform PEI doesn't install this HOB, it means this feature is disabled. > > PlatformPei is supposed to build the HOB in the first place, yes. > > However, if it doesn't, then there still may be a feedback loop between > the DXE IPL PEIM and BDS. The former builds the HOB from the UEFI > variable, and the latter updates the variable (as I understand) for > future boots. > UEFI variable is created by BDS only when HOB can be found. If Platform PEIM doesn't build HOB, DxeIpl will not build HOB, then BDS will not create UEFI variable. If so, there is no HOB in every boot. Thanks Liming > Thanks > Laszlo > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-14 15:55 ` Liming Gao @ 2019-08-16 15:18 ` Laszlo Ersek 2019-08-16 15:24 ` Liming Gao 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2019-08-16 15:18 UTC (permalink / raw) To: Gao, Liming, devel@edk2.groups.io, Kinney, Michael D Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan On 08/14/19 17:55, Gao, Liming wrote: > If Platform PEIM doesn't build HOB, DxeIpl will not build HOB, My reading of the code is the opposite. If the platform PEIM does not build the HOB, then the DXE IPL PEIM will attempt to build the HOB, from the UEFI variable. At commit caa7d3a896f6, in file "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(), we have: 363 if (GetFirstGuidHob ((CONST EFI_GUID *)&gEfiMemoryTypeInformationGuid) == NULL) { 364 // 365 // Don't build GuidHob if GuidHob has been installed. 366 // 367 Status = PeiServicesLocatePpi ( 368 &gEfiPeiReadOnlyVariable2PpiGuid, 369 0, 370 NULL, 371 (VOID **)&Variable 372 ); 373 if (!EFI_ERROR (Status)) { 374 DataSize = sizeof (MemoryData); 375 Status = Variable->GetVariable ( 376 Variable, 377 EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, 378 &gEfiMemoryTypeInformationGuid, 379 NULL, 380 &DataSize, 381 &MemoryData 382 ); 383 if (!EFI_ERROR (Status) && ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) { 384 // 385 // Build the GUID'd HOB for DXE 386 // 387 BuildGuidDataHob ( 388 &gEfiMemoryTypeInformationGuid, 389 MemoryData, 390 DataSize 391 ); 392 } 393 } 394 } Thanks Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-16 15:18 ` Laszlo Ersek @ 2019-08-16 15:24 ` Liming Gao 2019-08-16 18:54 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Liming Gao @ 2019-08-16 15:24 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Kinney, Michael D Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan Laszlo: > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek > Sent: Friday, August 16, 2019 11:18 PM > To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@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/14/19 17:55, Gao, Liming wrote: > > > If Platform PEIM doesn't build HOB, DxeIpl will not build HOB, > > My reading of the code is the opposite. If the platform PEIM does not build the HOB, then the DXE IPL PEIM will attempt to build the HOB, > from the UEFI variable. > > At commit caa7d3a896f6, in file "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(), we have: > > 363 if (GetFirstGuidHob ((CONST EFI_GUID *)&gEfiMemoryTypeInformationGuid) == NULL) { > 364 // > 365 // Don't build GuidHob if GuidHob has been installed. > 366 // > 367 Status = PeiServicesLocatePpi ( > 368 &gEfiPeiReadOnlyVariable2PpiGuid, > 369 0, > 370 NULL, > 371 (VOID **)&Variable > 372 ); > 373 if (!EFI_ERROR (Status)) { > 374 DataSize = sizeof (MemoryData); > 375 Status = Variable->GetVariable ( > 376 Variable, > 377 EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, > 378 &gEfiMemoryTypeInformationGuid, > 379 NULL, > 380 &DataSize, > 381 &MemoryData > 382 ); > 383 if (!EFI_ERROR (Status) && ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) { Only when this variable exists, Hob will be built. But, if no PEIM builds Hob, BDS will not set the variable. So, there is still no HOB. Thanks Liming > 384 // > 385 // Build the GUID'd HOB for DXE > 386 // > 387 BuildGuidDataHob ( > 388 &gEfiMemoryTypeInformationGuid, > 389 MemoryData, > 390 DataSize > 391 ); > 392 } > 393 } > 394 } > > Thanks > Laszlo > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-16 15:24 ` Liming Gao @ 2019-08-16 18:54 ` Laszlo Ersek 2019-08-19 0:40 ` Liming Gao 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2019-08-16 18:54 UTC (permalink / raw) To: Gao, Liming, devel@edk2.groups.io, Kinney, Michael D Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan On 08/16/19 17:24, Gao, Liming wrote: > Laszlo: > >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >> Laszlo Ersek >> Sent: Friday, August 16, 2019 11:18 PM >> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, >> Michael D <michael.d.kinney@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/14/19 17:55, Gao, Liming wrote: >> >>> If Platform PEIM doesn't build HOB, DxeIpl will not build HOB, >> >> My reading of the code is the opposite. If the platform PEIM does not >> build the HOB, then the DXE IPL PEIM will attempt to build the HOB, >> from the UEFI variable. >> >> At commit caa7d3a896f6, in file >> "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(), we >> have: >> >> 363 if (GetFirstGuidHob ((CONST EFI_GUID *)&gEfiMemoryTypeInformationGuid) == NULL) { >> 364 // >> 365 // Don't build GuidHob if GuidHob has been installed. >> 366 // >> 367 Status = PeiServicesLocatePpi ( >> 368 &gEfiPeiReadOnlyVariable2PpiGuid, >> 369 0, >> 370 NULL, >> 371 (VOID **)&Variable >> 372 ); >> 373 if (!EFI_ERROR (Status)) { >> 374 DataSize = sizeof (MemoryData); >> 375 Status = Variable->GetVariable ( >> 376 Variable, >> 377 EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, >> 378 &gEfiMemoryTypeInformationGuid, >> 379 NULL, >> 380 &DataSize, >> 381 &MemoryData >> 382 ); >> 383 if (!EFI_ERROR (Status) && ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) { > > Only when this variable exists, Hob will be built. But, if no PEIM > builds Hob, BDS will not set the variable. > So, there is still no HOB. So how is a platform supposed to enable this feature? If PlatformPei never builds the HOB, the variable will never be created, so the DXE IPL PEIM will also not build the HOB, ever. If PlatformPei builds the HOB with static data, then BDS will set (update) the variable, yes, but the DXE IPL PEIM will ignore the variable, because PlatformPei already built the HOB. So... Is PlatformPei supposed to use the Variable PPI, check if the variable exists, and create the static HOB only if the variable is absent? ... Ugh, wait. I've actually implemented this for OVMF almost 2 years ago! And the answer to my question is "yes": https://bugzilla.tianocore.org/show_bug.cgi?id=386 https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html See the OnReadOnlyVariable2Available() function: + // + // Check if the "MemoryTypeInformation" variable exists, in the + // gEfiMemoryTypeInformationGuid namespace. + // + ReadOnlyVariable2 = Ppi; + DataSize = 0; + Status = ReadOnlyVariable2->GetVariable ( + ReadOnlyVariable2, + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, + &gEfiMemoryTypeInformationGuid, + NULL, + &DataSize, + NULL + ); + if (Status == EFI_BUFFER_TOO_SMALL) { + // + // The variable exists; the DXE IPL PEIM will build the HOB from it. + // + return EFI_SUCCESS; + } + // + // Install the default memory type information HOB. + // + BuildMemTypeInfoHob (); Apologies for forgetting about this; you are right. Too bad my work for TianoCore#386 was rejected. :( Thanks Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-16 18:54 ` Laszlo Ersek @ 2019-08-19 0:40 ` Liming Gao 2019-08-21 8:46 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Liming Gao @ 2019-08-19 0:40 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io, Kinney, Michael D Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan Laszlo: >-----Original Message----- >From: Laszlo Ersek [mailto:lersek@redhat.com] >Sent: Saturday, August 17, 2019 2:54 AM >To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, >Michael D <michael.d.kinney@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/16/19 17:24, Gao, Liming wrote: >> Laszlo: >> >>> -----Original Message----- >>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >>> Laszlo Ersek >>> Sent: Friday, August 16, 2019 11:18 PM >>> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, >>> Michael D <michael.d.kinney@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/14/19 17:55, Gao, Liming wrote: >>> >>>> If Platform PEIM doesn't build HOB, DxeIpl will not build HOB, >>> >>> My reading of the code is the opposite. If the platform PEIM does not >>> build the HOB, then the DXE IPL PEIM will attempt to build the HOB, >>> from the UEFI variable. >>> >>> At commit caa7d3a896f6, in file >>> "MdeModulePkg/Core/DxeIplPeim/DxeLoad.c", function DxeLoadCore(), >we >>> have: >>> >>> 363 if (GetFirstGuidHob ((CONST EFI_GUID >*)&gEfiMemoryTypeInformationGuid) == NULL) { >>> 364 // >>> 365 // Don't build GuidHob if GuidHob has been installed. >>> 366 // >>> 367 Status = PeiServicesLocatePpi ( >>> 368 &gEfiPeiReadOnlyVariable2PpiGuid, >>> 369 0, >>> 370 NULL, >>> 371 (VOID **)&Variable >>> 372 ); >>> 373 if (!EFI_ERROR (Status)) { >>> 374 DataSize = sizeof (MemoryData); >>> 375 Status = Variable->GetVariable ( >>> 376 Variable, >>> 377 EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, >>> 378 &gEfiMemoryTypeInformationGuid, >>> 379 NULL, >>> 380 &DataSize, >>> 381 &MemoryData >>> 382 ); >>> 383 if (!EFI_ERROR (Status) && >ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) { >> >> Only when this variable exists, Hob will be built. But, if no PEIM >> builds Hob, BDS will not set the variable. >> So, there is still no HOB. > >So how is a platform supposed to enable this feature? > >If PlatformPei never builds the HOB, the variable will never be created, >so the DXE IPL PEIM will also not build the HOB, ever. > >If PlatformPei builds the HOB with static data, then BDS will set >(update) the variable, yes, but the DXE IPL PEIM will ignore the >variable, because PlatformPei already built the HOB. > >So... Is PlatformPei supposed to use the Variable PPI, check if the >variable exists, and create the static HOB only if the variable is >absent? > >... Ugh, wait. I've actually implemented this for OVMF almost 2 years >ago! And the answer to my question is "yes": > > https://bugzilla.tianocore.org/show_bug.cgi?id=386 > https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html > >See the OnReadOnlyVariable2Available() function: > >+ // >+ // Check if the "MemoryTypeInformation" variable exists, in the >+ // gEfiMemoryTypeInformationGuid namespace. >+ // >+ ReadOnlyVariable2 = Ppi; >+ DataSize = 0; >+ Status = ReadOnlyVariable2->GetVariable ( >+ ReadOnlyVariable2, >+ EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, >+ &gEfiMemoryTypeInformationGuid, >+ NULL, >+ &DataSize, >+ NULL >+ ); >+ if (Status == EFI_BUFFER_TOO_SMALL) { >+ // >+ // The variable exists; the DXE IPL PEIM will build the HOB from it. >+ // >+ return EFI_SUCCESS; >+ } >+ // >+ // Install the default memory type information HOB. >+ // >+ BuildMemTypeInfoHob (); > >Apologies for forgetting about this; you are right. > >Too bad my work for TianoCore#386 was rejected. :( > So, this is one value to add PEI variable support in OVMF. Do you consider to approve this feature request? Thanks Liming >Thanks >Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-19 0:40 ` Liming Gao @ 2019-08-21 8:46 ` Laszlo Ersek 2019-08-21 14:14 ` Liming Gao 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2019-08-21 8:46 UTC (permalink / raw) To: Gao, Liming, devel@edk2.groups.io, Kinney, Michael D Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan Hi Liming, On 08/19/19 02:40, Gao, Liming wrote: >> ... Ugh, wait. I've actually implemented this for OVMF almost 2 years >> ago! And the answer to my question is "yes": >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=386 >> https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html >> >> See the OnReadOnlyVariable2Available() function: >> >> + // >> + // Check if the "MemoryTypeInformation" variable exists, in the >> + // gEfiMemoryTypeInformationGuid namespace. >> + // >> + ReadOnlyVariable2 = Ppi; >> + DataSize = 0; >> + Status = ReadOnlyVariable2->GetVariable ( >> + ReadOnlyVariable2, >> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, >> + &gEfiMemoryTypeInformationGuid, >> + NULL, >> + &DataSize, >> + NULL >> + ); >> + if (Status == EFI_BUFFER_TOO_SMALL) { >> + // >> + // The variable exists; the DXE IPL PEIM will build the HOB from it. >> + // >> + return EFI_SUCCESS; >> + } >> + // >> + // Install the default memory type information HOB. >> + // >> + BuildMemTypeInfoHob (); >> >> Apologies for forgetting about this; you are right. >> >> Too bad my work for TianoCore#386 was rejected. :( >> > > So, this is one value to add PEI variable support in OVMF. > Do you consider to approve this feature request? Sorry, I don't understand your question. Are you asking me if, in my opinion, we should fix TianoCore#386 (= add PEI variable support to OVMF)? If that is your question, then my answer is -- trivially -- "yes". If you read through TianoCore#386, you will see that I submitted patches for solving the BZ, twice (comment 6, comment 9). Jordan rejected my patches both times, because he thought that the same feature should be implemented in OVMF for Xen as well. I disagreed (and still disagree) -- my patches depend on a build-time flag that produces an OVMF binary that is only compatible with QEMU, and not Xen. The reason for that is that the feature (PEI variable support) cannot be implemented *sensibly* on Xen. (Xen offers no spec-conformant variable storage, and in the PEI phase, the variable store would always appear empty.) Later, Jordan tried to add dynamic pflash detection to PEI, but it didn't work in all cases, because OVMF's PEI phase doesn't run in SMM, while QEMU restricts pflash access to SMM. (In other words, pflash protection in QEMU is less flexible than SMRAM protection -- SMRAM is unlocked in PEI, but pflash is always locked.) Please see the threads linked in the BZ for the discussion. With TianoCore#1689, Anthony has started separating OVMF into different firmware platforms for Xen and QEMU. In a year or so, "OVMF for QEMU" will likely have nothing Xen-related in it (because "OVMF for Xen" will exist separately). Then we can reopen TianoCore#386 just for "OVMF for QEMU", and solve this feature. In summary, I have had patches for TianoCore#386 ready for 2+ years, they've been blocked because they only target QEMU (with a build-time flag), and I expect to upstream them finally as soon as OvmfPkg offers dedicated firmware platforms for Xen and QEMU separately. Thanks Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-21 8:46 ` Laszlo Ersek @ 2019-08-21 14:14 ` Liming Gao 2019-08-22 11:56 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Liming Gao @ 2019-08-21 14:14 UTC (permalink / raw) To: devel@edk2.groups.io, lersek@redhat.com, Kinney, Michael D Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan Laszlo: > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek > Sent: Wednesday, August 21, 2019 4:46 PM > To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@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 > > Hi Liming, > > On 08/19/19 02:40, Gao, Liming wrote: > > >> ... Ugh, wait. I've actually implemented this for OVMF almost 2 years > >> ago! And the answer to my question is "yes": > >> > >> https://bugzilla.tianocore.org/show_bug.cgi?id=386 > >> https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html > >> > >> See the OnReadOnlyVariable2Available() function: > >> > >> + // > >> + // Check if the "MemoryTypeInformation" variable exists, in the > >> + // gEfiMemoryTypeInformationGuid namespace. > >> + // > >> + ReadOnlyVariable2 = Ppi; > >> + DataSize = 0; > >> + Status = ReadOnlyVariable2->GetVariable ( > >> + ReadOnlyVariable2, > >> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, > >> + &gEfiMemoryTypeInformationGuid, > >> + NULL, > >> + &DataSize, > >> + NULL > >> + ); > >> + if (Status == EFI_BUFFER_TOO_SMALL) { > >> + // > >> + // The variable exists; the DXE IPL PEIM will build the HOB from it. > >> + // > >> + return EFI_SUCCESS; > >> + } > >> + // > >> + // Install the default memory type information HOB. > >> + // > >> + BuildMemTypeInfoHob (); > >> > >> Apologies for forgetting about this; you are right. > >> > >> Too bad my work for TianoCore#386 was rejected. :( > >> > > > > So, this is one value to add PEI variable support in OVMF. > > Do you consider to approve this feature request? > > Sorry, I don't understand your question. > > Are you asking me if, in my opinion, we should fix TianoCore#386 (= add > PEI variable support to OVMF)? Yes. Fix TianoCore#386 is my suggestion. > > If that is your question, then my answer is -- trivially -- "yes". If > you read through TianoCore#386, you will see that I submitted patches > for solving the BZ, twice (comment 6, comment 9). I see your patch link in BZ. > > Jordan rejected my patches both times, because he thought that the same > feature should be implemented in OVMF for Xen as well. I disagreed (and > still disagree) -- my patches depend on a build-time flag that produces > an OVMF binary that is only compatible with QEMU, and not Xen. The > reason for that is that the feature (PEI variable support) cannot be > implemented *sensibly* on Xen. (Xen offers no spec-conformant variable > storage, and in the PEI phase, the variable store would always appear > empty.) Later, Jordan tried to add dynamic pflash detection to PEI, but > it didn't work in all cases, because OVMF's PEI phase doesn't run in > SMM, while QEMU restricts pflash access to SMM. (In other words, pflash > protection in QEMU is less flexible than SMRAM protection -- SMRAM is > unlocked in PEI, but pflash is always locked.) Please see the threads > linked in the BZ for the discussion. Thanks for you detail information. I miss them before. > > With TianoCore#1689, Anthony has started separating OVMF into different > firmware platforms for Xen and QEMU. In a year or so, "OVMF for QEMU" > will likely have nothing Xen-related in it (because "OVMF for Xen" will > exist separately). Then we can reopen TianoCore#386 just for "OVMF for > QEMU", and solve this feature. TianoCore#1689 is in edk2 stable tag 201908 feature planning. Dose it still catch 201808 stable tag? > > In summary, I have had patches for TianoCore#386 ready for 2+ years, > they've been blocked because they only target QEMU (with a build-time > flag), and I expect to upstream them finally as soon as OvmfPkg offers > dedicated firmware platforms for Xen and QEMU separately. How about add BZ386 for 201911 stable tag? Thanks Liming > > Thanks > Laszlo > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-21 14:14 ` Liming Gao @ 2019-08-22 11:56 ` Laszlo Ersek 2019-08-22 14:52 ` Liming Gao 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2019-08-22 11:56 UTC (permalink / raw) To: Gao, Liming Cc: devel@edk2.groups.io, Kinney, Michael D, Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan, Anthony Perard, Jordan Justen (Intel address) (+Anthony, +Jordan) On 08/21/19 16:14, Gao, Liming wrote: > Laszlo: > >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek >> Sent: Wednesday, August 21, 2019 4:46 PM >> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@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 >> >> Hi Liming, >> >> On 08/19/19 02:40, Gao, Liming wrote: >> >>>> ... Ugh, wait. I've actually implemented this for OVMF almost 2 years >>>> ago! And the answer to my question is "yes": >>>> >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=386 >>>> https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html >>>> >>>> See the OnReadOnlyVariable2Available() function: >>>> >>>> + // >>>> + // Check if the "MemoryTypeInformation" variable exists, in the >>>> + // gEfiMemoryTypeInformationGuid namespace. >>>> + // >>>> + ReadOnlyVariable2 = Ppi; >>>> + DataSize = 0; >>>> + Status = ReadOnlyVariable2->GetVariable ( >>>> + ReadOnlyVariable2, >>>> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, >>>> + &gEfiMemoryTypeInformationGuid, >>>> + NULL, >>>> + &DataSize, >>>> + NULL >>>> + ); >>>> + if (Status == EFI_BUFFER_TOO_SMALL) { >>>> + // >>>> + // The variable exists; the DXE IPL PEIM will build the HOB from it. >>>> + // >>>> + return EFI_SUCCESS; >>>> + } >>>> + // >>>> + // Install the default memory type information HOB. >>>> + // >>>> + BuildMemTypeInfoHob (); >>>> >>>> Apologies for forgetting about this; you are right. >>>> >>>> Too bad my work for TianoCore#386 was rejected. :( >>>> >>> >>> So, this is one value to add PEI variable support in OVMF. >>> Do you consider to approve this feature request? >> >> Sorry, I don't understand your question. >> >> Are you asking me if, in my opinion, we should fix TianoCore#386 (= add >> PEI variable support to OVMF)? > > Yes. Fix TianoCore#386 is my suggestion. > >> >> If that is your question, then my answer is -- trivially -- "yes". If >> you read through TianoCore#386, you will see that I submitted patches >> for solving the BZ, twice (comment 6, comment 9). > > I see your patch link in BZ. > >> >> Jordan rejected my patches both times, because he thought that the same >> feature should be implemented in OVMF for Xen as well. I disagreed (and >> still disagree) -- my patches depend on a build-time flag that produces >> an OVMF binary that is only compatible with QEMU, and not Xen. The >> reason for that is that the feature (PEI variable support) cannot be >> implemented *sensibly* on Xen. (Xen offers no spec-conformant variable >> storage, and in the PEI phase, the variable store would always appear >> empty.) Later, Jordan tried to add dynamic pflash detection to PEI, but >> it didn't work in all cases, because OVMF's PEI phase doesn't run in >> SMM, while QEMU restricts pflash access to SMM. (In other words, pflash >> protection in QEMU is less flexible than SMRAM protection -- SMRAM is >> unlocked in PEI, but pflash is always locked.) Please see the threads >> linked in the BZ for the discussion. > > Thanks for you detail information. I miss them before. > >> >> With TianoCore#1689, Anthony has started separating OVMF into different >> firmware platforms for Xen and QEMU. In a year or so, "OVMF for QEMU" >> will likely have nothing Xen-related in it (because "OVMF for Xen" will >> exist separately). Then we can reopen TianoCore#386 just for "OVMF for >> QEMU", and solve this feature. > > TianoCore#1689 is in edk2 stable tag 201908 feature planning. > Dose it still catch 201808 stable tag? Yes, I pushed Anthony's v5 series yesterday, and closed TianoCore#1689. TianoCore#1689 is also listed on the feature planning page, as "New OvmfXen platform with Xen PVH support": https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning > >> >> In summary, I have had patches for TianoCore#386 ready for 2+ years, >> they've been blocked because they only target QEMU (with a build-time >> flag), and I expect to upstream them finally as soon as OvmfPkg offers >> dedicated firmware platforms for Xen and QEMU separately. > > How about add BZ386 for 201911 stable tag? That would make me very happy, but I don't think we can do it so quickly (in three months). Here's why: TianoCore#1689 has introduced the new, dedicated OVMF Xen platform, but it has not removed the Xen parts from the "traditional" OVMF platform. The separation between "OVMF for Xen" and "OVMF for QEMU" is not complete yet. This is the current status: - OvmfXen: - runs in Xen HVM guests - runs in Xen PVH guests - traditional OVMF - runs in Xen HVM guests - runs in QEMU/KVM guests The desired (and agreed upon) end-status is the following: - OvmfXen: - runs in Xen HVM guests - runs in Xen PVH guests - traditional OVMF - runs in QEMU/KVM guests (This will match the platform separation that we have under ArmVirtPkg too.) Anthony plans to remove the Xen HVM code from traditional OVMF in a year or so, if I understand correctly. That's when "traditional OVMF" will become QEMU/KVM-only, completing the separation. And that's when I expect we can fix TianoCore#386 for QEMU/KVM *only*, without Jordan NACKing the patch set. Basically, "PcdMemVarstoreEmuEnable" would be constant TRUE in OvmfXen (inherited from the OVMF DEC file), and it would be exposed to the platform builder via "-D MEM_VARSTORE_EMU_ENABLE=FALSE" in the "OVMF for QEMU/KVM" platform. FaultTolerantWritePei and VariablePei would be included in the "OVMF for QEMU/KVM" DSC files only, and so on. Thanks Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-22 11:56 ` Laszlo Ersek @ 2019-08-22 14:52 ` Liming Gao 2019-08-23 12:40 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Liming Gao @ 2019-08-22 14:52 UTC (permalink / raw) To: Laszlo Ersek Cc: devel@edk2.groups.io, Kinney, Michael D, Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan, Anthony Perard, Justen, Jordan L Laszlo: > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, August 22, 2019 7:56 PM > To: Gao, Liming <liming.gao@intel.com> > Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; 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>; Anthony Perard > <anthony.perard@citrix.com>; Justen, Jordan L <jordan.l.justen@intel.com> > Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update > > (+Anthony, +Jordan) > > On 08/21/19 16:14, Gao, Liming wrote: > > Laszlo: > > > >> -----Original Message----- > >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Laszlo Ersek > >> Sent: Wednesday, August 21, 2019 4:46 PM > >> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@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 > >> > >> Hi Liming, > >> > >> On 08/19/19 02:40, Gao, Liming wrote: > >> > >>>> ... Ugh, wait. I've actually implemented this for OVMF almost 2 years > >>>> ago! And the answer to my question is "yes": > >>>> > >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=386 > >>>> https://lists.01.org/pipermail/edk2-devel/2017-November/018312.html > >>>> > >>>> See the OnReadOnlyVariable2Available() function: > >>>> > >>>> + // > >>>> + // Check if the "MemoryTypeInformation" variable exists, in the > >>>> + // gEfiMemoryTypeInformationGuid namespace. > >>>> + // > >>>> + ReadOnlyVariable2 = Ppi; > >>>> + DataSize = 0; > >>>> + Status = ReadOnlyVariable2->GetVariable ( > >>>> + ReadOnlyVariable2, > >>>> + EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME, > >>>> + &gEfiMemoryTypeInformationGuid, > >>>> + NULL, > >>>> + &DataSize, > >>>> + NULL > >>>> + ); > >>>> + if (Status == EFI_BUFFER_TOO_SMALL) { > >>>> + // > >>>> + // The variable exists; the DXE IPL PEIM will build the HOB from it. > >>>> + // > >>>> + return EFI_SUCCESS; > >>>> + } > >>>> + // > >>>> + // Install the default memory type information HOB. > >>>> + // > >>>> + BuildMemTypeInfoHob (); > >>>> > >>>> Apologies for forgetting about this; you are right. > >>>> > >>>> Too bad my work for TianoCore#386 was rejected. :( > >>>> > >>> > >>> So, this is one value to add PEI variable support in OVMF. > >>> Do you consider to approve this feature request? > >> > >> Sorry, I don't understand your question. > >> > >> Are you asking me if, in my opinion, we should fix TianoCore#386 (= add > >> PEI variable support to OVMF)? > > > > Yes. Fix TianoCore#386 is my suggestion. > > > >> > >> If that is your question, then my answer is -- trivially -- "yes". If > >> you read through TianoCore#386, you will see that I submitted patches > >> for solving the BZ, twice (comment 6, comment 9). > > > > I see your patch link in BZ. > > > >> > >> Jordan rejected my patches both times, because he thought that the same > >> feature should be implemented in OVMF for Xen as well. I disagreed (and > >> still disagree) -- my patches depend on a build-time flag that produces > >> an OVMF binary that is only compatible with QEMU, and not Xen. The > >> reason for that is that the feature (PEI variable support) cannot be > >> implemented *sensibly* on Xen. (Xen offers no spec-conformant variable > >> storage, and in the PEI phase, the variable store would always appear > >> empty.) Later, Jordan tried to add dynamic pflash detection to PEI, but > >> it didn't work in all cases, because OVMF's PEI phase doesn't run in > >> SMM, while QEMU restricts pflash access to SMM. (In other words, pflash > >> protection in QEMU is less flexible than SMRAM protection -- SMRAM is > >> unlocked in PEI, but pflash is always locked.) Please see the threads > >> linked in the BZ for the discussion. > > > > Thanks for you detail information. I miss them before. > > > >> > >> With TianoCore#1689, Anthony has started separating OVMF into different > >> firmware platforms for Xen and QEMU. In a year or so, "OVMF for QEMU" > >> will likely have nothing Xen-related in it (because "OVMF for Xen" will > >> exist separately). Then we can reopen TianoCore#386 just for "OVMF for > >> QEMU", and solve this feature. > > > > TianoCore#1689 is in edk2 stable tag 201908 feature planning. > > Dose it still catch 201808 stable tag? > > Yes, I pushed Anthony's v5 series yesterday, and closed TianoCore#1689. > > TianoCore#1689 is also listed on the feature planning page, as "New > OvmfXen platform with Xen PVH support": > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Release-Planning > Thanks. I have seen this feature is done. > > > >> > >> In summary, I have had patches for TianoCore#386 ready for 2+ years, > >> they've been blocked because they only target QEMU (with a build-time > >> flag), and I expect to upstream them finally as soon as OvmfPkg offers > >> dedicated firmware platforms for Xen and QEMU separately. > > > > How about add BZ386 for 201911 stable tag? > > That would make me very happy, but I don't think we can do it so quickly > (in three months). Here's why: TianoCore#1689 has introduced the new, > dedicated OVMF Xen platform, but it has not removed the Xen parts from > the "traditional" OVMF platform. The separation between "OVMF for Xen" > and "OVMF for QEMU" is not complete yet. > > This is the current status: > > - OvmfXen: > - runs in Xen HVM guests > - runs in Xen PVH guests > > - traditional OVMF > - runs in Xen HVM guests > - runs in QEMU/KVM guests > > The desired (and agreed upon) end-status is the following: > > - OvmfXen: > - runs in Xen HVM guests > - runs in Xen PVH guests > > - traditional OVMF > - runs in QEMU/KVM guests > > (This will match the platform separation that we have under ArmVirtPkg too.) > > Anthony plans to remove the Xen HVM code from traditional OVMF in a year > or so, if I understand correctly. That's when "traditional OVMF" will > become QEMU/KVM-only, completing the separation. And that's when I > expect we can fix TianoCore#386 for QEMU/KVM *only*, without Jordan > NACKing the patch set. > Ok. Is there BZ for this change? The contributor can propose his work planning. Then, I will update edk2 feature planning wiki. > Basically, "PcdMemVarstoreEmuEnable" would be constant TRUE in OvmfXen > (inherited from the OVMF DEC file), and it would be exposed to the > platform builder via "-D MEM_VARSTORE_EMU_ENABLE=FALSE" in the "OVMF for > QEMU/KVM" platform. FaultTolerantWritePei and VariablePei would be > included in the "OVMF for QEMU/KVM" DSC files only, and so on. > I think this design meets with the request BZ386. Thanks Liming > Thanks > Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update 2019-08-22 14:52 ` Liming Gao @ 2019-08-23 12:40 ` Laszlo Ersek 0 siblings, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2019-08-23 12:40 UTC (permalink / raw) To: Gao, Liming Cc: devel@edk2.groups.io, Kinney, Michael D, Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan, Anthony Perard, Justen, Jordan L On 08/22/19 16:52, Gao, Liming wrote: >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Thursday, August 22, 2019 7:56 PM >> To: Gao, Liming <liming.gao@intel.com> >> Cc: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; 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>; Anthony Perard >> <anthony.perard@citrix.com>; Justen, Jordan L <jordan.l.justen@intel.com> >> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update >> This is the current status: >> >> - OvmfXen: >> - runs in Xen HVM guests >> - runs in Xen PVH guests >> >> - traditional OVMF >> - runs in Xen HVM guests >> - runs in QEMU/KVM guests >> >> The desired (and agreed upon) end-status is the following: >> >> - OvmfXen: >> - runs in Xen HVM guests >> - runs in Xen PVH guests >> >> - traditional OVMF >> - runs in QEMU/KVM guests >> >> (This will match the platform separation that we have under ArmVirtPkg too.) >> >> Anthony plans to remove the Xen HVM code from traditional OVMF in a year >> or so, if I understand correctly. That's when "traditional OVMF" will >> become QEMU/KVM-only, completing the separation. And that's when I >> expect we can fix TianoCore#386 for QEMU/KVM *only*, without Jordan >> NACKing the patch set. >> > Ok. Is there BZ for this change? The contributor can propose his work planning. > Then, I will update edk2 feature planning wiki. I've now reported: https://bugzilla.tianocore.org/show_bug.cgi?id=2122 and assigned it to Anthony. (Anthony, I hope that's OK with you.) >> Basically, "PcdMemVarstoreEmuEnable" would be constant TRUE in OvmfXen >> (inherited from the OVMF DEC file), and it would be exposed to the >> platform builder via "-D MEM_VARSTORE_EMU_ENABLE=FALSE" in the "OVMF for >> QEMU/KVM" platform. FaultTolerantWritePei and VariablePei would be >> included in the "OVMF for QEMU/KVM" DSC files only, and so on. >> > I think this design meets with the request BZ386. I've also reopened https://bugzilla.tianocore.org/show_bug.cgi?id=386 now (moving it back to CONFIRMED state, from RESOLVED|WONTFIX), and I've made it dependent on TianoCore#2122. Thanks, Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <15B9950E072DB087.17773@groups.io>]
* Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update [not found] <15B9950E072DB087.17773@groups.io> @ 2019-08-10 14:16 ` Liming Gao 0 siblings, 0 replies; 18+ messages in thread From: Liming Gao @ 2019-08-10 14:16 UTC (permalink / raw) To: devel@edk2.groups.io, Gao, Liming Cc: Mike Turner, Wang, Jian J, Wu, Hao A, Bi, Dandan Hi, all This is one bug fix. I plan to catch it for 201808 stable tag. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1978 Thanks Liming > -----Original Message----- > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Liming Gao > Sent: Saturday, August 10, 2019 10:10 PM > To: devel@edk2.groups.io > 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: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update > > 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. > > 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. > > This patch is cherry pick from Project Mu: > https://github.com/microsoft/mu_basecore/commit/a9be767d9be96af94016ebd391ea6f340920735a > 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. > + */ > + > + 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; > + } > + } > + } > } > > if (Type == AllocateMaxAddress) { > -- > 2.13.0.windows.1 > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-08-23 12:40 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-10 14:10 [Patch] MdeModulePkg DxeCore: Fix for missing MAT update Liming Gao
2019-08-12 5:10 ` [edk2-devel] " Wang, Jian J
2019-08-12 16:24 ` Laszlo Ersek
2019-08-12 23:22 ` Michael D Kinney
2019-08-13 9:47 ` Laszlo Ersek
2019-08-14 14:00 ` Liming Gao
2019-08-14 15:12 ` Laszlo Ersek
2019-08-14 15:55 ` Liming Gao
2019-08-16 15:18 ` Laszlo Ersek
2019-08-16 15:24 ` Liming Gao
2019-08-16 18:54 ` Laszlo Ersek
2019-08-19 0:40 ` Liming Gao
2019-08-21 8:46 ` Laszlo Ersek
2019-08-21 14:14 ` Liming Gao
2019-08-22 11:56 ` Laszlo Ersek
2019-08-22 14:52 ` Liming Gao
2019-08-23 12:40 ` Laszlo Ersek
[not found] <15B9950E072DB087.17773@groups.io>
2019-08-10 14:16 ` Liming Gao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox