[edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

[Yao, Jiewen]

Comment below

> -----Original Message-----
> From: kraxel at redhat.com <kraxel at redhat.com>
> Sent: Friday, September 24, 2021 10:02 PM
> To: Yao, Jiewen <jiewen.yao at intel.com>
> Cc: devel at edk2.groups.io; Xu, Min M <min.m.xu at intel.com>; Brijesh Singh
> <brijesh.singh at amd.com>; Ard Biesheuvel <ardb+tianocore at kernel.org>;
> Justen, Jordan L <jordan.l.justen at intel.com>; Erdem Aktas
> <erdemaktas at google.com>; James Bottomley <jejb at linux.ibm.com>; Tom
> Lendacky <thomas.lendacky at amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> On Fri, Sep 24, 2021 at 10:33:35AM +0000, Yao, Jiewen wrote:
> > Again. Two topics. We need discuss them separately.
> >
> > Topic 1: TD metadata table is an architecture way to communicate with
> > VMM.  We took the design from PE/COFF image section, which is flexible
> > to support different binary format.
> > EDKII TDVF is just one possible producer. There could be other
> > producer in the future. We don't want to define something only meet
> > current TDVF need.
> 
> Hmm.  efi has a kind-of binary format (EFI_FIRMWARE_VOLUME_HEADER).
> It's not fully self-contained though, you need to know where the
> architecture places the firmware (i.e. just below 4G for x86)
> because the load address isn't there.  So I do see the point of
> adding other headers adding that.
>
> Possible alternative approach: Define an extension
> (EFI_FIRMWARE_VOLUME_EXT_HEADER) for the load address and use that
> instead of defining something new.
>

[Jiewen] I would say it is terrible idea to use EFI_FIRMWARE_VOLUME_HEADER.

This is defined by PI specification, the purpose is to have a way to manage the firmware file.
It is very complicated and with overhead only needed for flash.

Intel has multiple technologies requires firmware/hardware interface. None of them uses EFI_FIRMARE_VOLUME_HEADER, because it is unnecessary to carry the complexity.

The most famous firmware/hardware interface is called Firmware Interface Table (FIT) table. - https://software.intel.com/content/dam/develop/external/us/en/documents/firmware-interface-table-bios-specification-r1p2p1.pdf.

Also using EFI_FIRMWARE_VOLUME_HEADER means you have to use PI FV layout, which is another unnecessary limitation. 

I would strongly disagree to using EFI_FIRMWARE_VOLUME_HEADER for metadata table.

> Still not clear why the size is in there twice.  I think you could
> instead use a flag telling whenever a section must be loaded from
> the image or not.  This is what COFF and ELF are doing too.
> 
> Also not clear why you want stick to 64bit base address.  Loading the
> firmware above 4G isn't going to work without also changing a bunch of
> other things and it will break backward compatibility anyway.

[Jiewen] That is our previously experience, when we define a physical address, we always use 64bit to leave room for extension.
Like UEFI specification defines PHYSICAL_ADDRESS to be UINT64 even in IA32 platform.

Let me tell you a story.
In https://github.com/tianocore/edk2/blob/master/SecurityPkg/Include/Ppi/FirmwareVolumeInfoPrehashedFV.h, we defined the physical address to be FvBase.
  UINT32                                     FvBase;
  UINT32                                     FvLength;

It is rare, because in other context, we usually define FvBase to be 64bit.

Later, when we want to enable a host app based unit test for this data structure, we got test crash directly, because the OS app test allocates an above 4G base address, and the test case cast it to UINT32.

Do we really care to save 4 bytes in the PPI definition? I don't think so.
But it just defines in this way and it brings a big burden to enable unit test.
This code has been checked in for 2 years. Till now, I still regret that why we didn't use UINT64 in the beginning.

I have seen other examples to define small size, such as PI Section Size (24 bit), FFS Size (24 bit), HOB size (16 bit). Then we run into problem to support large structure later and we have to figure out ugly work-around to support those cases.

Defining UINT64 gives us enough flexibility for the future, including test in above 4G environment.
I am wondering that, do you really care to save 4 bytes from UINT64 to UINT32 ?

For type, maybe 2^16 is enough. But for flags, I prefer 32bit.

> 
> I think the entry size can be cut in half with something like this:
> 
> struct {
> 	uint32_t file_offset;
> 	uint32_t load_address;
> 	uint32_t section_size;
> 	uint16_t section_type;
> 	uint16_t section_flags;
> };
> 
> > Topic 2: In config-B we remove PEI.
> > I think we should say it in different way: We add PEI back in config-A.
> > In our original design we totally eliminated PEI, because it is unnecessary.
> IMHO, it is totally an overdesign in OVMF to include PEI.
> 
> Granted.  PEI basically allows OEMs to plug their binary PEIMs into
> early hardware initialization.  For a full open source firmware there
> is little reason to support that, other than maybe using PEIMs from
> other edk2 Pkgs unmodified.
> 
> But, again, I don't want have two completely different initialization
> code paths in OVMF.  We can certainly investigate and discuss dropping
> PEI, but that clearly shouldn't be a TDX-only thing.  When we eliminate
> PEI, we should do it for all OVMF builds.

[Jiewen] I think this is out of scope of TDVF config-B patch series.
I don't think it is fair to enable OVMF to remove PEI, just because TDVF does not need PEI. 

If you look around other edk2 platform projects, you can already find some of them does not have PEI. And EmbeddedPkg includes some libs to support those case, such as https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/PrePiHobLib, https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Library/PrePiLib.
But that does not mean, we need do that for every platform.

Each platform owner can have their own choice.

If you have intertest to remove PEI, I am happy to discuss with you on detail.
However, I would treat "removing PEI from OVMF" and "enable TDVF config-B" be two different tasks.


> 
> take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81107): https://edk2.groups.io/g/devel/message/81107
Mute This Topic: https://groups.io/mt/85761661/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-