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

["Yao, Jiewen" <jiewen.yao@intel.com>]

For size field, please refer to PE/COFF specification https://docs.microsoft.com/en-us/windows/win32/debug/pe-format

The "Section Table (Section Headers)" defines two fields:
=======================
VirtualSize - The total size of the section when loaded into memory. If this value is greater than SizeOfRawData, the section is zero-padded. This field is valid only for executable images and should be set to zero for object files.

SizeOfRawData - The size of the section (for object files) or the size of the initialized data on disk (for image files). For executable images, this must be a multiple of FileAlignment from the optional header. If this is less than VirtualSize, the remainder of the section is zero-filled. Because the SizeOfRawData field is rounded but the VirtualSize field is not, it is possible for SizeOfRawData to be greater than VirtualSize as well. When a section contains only uninitialized data, this field should be zero.
=======================

We took similar concept here.

RawDataSize == size in the file.
MemoryDataSize == size in the memory. They are totally different concept.

For example, you can have 0xC81 RawDataSize, but the MemoryDataSize is 0x1000.

If one project enforces RawDataSize == MemoryDataSize, then only one size is needed.
But if one project wants to RawDataSize <= MemoryDataSize, then we need two size fields.



Thank you
Yao Jiewen

> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Monday, September 27, 2021 10:59 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; Xu, Min M <min.m.xu@intel.com>; Brijesh Singh
> <brijesh.singh@amd.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Erdem Aktas
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
>   Hi,
> 
> > > 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).
> 
> Hmm.  Not covered in tdx-virtual-firmware-design-guide-rev-1.pdf
> 
> > 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.
> 
> That still doesn't explain why you need two sizes.  Instead of depending
> on zero-fill in case MemoryDataSize > RawDataSize you can just use two
> entries, simliar to ELF binaries which have separate '.data' and '.bss'
> sections too.
> 
> > 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.
> 
> So you are doing stuff behind closed doors ...
> > 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.)
> 
> ... and don't want tell details.  Even the fact that you are doing that
> is disclosed only after poking for a while because the patches submitted
> leave a bunch of questions open.
> 
> This is NOT how Open Source Development works.
> 
> If the patches can't speak for themself in cases like this the very
> minimum requirement is proper documentation.  It is not acceptable
> that I have to ask five times to figure that the format is supposed
> to cover use cases beyond TDVF.
> 
> > 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.
> 
> The benefit of a unified table for tdx and sev is that the VMM can
> have common logic to find page ranges which need special
> initialization.
> 
> But I suspect at that point we are going to trade code sharing at one
> place for code sharing at another place.
> 
> > 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.
> 
> So, when submitting the next revision of this series, please ...
> 
>   (1) Move the tdx metadata changes to a separate patch.
>   (2) Add *complete* documentation for the TDX metadata format
> 
> ... so the SEV people can make up their mind whenever they want use
> that or not.
> 
> Please do also clarify what the process to allocate section type numbers
> (or reserve a number range) for SEV would be.
> 
> > [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.
> 
> The differences between Ovmf and AmdSev are very small.
> 
> Bhyve has more differences, but it's a different hypervisor
> so it isn't surprising it has its own PlatformPei.
> 
> How does Tdvf handle the platform setup?  It must be done in SEC
> somehow, so I suspect you have a (possibly stripped-down) version
> of the PlatformPei adapted for SEC?  That is exactly the kind of
> code duplication I want avoid.
> 
> > I reluctant to merge it back to Ovmf.dsc/fdf.
> 
> I don't worry that much about Ovmf.dsc/fdf files.  Whenever we add a
> compile-time option (-D ENABLE_TDX) to Ovmf.dsc/fdf or whenever we add a
> separate Tdvf.dsc/fdf doesn't make that much of a difference.
> 
> I'm more worried about the code duplication and the completely different
> (PEI-less) initialization workflow.  When touching the platform setup
> code both cases (with/without PEI) must be considered, which increases
> development and testing and maintenance effort long-term.
> 
> I want less variants, not more.  Ideally I'd like to also get rid of the
> OvmfPkgIa32X64.dsc/fdf for example.  It seems some features have a
> dependency on PEI running in 32-bit mode though.
> 
> > 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.
> 
> So using PEI in OVMF isn't that over-engineered, isn't it?
> 
> And I suspect SMM support can be added to the list of features which
> depend on the PEI phase (at least when we want reuse the existing common
> code in UefiCpuPkg, MdePkg and elsewhere).
> 
> > We need re-evaluate the effort to enable those features in non-PEI
> > configuration in OVMF. - That is totally unnecessary in TDVF enabling
> > task.
> 
> Well, it's surely additional upfront work.  But I expect it will pay off
> long-term.  Less maintenance work, less testing work, lower risk of
> adding regressions due to SEC and PEI init code paths variants running
> out of sync.  So "totally unnecessary" only when you ignore the work
> needed after the initial merge.
> 
> take care,
>   Gerd