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

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

(+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