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

[Gerd Hoffmann]

  Hi,

> > 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.
> 
> [ ... details snipped ... ]

Ok, lets scratch the idea then.

> > 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.

Sure, physical address space on IA32 is 64G which simply doesn't fit into UINT32 ...

> 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 ?

Well, MEMFD isn't that big, so we have to care about the size there ...

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

Making one 16bit and the other 32bit doesn't make much sense due
to struct padding and alignment.  So with 64-bit load address and
fields reordered so we don't have any padding the struct could look
like this:

struct {
    uint64_t load_address;
    uint32_t file_offset;
    uint32_t section_size;
    uint32_t section_type;
    uint32_t section_flags;
};

> > 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. 

Hmm?  TDVF is based on OVMF.  My expectation would be that TDVF is
basically OVMF with TDX support added and most code being identical.
So with TDVF being able to boot without PEI most of the work should
already be done ...

> Each platform owner can have their own choice.

Why do you consider TDVF a separate platform?  I see it as OVMF variant.

Or did you just fork OVMF for config-b?  Doing so is certainly easier
for initial development and prototyping, but it's bad in the long run.
Having similar code twice in the code base means more long-term
maintenance work, which isn't fair either.

> 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.

Given TDVF wants boot without PEI the later depends on the former
though.  Or TDVF config-B continues to use the PEI-based boot workflow
for now like config-A does, and we look into removing PEI from OVMF
later.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81156): https://edk2.groups.io/g/devel/message/81156
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]
-=-=-=-=-=-=-=-=-=-=-=-