Re: [PATCH v3 13/26] x86/virt/seamldr: Allocate and populate a module update request

[Chao Gao <chao.gao@intel.com>]

On Tue, Jan 27, 2026 at 11:21:06AM +0800, Huang, Kai wrote:
>
>> +/*
>> + * Allocate and populate a seamldr_params.
>> + * Note that both @module and @sig should be vmalloc'd memory.
>> + */
>> +static struct seamldr_params *alloc_seamldr_params(const void *module, unsigned int module_size,
>> +						   const void *sig, unsigned int sig_size)
>> +{
>> +	struct seamldr_params *params;
>> +	const u8 *ptr;
>> +	int i;
>> +
>> +	BUILD_BUG_ON(sizeof(struct seamldr_params) != SZ_4K);
>> +	if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	if (!IS_ALIGNED(module_size, SZ_4K) || sig_size != SZ_4K ||
>> +	    !IS_ALIGNED((unsigned long)module, SZ_4K) ||
>> +	    !IS_ALIGNED((unsigned long)sig, SZ_4K))
>> +		return ERR_PTR(-EINVAL);
>> +
>
>Based on the the blob format link below, we have 
>
>struct tdx_blob
>{
>	...
>	_u64 sigstruct[256]; // 2KB sigstruct,intel_tdx_module.so.sigstruct
>	_u64 reserved2[256]; // Reserved space
>	...
>}
>
>So it's clear SIGSTRUCT is just 2KB and the second half 2KB is "reserved
>space".
>
>Why is the "reserved space" treated as part of SIGSTRUCT here? 

Good question. Because the space is reserved for sigstruct expansion.

The __current__ SEAMLDR ABI accepts one 4KB page, but all __existing__
sigstructs are only 2KB. so, tdx_blob currently defines a 2KB sigstruct field
followed by 2KB of reserved space. We anticipate that sigstructs will
eventually exceed 4KB, so we added reserved3[N*512] to accommodate future
growth.

You're right. The current tdx_blob definition doesn't clearly indicate that
reserved2/3 are actually part of the sigstruct.

Does this revised tdx_blob definition make that clearer and better align with
this patch? The idea is to make tdx_blob generic enough to clearly represent:
a 4KB header, followed by 4KB-aligned sigstruct, followed by the TDX Module
binary. Current SEAMLDR ABI details or current sigstruct sizes are irrelevant.

struct tdx_blob
{
        _u16 version;              // Version number
        _u16 checksum;             // Checksum of the entire blob should be zero
        _u32 offset_of_module;     // Offset of the module binary intel_tdx_module.bin in bytes
        _u8  signature[8];         // Must be "TDX-BLOB"
        _u32 length;               // The length in bytes of the entire blob
        _u32 reserved0;            // Reserved space
        _u64 reserved1[509];       // Reserved space
        _u64 sigstruct[512 + N*512]; // sigstruct, 4KB aligned

	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        _u8  module[];             // intel_tdx_module.bin, 4KB aligned, to the end of the file
}


>
>> +
>> +/*
>> + * Intel TDX Module blob. Its format is defined at:
>> + * https://github.com/intel/tdx-module-binaries/blob/main/blob_structure.txt
>> + */
>> +struct tdx_blob {
>> +	u16	version;
>> +	u16	checksum;
>> +	u32	offset_of_module;
>> +	u8	signature[8];
>> +	u32	len;
>> +	u32	resv1;
>> +	u64	resv2[509];
>
>Nit:  Perhaps s/resv/rsvd ?
>

Sure. Will do.

>"#grep rsvd arch/x86 -Rn" gave me a bunch of results but "#grep resv" gave
>me much less (and part of the results were 'resvd' and 'resv_xx' instead of
>plain 'resv').
>  
>> +	u8	data[];
>> +} __packed;
>
>For this structure, I need to click the link and open it in a browser to
>understand where is the sigstruct and module, and ...
>
>> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
>> +{
>> +	const struct tdx_blob *blob = (const void *)data;
>> +	int module_size, sig_size;
>> +	const void *sig, *module;
>> +
>> +	if (blob->version != 0x100) {
>> +		pr_err("unsupported blob version: %x\n", blob->version);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	if (blob->resv1 || memchr_inv(blob->resv2, 0, sizeof(blob->resv2))) {
>> +		pr_err("non-zero reserved fields\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	/* Split the given blob into a sigstruct and a module */
>> +	sig		= blob->data;
>> +	sig_size	= blob->offset_of_module - sizeof(struct tdx_blob);
>> +	module		= data + blob->offset_of_module;
>> +	module_size	= size - blob->offset_of_module;
>> +
>
>... to see whether this code makes sense.
>
>I understand the
>
>	...
>	u64	rsvd[N*512];
>	u8	module[];
>
>is painful to be declared explicitly in 'struct tdx_blob' because IIUC we
>cannot put two flexible array members at the end of the structure.

Yes.

>
>But I think if we add 'sigstruct' to the 'struct tdx_blob', e.g.,
>
>struct tdx_blob {
>	u16	version;
>	...
>	u64	rsvd2[509];
>	u64	sigstruct[256];
>	u64	rsvd3[256];
>	u64	data;
>} __packed;
>
>.. we can just use
>
>	sig		= blob->sigstruct;
>	sig_size	= 2K (or 4K I don't quite follow);
>
>which is clearer to read IMHO?

The problem is hard-coding the sigstruct size to 2KB/4KB. This will soon no
longer hold.

But
	sig		= blob->data;
	sig_size	= blob->offset_of_module - sizeof(struct tdx_blob);

doesn't make that assumption, making it more future-proof.

>
>> +	return alloc_seamldr_params(module, module_size, sig, sig_size);
>> +}
>> +
>
>
>