Subject: Re: [PATCH v9 00/23] Runtime TDX module update support
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.]

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.