Comment below:
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Monday, September 27, 2021 4:05 PM
> To: Yao, Jiewen <jiewen....@intel.com>
> Cc: devel@edk2.groups.io; Xu, Min M <min.m...@intel.com>; Brijesh Singh
> <brijesh.si...@amd.com>; Ard Biesheuvel <ardb+tianoc...@kernel.org>;
> Justen, Jordan L <jordan.l.jus...@intel.com>; Erdem Aktas
> <erdemak...@google.com>; James Bottomley <j...@linux.ibm.com>; Tom
> Lendacky <thomas.lenda...@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
>
> 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;
> };
[Jiewen] This data structure does not work in a special use case - A TD may
want to have a fixed memory size.
It is TD that tells the VMM how many DRAM should be allocated by using metadata
table. Not the case that a VMM tells the TD how many DRAM is allocated by using
HOB.
In that special case, the TD_HOB is NOT required. The VMM parses the metadata
to allocate the DRAM (AUG page).
The runtime section size must be UINT64, otherwise we cannot support > 4G
memory section.
The build time section size can be UINT32. We don't expect to create a >4G
binary in near future.
And we need both.
I can understand why you think there is no needed fields, based upon what you
see in EDKII/TDVF project.
However, the usage in current TDVF is just a subset, but not all usages.
The TDX metadata structure is carefully designed to support those variants.
Also, leaving some room for future is a common practice.
Besides EDKII/TDVF, we are doing other TDX related projects reusing the same
metadata structure. (But sorry, I cannot tell more at this time.)
The benefit is that the KVM or cloud hypervisor can have a common logic to
handle "TDX boot", instead of using different table in different use cases.
Defining something only feasible in EDKII/OVMF will bring a burden. That is NOT
a way we want to go.
Let me say in this way:
I think it is OK, if SEV wants to reuse the existing TDX metadata table. (We
need SEV people agree.) Then we can have one metadata table.
But we cannot reuse the SEV defined metadata table in
https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm,
because it missed some fields we need. If that is the case, then we have to
use 2 metadata tables.
>
> > > 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.
[Jiewen] We don't fork OVMF in config-B.
Instead of we will create new Tdvf/Tdvf.dsc and Tdvf/Tdvf.fdf in config-B,
similar to https://github.com/tianocore/edk2/tree/master/OvmfPkg/AmdSev or
https://github.com/tianocore/edk2/tree/master/OvmfPkg/Bhyve
That is why I treat it as different platform.
Non-PEI is not a new concept. It is quite mature known configuration.
I reluctant to merge it back to Ovmf.dsc/fdf. The reason is the main Ovmf
supports some features (such as S3, TPM) which may depend on PEI modules, but
it is NOT needed in TDVF.
We need re-evaluate the effort to enable those features in non-PEI
configuration in OVMF. - That is totally unnecessary in TDVF enabling task.
That is why I treat them as separate task. And something the OVMF community may
look into in the future.
>
> take care,
> Gerd
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81165): https://edk2.groups.io/g/devel/message/81165
Mute This Topic: https://groups.io/mt/85761661/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
