From: Dave Hansen To: dave.hansen@linux.intel.com Subject: Re: [PATCH v9 08/23] coco/tdx-host: Expose P-SEAMLDR information via sysfs In-Reply-To: <20260513151045.1420990-9-chao.gao@intel.com> References: <20260513151045.1420990-9-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.] > +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 (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 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."