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

["Liming Gao" <liming.gao@intel.com>]

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