Re: [PATCH v8 17/55] i386/tdvf: Introduce function to parse TDVF metadata

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 4/2/2025 8:08 PM, Daniel P. Berrangé wrote:
> On Tue, Apr 01, 2025 at 09:01:27AM -0400, Xiaoyao Li wrote:
>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>
>> TDX VM needs to boot with its specialized firmware, Trusted Domain
>> Virtual Firmware (TDVF). QEMU needs to parse TDVF and map it in TD
>> guest memory prior to running the TDX VM.
>>
>> A TDVF Metadata in TDVF image describes the structure of firmware.
>> QEMU refers to it to setup memory for TDVF. Introduce function
>> tdvf_parse_metadata() to parse the metadata from TDVF image and store
>> the info of each TDVF section.
>>
>> TDX metadata is located by a TDX metadata offset block, which is a
>> GUID-ed structure. The data portion of the GUID structure contains
>> only an 4-byte field that is the offset of TDX metadata to the end
>> of firmware file.
>>
>> Select X86_FW_OVMF when TDX is enable to leverage existing functions
>> to parse and search OVMF's GUID-ed structures.
>>
>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>> Co-developed-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>> Changes in v8:
>>   - Drop the failure handling of memcpy() since it cannot fail;
>>
>> Changes in v7:
>>   - Update license info to only use SPDX tag;
>>   - use g_autofree to avoid manually free;
>>
>> Changes in v6:
>>   - Drop the the data endianness change for metadata->Length;
>>
>> Changes in v1:
>>   - rename tdvf_parse_section_entry() to
>>     tdvf_parse_and_check_section_entry()
>>
>> Changes in RFC v4:
>>   - rename TDX_METADATA_GUID to TDX_METADATA_OFFSET_GUID
>> ---
>>   hw/i386/Kconfig        |   1 +
>>   hw/i386/meson.build    |   1 +
>>   hw/i386/tdvf.c         | 183 +++++++++++++++++++++++++++++++++++++++++
>>   include/hw/i386/tdvf.h |  38 +++++++++
>>   4 files changed, 223 insertions(+)
>>   create mode 100644 hw/i386/tdvf.c
>>   create mode 100644 include/hw/i386/tdvf.h
>>
>> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
>> index cce9521ba934..eb65bda6e071 100644
>> --- a/hw/i386/Kconfig
>> +++ b/hw/i386/Kconfig
>> @@ -12,6 +12,7 @@ config SGX
>>   
>>   config TDX
>>       bool
>> +    select X86_FW_OVMF
>>       depends on KVM
>>   
>>   config PC
>> diff --git a/hw/i386/meson.build b/hw/i386/meson.build
>> index 10bdfde27c69..3bc1da2b6eb4 100644
>> --- a/hw/i386/meson.build
>> +++ b/hw/i386/meson.build
>> @@ -32,6 +32,7 @@ i386_ss.add(when: 'CONFIG_PC', if_true: files(
>>     'port92.c'))
>>   i386_ss.add(when: 'CONFIG_X86_FW_OVMF', if_true: files('pc_sysfw_ovmf.c'),
>>                                           if_false: files('pc_sysfw_ovmf-stubs.c'))
>> +i386_ss.add(when: 'CONFIG_TDX', if_true: files('tdvf.c'))
>>   
>>   subdir('kvm')
>>   subdir('xen')
>> diff --git a/hw/i386/tdvf.c b/hw/i386/tdvf.c
>> new file mode 100644
>> index 000000000000..328d1b7ffdf8
>> --- /dev/null
>> +++ b/hw/i386/tdvf.c
>> @@ -0,0 +1,183 @@
>> +/*
>> + * Copyright (c) 2025 Intel Corporation
>> + * Author: Isaku Yamahata <isaku.yamahata at gmail.com>
>> + *                        <isaku.yamahata at intel.com>
>> + *         Xiaoyao Li <xiaoyao.li@intel.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>> +
>> +#include "hw/i386/pc.h"
>> +#include "hw/i386/tdvf.h"
>> +#include "system/kvm.h"
>> +
>> +#define TDX_METADATA_OFFSET_GUID    "e47a6535-984a-4798-865e-4685a7bf8ec2"
>> +#define TDX_METADATA_VERSION        1
>> +#define TDVF_SIGNATURE              0x46564454 /* TDVF as little endian */
>> +
>> +typedef struct {
>> +    uint32_t DataOffset;
>> +    uint32_t RawDataSize;
>> +    uint64_t MemoryAddress;
>> +    uint64_t MemoryDataSize;
>> +    uint32_t Type;
>> +    uint32_t Attributes;
>> +} TdvfSectionEntry;
>> +
>> +typedef struct {
>> +    uint32_t Signature;
>> +    uint32_t Length;
>> +    uint32_t Version;
>> +    uint32_t NumberOfSectionEntries;
>> +    TdvfSectionEntry SectionEntries[];
>> +} TdvfMetadata;
> 
> struct field names starting with an initial capital is
> not the usual QEMU code style. Can this be all initial
> lowercase, with capital just for word separation.

I think Isaku used such names because they are defined in TDVF Design 
Guide spec[1].

QEMU's internal data structure for metadata are below, which follows 
QEMU coding style.

	typedef struct TdxFirmwareEntry {
	    uint32_t data_offset;
	    uint32_t data_len;
	    uint64_t address;
	    uint64_t size;
	    uint32_t type;
	    uint32_t attributes;
	} TdxFirmwareEntry;

	typedef struct TdxFirmware {
	    uint32_t nr_entries;
	    TdxFirmwareEntry *entries;
	} TdxFirmware;

So if no strong preference, I would keep it as-is that the raw struct 
read from TDVF keeps the name convention of TDVF Design Guide spec and 
internal data struct uses QEMU's convention.

[1] https://cdrdv2.intel.com/v1/dl/getContent/733585

> 
>> +
>> +struct tdx_metadata_offset {
>> +    uint32_t offset;
>> +};
>> +
> 
> With regards,
> Daniel