Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information

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

On Wed, Jan 28, 2026 at 03:54:38PM -0800, Dave Hansen wrote:
>On 1/23/26 06:55, Chao Gao wrote:
>> P-SEAMLDR returns its information e.g., version and supported features, in
>> response to the SEAMLDR.INFO SEAMCALL.
>> 
>> This information is useful for userspace. For example, the admin can decide
>> which TDX module versions are compatible with the P-SEAMLDR according to
>> the P-SEAMLDR version.
>> 
>> Add and export seamldr_get_info() which retrieves P-SEAMLDR information by
>
>I don't need to know what the function name is. That's in the code.
>

Hi Dave,

Thank you for the thorough review.

I will go through the following patches to ensure they don't have the same
issues you have pointed out.

>> invoking SEAMLDR.INFO SEAMCALL in preparation for exposing P-SEAMLDR
>> version and other necessary information to userspace.
>
>I also want to know what spec you are getting this out of.

Will add a link in the changelog.

>
>I think it's also worth calling out that there are SEAMLDR calls for both:
>
>	SEAMLDR_INFO
>and
>	SEAMLDR_SEAMINFO
>
>Which is astonishingly confusing. Please have mercy on folks that are
>looking through the docs for the first time and explain this.

Sorry about this. Will do.

>
>> diff --git a/arch/x86/include/asm/seamldr.h b/arch/x86/include/asm/seamldr.h
>> new file mode 100644
>> index 000000000000..d1e9f6e16e8d
>> --- /dev/null
>> +++ b/arch/x86/include/asm/seamldr.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_X86_SEAMLDR_H
>> +#define _ASM_X86_SEAMLDR_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct seamldr_info {
>
>/*
> * This called the "SEAMLDR_INFO" data structure and is defined
> * in "SEAM Loader (SEAMLDR) Interface Specification".
> */

Will do.

>
>
>> +	u32	version;
>> +	u32	attributes;
>> +	u32	vendor_id;
>> +	u32	build_date;
>> +	u16	build_num;
>> +	u16	minor_version;
>> +	u16	major_version;
>> +	u16	update_version;
>> +	u8	reserved0[4];
>
>Why not label this:
>
>	u32	acm_x2apicid: /* unused by kernel */
>
>?

Will do. Probably because I thought the kernel would never use it.

<snip>

>> +const struct seamldr_info *seamldr_get_info(void)
>> +{
>> +	struct tdx_module_args args = { .rcx = __pa(&seamldr_info) };
>> +
>> +	return seamldr_call(P_SEAMLDR_INFO, &args) ? NULL : &seamldr_info;
>> +}
>> +EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
>
>I'd also prefer a
>
>	BUILD_BUG_ON(sizeof(struct seamldr_info) != 2048);
>
>just as a sanity check. It doesn't cost anything and it makes sure that
>as you muck around with reserved fields and padding that there's at
>least one check making sure it's OK.

ok.