Subject: Re: [PATCH v9 08/23] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Dave Hansen <[email-masked]>
To: [email-masked]

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

> +static inline bool tdx_supports_runtime_update(const struct tdx_sys_info *sysinfo)
> +{
> +	/* To be enabled when kernel is ready. */
> +	return false;
> +}

This is the bit that flags the loudest on a first pass (Dave_Hansen.txt
RULE 17 -- "be ready to answer 'why' before the question is asked").

A predicate that returns a hardcoded `false` and lives in <asm/tdx.h>
(i.e. exposed kernel-wide) for 13 patches in a row before it gets a
real body in 21/23 means:

  (a) every reviewer between 08 and 21 has to flip back and forth to
      decide whether the predicate is meaningfully checked anywhere;
  (b) anyone bisecting between 08 and 21 sees the seamldr_* sysfs
      knobs permanently hidden, which is not the user-visible state
      the series is trying to land;
  (c) the helper sits in <asm/tdx.h> rather than the driver that uses
      it, but only tdx-host calls it -- so the public-header placement
      buys us nothing.

Either:

  - Introduce tdx_supports_runtime_update() in 21/23 (where it gets
    its real body) and have 08/23 hardcode `false` inline with a
    /* unconditionally hidden until patch NN wires it up */ comment.
    That makes each commit honest about its own visibility state.
  - Or move the predicate into drivers/virt/coco/tdx-host/tdx-host.c
    (or a local header) where its only caller lives, and give it the
    real body from the start.  The header churn in the meantime is
    nil.

Otherwise the patch is fine.  The two-attribute split into a separate
group with .is_visible is the right shape, and the ABI documentation
(including the spec chapter references at the bottom of the entry)
is exactly what Dave_Hansen.txt RULE 13 wants.

One ABI-text consistency point: the changelog says "Make seamldr
attributes visible only when the update feature is supported, as
that's their sole purpose," and then the very next line says "the
underlying P-SEAMLDR attributes are available regardless of update
support; this only restricts their visibility in Linux."  Both true,
but the reader has to parse the contradiction.  Suggest tightening:

  "Make seamldr_version and num_remaining_updates visible only when
   the TDX module supports updates.  The values are well-defined on
   *any* system with a P-SEAMLDR, but if the kernel can't actually
   perform updates the attributes have no useful consumer, so hide
   them."