From: Dave Hansen To: dave.hansen@linux.intel.com Subject: Re: [PATCH v9 00/23] Runtime TDX module update support In-Reply-To: <20260513151045.1420990-1-chao.gao@intel.com> References: <20260513151045.1420990-1-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.] Series-level read after applying Dave_Hansen.txt to v9. Overall the series looks substantially improved from v8: the cover letter now states the design (RULE 14), the kernel/userspace split is articulated clearly, and the rejected alternatives are written down where they belong (in the patches that needed them, not buried in the thread). A few series-level concerns to chase down before this goes any further: 1. tdx_supports_runtime_update() is introduced in patch 08 as a stub that returns false, and given its real body in patch 21. For 13 patches in a row, the predicate is dead code that always hides the sysfs attributes it gates. That makes those 13 commits individually unbisectable for runtime-update behavior and makes the series harder to review one commit at a time (Dave_Hansen.txt RULE 17 -- the "why does this exist yet?" question gets asked on every reviewer's first pass). Two options: a. Move the introduction of tdx_supports_runtime_update() into patch 21 alongside its real body, and hardcode "false" inline in patch 08 with a /* enabled in patch NN */ comment, or b. Give it its real body from the start and order the dependency chain so the feature lights up the moment the wiring is complete. 2. Patch 20 references TDX_SEAMCALL_STATUS_MASK from arch/x86/virt/vmx/tdx/tdx.c, but the only definition of that macro in-tree is in arch/x86/kvm/vmx/tdx_errno.h, which is KVM-private. Either (a) the base-commit already has it lifted to a shared header and I missed the change, or (b) a prep patch is missing that moves it (and probably its companion error codes) into a header the host side can include. Please confirm; if (b), add the prep patch up front so the series builds at every commit. 3. WARN_ON_ONCE-then-continue with stale data shows up at least twice: - patch 14, tdx_module_shutdown(): WARN_ON_ONCE(ret) on get_tdx_sys_info_handoff() failure, then args.rcx = handoff.module_hv uses the (uninitialized-on-failure) value. - patch 19, tdx_module_run_update(): WARN_ON_ONCE(ret) on get_tdx_sys_info_version() failure, then tdx_sysinfo.version reflects whatever the failed read left behind. Dave_Hansen.txt RULE 8 is "Use WARN_ON_ONCE to lock in invariants that future changes could silently break". The usage here is inverted: a real runtime error is being demoted to a one-shot dmesg line and the code carries on regardless. Either propagate the error or document explicitly (in a comment) why proceeding with the default-initialised struct is safe. 4. Many patches still have stub bodies that get filled in by a later patch in the same series (10, 12, 14, 16, 17, 18 in particular). The split is defensible, but several subject lines overpromise: e.g. patch 10 "Implement firmware upload sysfs ABI for TDX module updates" wires the ABI but install_module() is /* TODO */. Consider tightening the subjects to say what the patch actually contains ("Add fw_upload ABI plumbing for ..." with the implementation in the later commits the ABI calls into). RULE 1 / RULE 3. 5. RULE 5 -- selftests. This series adds a new sysfs ABI plus a firmware-upload pathway and provides no selftest in tools/testing/selftests/. At minimum, a tiny selftest that opens each new sysfs attribute, asserts they parse to the documented format, and (where the feature is available) exercises the fw_upload upload-cancel-status state machine, would catch the most common future regressions and answer the "did you actually test this?" question for free. The patch-by-patch reviews follow as separate replies.