Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
On 08/22/19 16:52, Gao, Liming wrote: >> -Original Message- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Thursday, August 22, 2019 7:56 PM >> 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 >> 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#46301): https://edk2.groups.io/g/devel/message/46301 Mute This Topic: https://groups.io/mt/32821535/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
Laszlo:
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, August 22, 2019 7:56 PM
> 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
> 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 ; devel@edk2.groups.io; Kinney,
> >> Michael D
> >> Cc: Mike Turner ; Wang, Jian J
> >> ; Wu, Hao A ; Bi, Dandan
> >>
> >> 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,
> >>>> +,
> >>>> +NULL,
> >>>> +,
> >>>> +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
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 ; devel@edk2.groups.io; Kinney,
>> Michael D
>> Cc: Mike Turner ; Wang, Jian J
>> ; Wu, Hao A ; Bi, Dandan
>>
>> 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,
>>>> +,
>>>> +NULL,
>>>> +,
>>>> +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
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
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 ; devel@edk2.groups.io; Kinney, Michael
> D
> Cc: Mike Turner ; Wang, Jian J
> ; Wu, Hao A ; Bi, Dandan
>
> 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,
> >> +,
> >> +NULL,
> >> +,
> >> +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
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46154): https://edk2.groups.io/g/devel/message/46154
Mute This Topic: https://groups.io/mt/32821535/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
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,
>> +,
>> +NULL,
>> +,
>> +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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46141): https://edk2.groups.io/g/devel/message/46141
Mute This Topic: https://groups.io/mt/32821535/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
Laszlo:
>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Saturday, August 17, 2019 2:54 AM
>To: Gao, Liming ; devel@edk2.groups.io; Kinney,
>Michael D
>Cc: Mike Turner ; Wang, Jian J
>; Wu, Hao A ; Bi, Dandan
>
>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 ; devel@edk2.groups.io; Kinney,
>>> Michael D
>>> Cc: Mike Turner ; Wang, Jian J
>>> ; Wu, Hao A ; Bi, Dandan
>>>
>>> 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:
>>>
>>>363if (GetFirstGuidHob ((CONST EFI_GUID
>*)) == NULL) {
>>>364 //
>>>365 // Don't build GuidHob if GuidHob has been installed.
>>>366 //
>>>367 Status = PeiServicesLocatePpi (
>>>368 ,
>>>369 0,
>>>370 NULL,
>>>371 (VOID **)
>>>372 );
>>>373 if (!EFI_ERROR (Status)) {
>>>374DataSize = sizeof (MemoryData);
>>>375Status = Variable->GetVariable (
>>>376 Variable,
>>>377
>>> EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
>>>378 ,
>>>379 NULL,
>>>380 ,
>>>381
>>>382 );
>>>383if (!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,
>+,
>+NULL,
>+,
>+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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#46016): https://edk2.groups.io/g/devel/message/46016
Mute This Topic: https://groups.io/mt/32821535/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
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 ; devel@edk2.groups.io; Kinney,
>> Michael D
>> Cc: Mike Turner ; Wang, Jian J
>> ; Wu, Hao A ; Bi, Dandan
>>
>> 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:
>>
>>363if (GetFirstGuidHob ((CONST EFI_GUID
>> *)) == NULL) {
>>364 //
>>365 // Don't build GuidHob if GuidHob has been installed.
>>366 //
>>367 Status = PeiServicesLocatePpi (
>>368 ,
>>369 0,
>>370 NULL,
>>371 (VOID **)
>>372 );
>>373 if (!EFI_ERROR (Status)) {
>>374DataSize = sizeof (MemoryData);
>>375Status = Variable->GetVariable (
>>376 Variable,
>>377 EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
>>378 ,
>>379 NULL,
>>380 ,
>>381
>>382 );
>>383if (!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,
+,
+NULL,
+,
+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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45849): https://edk2.groups.io/g/devel/message/45849
Mute This Topic: https://groups.io/mt/32821535/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
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 ; devel@edk2.groups.io; Kinney, Michael
> D
> Cc: Mike Turner ; Wang, Jian J
> ; Wu, Hao A ; Bi, Dandan
>
> 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:
>
>363if (GetFirstGuidHob ((CONST EFI_GUID
> *)) == NULL) {
>364 //
>365 // Don't build GuidHob if GuidHob has been installed.
>366 //
>367 Status = PeiServicesLocatePpi (
>368 ,
>369 0,
>370 NULL,
>371 (VOID **)
>372 );
>373 if (!EFI_ERROR (Status)) {
>374DataSize = sizeof (MemoryData);
>375Status = Variable->GetVariable (
>376 Variable,
>377 EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
>378 ,
>379 NULL,
>380 ,
>381
>382 );
>383if (!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,
>389MemoryData,
>390DataSize
>391);
>392}
>393 }
>394}
>
> Thanks
> Laszlo
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45821): https://edk2.groups.io/g/devel/message/45821
Mute This Topic: https://groups.io/mt/32821535/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
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:
363if (GetFirstGuidHob ((CONST EFI_GUID
*)) == NULL) {
364 //
365 // Don't build GuidHob if GuidHob has been installed.
366 //
367 Status = PeiServicesLocatePpi (
368 ,
369 0,
370 NULL,
371 (VOID **)
372 );
373 if (!EFI_ERROR (Status)) {
374DataSize = sizeof (MemoryData);
375Status = Variable->GetVariable (
376 Variable,
377 EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME,
378 ,
379 NULL,
380 ,
381
382 );
383if (!EFI_ERROR (Status) &&
ValidateMemoryTypeInfoVariable(MemoryData, DataSize)) {
384 //
385 // Build the GUID'd HOB for DXE
386 //
387 BuildGuidDataHob (
388,
389MemoryData,
390DataSize
391);
392}
393 }
394}
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45819): https://edk2.groups.io/g/devel/message/45819
Mute This Topic: https://groups.io/mt/32821535/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
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 ; Kinney, Michael D > ; devel@edk2.groups.io > Cc: Mike Turner ; Wang, Jian J > ; Wu, Hao A ; Bi, Dandan > > 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 > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45619): https://edk2.groups.io/g/devel/message/45619 Mute This Topic: https://groups.io/mt/32821535/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
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. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45614): https://edk2.groups.io/g/devel/message/45614 Mute This Topic: https://groups.io/mt/32821535/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
Laszlo:
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, August 13, 2019 5:48 PM
> To: Kinney, Michael D ; devel@edk2.groups.io;
> Gao, Liming
> Cc: Mike Turner ; Wang, Jian J
> ; Wu, Hao A ; Bi, Dandan
>
> 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
> >>
> >> Cc: Mike Turner ; Wang, Jian J
> >> ; Wu, Hao A ;
> >> Bi, Dandan
> >> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore:
> >> Fix for missing MAT update
> >>
> >> On 08/10/19 16:10, Liming Gao wrote:
> >>> From: Mike Turner
> >>>
> >>> 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
> >>> Cc: Hao A Wu
> >>> Cc: Dandan Bi
> >>> Signed-off-by: Liming Gao
> >>> ---
> >>> 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 BOOLEANNeedGuard
> >>>)
> >>> {
> >>> - EFI_STATUS Status;
> >>> - UINT64 Start;
> >>> - UINT64 NumberOfBytes;
> >>> - UINT64 End;
> >>> - UINT64 MaxAddress;
> >>> - UINTN Alignment;
> >>> + EFI_STATUS Status;
> >>> + UINT64 Start;
> >>> + UINT64 NumberOfBytes;
> >>> + UINT64 End;
> >>> + UINT64 MaxAddress;
> >>> + UINTNAlignment;
> >>> + EFI_MEMORY_TYPE CheckType;
> >>>
> >>>if ((UINT32)Type >= MaxAllocateType) {
> >>> return EFI_INVALID_PARAMETER;
> >>> @@ -1321,6 +1322,7 @@ CoreInternalAllocatePages (
> >>>// if (Start + NumberOfBytes) rolls ov
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
>>
>> Cc: Mike Turner ; Wang, Jian J
>> ; Wu, Hao A ;
>> Bi, Dandan
>> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore:
>> Fix for missing MAT update
>>
>> On 08/10/19 16:10, Liming Gao wrote:
>>> From: Mike Turner
>>>
>>> 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
>>> Cc: Hao A Wu
>>> Cc: Dandan Bi
>>> Signed-off-by: Liming Gao
>>> ---
>>> 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 BOOLEANNeedGuard
>>>)
>>> {
>>> - EFI_STATUS Status;
>>> - UINT64 Start;
>>> - UINT64 NumberOfBytes;
>>> - UINT64 End;
>>> - UINT64 MaxAddress;
>>> - UINTN Alignment;
>>> + EFI_STATUS Status;
>>> + UINT64 Start;
>>> + UINT64 NumberOfBytes;
>>> + UINT64 End;
>>> + UINT64 MaxAddress;
>>> + UINTNAlignment;
>>> + 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
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
> -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
>
> Cc: Mike Turner ; Wang, Jian J
> ; Wu, Hao A ;
> Bi, Dandan
> Subject: Re: [edk2-devel] [Patch] MdeModulePkg DxeCore:
> Fix for missing MAT update
>
> On 08/10/19 16:10, Liming Gao wrote:
> > From: Mike Turner
> >
> > 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
> > Cc: Hao A Wu
> > Cc: Dandan Bi
> > Signed-off-by: Liming Gao
> > ---
> > 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 BOOLEANNeedGuard
> >)
> > {
> > - EFI_STATUS Status;
> > - UINT64 Start;
> > - UINT64 NumberOfBytes;
> > - UINT64 End;
> > - UINT64 MaxAddress;
> > - UINTN Alignment;
> > + EFI_STATUS Status;
> > + UINT64 Start;
> > + UINT64 NumberOfBytes;
> > + UINT64 End;
> > + UINT64 MaxAddress;
> > + UINTNAlignment;
> > + 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
> //
>
>
> > +
> > +fo
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
On 08/10/19 16:10, Liming Gao wrote:
> From: Mike Turner
>
> 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
> Cc: Hao A Wu
> Cc: Dandan Bi
> Signed-off-by: Liming Gao
> ---
> 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 BOOLEANNeedGuard
>)
> {
> - EFI_STATUS Status;
> - UINT64 Start;
> - UINT64 NumberOfBytes;
> - UINT64 End;
> - UINT64 MaxAddress;
> - UINTN Alignment;
> + EFI_STATUS Status;
> + UINT64 Start;
> + UINT64 NumberOfBytes;
> + UINT64 End;
> + UINT64 MaxAddress;
> + UINTNAlignment;
> + 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
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
Reviewed-by: Jian J Wang
> -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 ; Wang, Jian J
> ; Wu, Hao A ; Bi, Dandan
>
> Subject: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT
> update
>
> From: Mike Turner
>
> 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
> Cc: Hao A Wu
> Cc: Dandan Bi
> Signed-off-by: Liming Gao
> ---
> 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 BOOLEANNeedGuard
>)
> {
> - EFI_STATUS Status;
> - UINT64 Start;
> - UINT64 NumberOfBytes;
> - UINT64 End;
> - UINT64 MaxAddress;
> - UINTN Alignment;
> + EFI_STATUS Status;
> + UINT64 Start;
> + UINT64 NumberOfBytes;
> + UINT64 End;
> + UINT64 MaxAddress;
> + UINTNAlignment;
> + 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
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45392): https://edk2.groups.io/g/devel/message/45392
Mute This Topic: https://groups.io/mt/32821535/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
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 ; Wang, Jian J
> ; Wu, Hao A ; Bi, Dandan
>
> Subject: [edk2-devel] [Patch] MdeModulePkg DxeCore: Fix for missing MAT update
>
> From: Mike Turner
>
> 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
> Cc: Hao A Wu
> Cc: Dandan Bi
> Signed-off-by: Liming Gao
> ---
> 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 BOOLEANNeedGuard
>)
> {
> - EFI_STATUS Status;
> - UINT64 Start;
> - UINT64 NumberOfBytes;
> - UINT64 End;
> - UINT64 MaxAddress;
> - UINTN Alignment;
> + EFI_STATUS Status;
> + UINT64 Start;
> + UINT64 NumberOfBytes;
> + UINT64 End;
> + UINT64 MaxAddress;
> + UINTNAlignment;
> + 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
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45318): https://edk2.groups.io/g/devel/message/45318
Mute This Topic: https://groups.io/mt/32821535/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
