Re: [PATCH v9 07/23] x86/virt/seamldr: Add a helper to retrieve P-SEAMLDR information

[Dave Hansen <dave.hansen@intel.com>]

On 5/13/26 08:09, Chao Gao wrote:
> P-SEAMLDR reports its state via SEAMLDR.INFO, including its version and
> the number of remaining runtime updates.
> 
> This information is useful for userspace. For example, the admin can use
> the P-SEAMLDR version to determine whether a candidate TDX module is
> compatible with the running loader, and can use the remaining update count
> to determine whether another runtime update is still possible.
> 
> Add a helper to retrieve P-SEAMLDR information in preparation for
> exposing P-SEAMLDR version and other necessary information to userspace.
> Export the new kAPI for use by tdx-host.ko.

What is "tdx-host.ko"? My kernel doesn't have that. Remember a few
patches ago, the "tristate" in Kconfig? ;)

You also called in the "tdx_host" device. Please be consistent with the
naming.

> Note that there are two distinct P-SEAMLDR APIs with similar names:
> 
>   SEAMLDR.INFO: Returns a SEAMLDR_INFO structure containing SEAMLDR
>                 information such as version and remaining updates.
> 
>   SEAMLDR.SEAMINFO: Returns a SEAMLDR_SEAMINFO structure containing SEAM
>                     and system information such as Convertible Memory
> 		    Regions (CMRs) and number of CPUs and sockets.
> 
> The former is used here.
This doesn't help.

"SEAMLDR.INFO" is metadata about the loader. It's metadata for the
update process.

"SEAMLDR.SEAMINFO" is metadata about the module. It is for the module
init process, not for the update process.

Right? Isn't that a billion times more useful and actually helps
differentiate them?

Also, more nits: I hate former/latter too. It makes me stop reading and
have to go back *EVER* *TIME*. I hate that particular english construct.
It's horrid.

Just say:

	Use SEAMLDR.INFO here.

It's even shorter than the passive "is used" and doesn't require me
going back and re-read the text.

> +++ b/arch/x86/include/asm/seamldr.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_SEAMLDR_H
> +#define _ASM_X86_SEAMLDR_H
> +
> +#include <linux/types.h>
> +
> +/*
> + * This is called the "SEAMLDR_INFO" data structure and is defined
> + * in "SEAM Loader (SEAMLDR) Interface Specification".

More succinct:

    * This is the "SEAMLDR_INFO" data structure defined in the
      "SEAM Loader (SEAMLDR) Interface Specification".

> + * The SEAMLDR.INFO documentation requires this to be aligned to a
> + * 256-byte boundary.
> + */

Just say:

 * Must be aligned to a 256-byte boundary.

With those fixed:

Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>