From: Dave Hansen <dave.hansen@linux.intel.com>
To: dave.hansen@linux.intel.com
Subject: Re: [PATCH v9 11/23] x86/virt/seamldr: Allocate and populate a module update request
In-Reply-To: <20260513151045.1420990-12-chao.gao@intel.com>
References: <20260513151045.1420990-12-chao.gao@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

[Draft review generated by x86-maintainer-agent applying Dave_Hansen.txt
 rules to the v9 series.  Strip this banner before sending.]

A handful of things on this one -- it's a load-bearing patch and worth
getting tight before merging.

> +#define SEAMLDR_MAX_NR_MODULE_PAGES	496
> +#define SEAMLDR_MAX_NR_SIG_PAGES	1

Both of these are magic numbers (Dave_Hansen.txt RULE 7).  496 is
derived from "however many 8-byte PA entries fit in struct
seamldr_params after the fixed-size preamble".  Spell that out next to
the definition -- I should not have to do the arithmetic to know
whether changing the struct silently breaks the constant:

  /*
   * struct seamldr_params is one 4 KiB page.  The PA-list arrays
   * are sized so that the whole struct ends at exactly PAGE_SIZE;
   * see static_assert below.
   */
  #define SEAMLDR_MAX_NR_MODULE_PAGES   496
  #define SEAMLDR_MAX_NR_SIG_PAGES      1

The existing static_assert(sizeof(struct seamldr_params) == 4096) is
exactly the RULE 8 invariant lock-in that catches this if anyone ever
changes the struct.

> +struct tdx_image_header {
> +	u16	version; // This ABI is always 0x200

Kernel coding style is /* C-style */ for comments.  The series mixes
both -- pick one (the rest of the file is /* */, so this one is the
outlier).

Also: TDX_IMAGE_VERSION_2 has value 0x200.  Convention?  Major-byte
in the high byte?  A two-line comment near the #define would save
someone reverse-engineering the meaning later.  RULE 2.

> +	u8	reserved[4076];
> +} __packed;

Good -- and the memchr_inv(header->reserved, 0, ...) below is doing
exactly the right thing to enforce "reserved fields must be zero" as
an ABI rule.

> +static void populate_pa_list(u64 *pa_list, u32 max_entries, const u8 *start, u32 nr_pages)
> +{
> +	int i;
> +
> +	nr_pages = MIN(nr_pages, max_entries);

This silently truncates if the caller asks for more pages than fit.
init_seamldr_params() validates `HEADER_SIZE + sigstruct_len +
module_len != size` before getting here, which makes the
out-of-bounds case unreachable in practice -- but that defence is in
a different function.  Either:

  - add a comment in populate_pa_list() saying "caller validates the
    range; the MIN() is defence in depth", or
  - WARN_ON_ONCE(nr_pages > max_entries) so a future caller skipping
    the size check yells loudly.

Dave_Hansen.txt RULE 8 -- prefer WARN_ON_ONCE for invariants future
changes might break.

> +	params->version			= 0;

What is "0" here?  The ABI version of seamldr_params?  Comment please
(RULE 2 / RULE 7).

> +	params = kzalloc_obj(*params);

If this is a wrapper macro you've added elsewhere, link the patch that
adds it (or carry that patch in the series).  I don't see kzalloc_obj()
elsewhere in this submission; if you're depending on something in
-mm or -tip that isn't merged yet, call that dependency out in the
cover letter.  Otherwise it's just `kzalloc(sizeof(*params), GFP_KERNEL)`.

> +	const struct tdx_image *image		= (const void *)data;
> +	const struct tdx_image_header *header	= &image->header;

Nice -- the column-aligned local declarations and the comment-free
"check size, check version, check signature, check reserved bits"
sequence reads cleanly.  Order matters: please add a one-line comment
explaining *why* you check version before signature (if there's a
reason) -- or reorder so signature comes first, which is the more
intuitive "is this even our blob?" check.