Re: [RFC PATCH 06/15] virt: tdx-guest: Support TDI bind and unbind operations

[Nikolay Borisov <nik.borisov@suse.com>]

On 10.11.26 г. 11:40 ч., Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Edgecombe, Rick P <rick.p.edgecombe@intel.com>
>> Subject: Re: [RFC PATCH 06/15] virt: tdx-guest: Support TDI bind and unbind
>> operations
>>
>> On Tue, 2026-11-03 at 08:49 +0000, Duan, Zhenzhong wrote:
>>>
>>>> -----Original Message-----
>>>> From: Edgecombe, Rick P <rick.p.edgecombe@intel.com>
>>>> On Thu, 2026-10-29 at 03:26 +0000, Duan, Zhenzhong wrote:
>>>>> I want to discuss this further.
>>>>>
>>>>> If the TSM driver successfully locks a device but later fails to
>> unlock
>>>> it,
>>>>
>>>> Can you explain some concrete scenarios where this would happen? We can
>>>> think
>>>> about whether it is required for an initial minimal set that does
>> something
>>>> useful.
>>>
>>> I can think of some rare cases: the TDCM hypercall may return an
>> error(e.g., a messing VMM or TDI device firmware),
>>>
>>
>> I'm not clear whose fault this is.
>>
>>>   or TDCM hypercall polling timeout(e.g., heavy load in host).
>>
>> Heavy load in the host leads to broken flag being set? This is pretty
>> similar to
>> the issues around the BUSY errors. You really need to think through how
>> this is
>> all going to work.
> 
> I found current implementation did not handle a polling timeout well,
> so I reworked it following the GetQuote implementation. The shared buffer
> is now allocated per device instead of per command. After a timeout,
> further commands fail with -EBUSY until the VMM reports the pending
> command complete, and the buffer is leaked only if that command is still
> pending when the device is unlocked.
> 
> Below is the reworked patch that handles polling timeout, I'd like to seek
> comments on this new implementation early, comments welcome!
> 
>      virt: tdx-guest: Add TDCM helper infrastructure
> 
>      Add internal helpers that send TDCM commands to the host and wait for
>      them to complete, using a buffer shared with the VMM. The tdx-guest
>      driver uses them to manage the TDI of an assigned TEE-IO device, for
>      example to bind or unbind it.
> 
>      A TDCM VMCALL acknowledges command submission, but the command may
>      remain pending after it returns. GHCI allows asynchronous completion and
>      defines status polling and event notification as completion methods. A
>      valid notification vector (32-255) enables event notification.

Remove last sentence, this is an implementation detail of the vent 
notification which is not even being used. It's sufficient to say there 
are 2 methods and you've chosen polling and then say *WHY* you have 
chosen it, which you do with the following paragraph.

> 
>      Use status polling instead of event notification. TDCM commands are only
>      issued when a device is locked, unlocked or accepted, which are
>      infrequent administrative operations, so polling adds little overhead
>      and avoids setting up an interrupt vector and its completion handling.

This can be simplified, just say that mechanism is only used during 
setup which is a small part of the whole process and polling has been 
chosen for simplicity, no need to go into detail about interrupt vector 
setting up and what not.

> 
>      After the VMCALL returns, check the shared status and, while the command
>      is still pending, poll it with a sleep of at most 1 ms until a 10-second
>      timeout expires. Each DOE request-response exchange may take up to one
>      second, the limit set by the PCIe specification and used by Linux's DOE
>      code. In testing, BIND used the most exchanges of any TDCM command, at
>      three, so 10 seconds leaves room for those exchanges and additional host
>      processing. This is a guest-side timeout, not a GHCI completion
>      guarantee, and does not bound the time spent in the VMCALL itself.

I find this somewhat way too verbose with implementation detail which 
can be derived from just looking at the code.

> 
>      Converting memory between private and shared is expensive and breaks up
>      large direct-map and EPT mappings. Keep one shared buffer per TDI
>      context: allocate it and convert it to shared when the device is
>      locked, and convert it back and free it when the device is unlocked,
>      rather than converting memory for every command. All TDCM commands for
>      a device run under its device lock, which serializes use of the buffer.

Again, way too many implementation detail. Just say the memory is 
initialized at device lock and freed when you are finished.

> 
>      Only the command helper writes to the buffer. It takes the command data
>      from the caller and copies it in once the VMM no longer owns the buffer.
>      GHCI defines TDCM commands that carry command data, so the helper
>      accepts it even though the commands used initially carry none.
> 
>      Treat the response as VMM-controlled: read its length once, require
>      exactly the expected size for fixed-size responses or at most the
>      caller's maximum otherwise, and leave reading or copying the data to
>      the caller.

Remove this paragraph, it simply restates what the code is doing. The 
gist is that once the TDCM is sent we have to treat the contents of buf 
as coming from an outside party.

> 
>      If a command is still pending at timeout, reject further commands with
>      -EBUSY while the VMM still reports it as pending, as GetQuote does. The
>      VMM may still complete such a command, so its effect is unknown to the
>      guest. The operation that issued it fails, and userspace can retry it
>      by writing the PCI TSM sysfs attribute again, which issues the command
>      again.
> 
>      When the device is unlocked, or locking fails, convert the buffer back
>      to private and free it. If a timed-out command is still pending at that
>      point, leak the buffer instead: the VMM may still write the response,
>      and freed pages can be reused.
> 
>      tdx_tdcm_run() is initially unused. A following patch uses it to
>      execute the TEE-IO support check during device locking.

Those sentence convey no useful information whatsoever, the 
__mabybe_unused in the code, which even gets removed in a feature patch 
already say this.

> 
>      Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> 
>      ---
>      rfcv2:
>          - move TDCM specific declaration into connect.c (Nikolay)
>          - Explain asynchronous completion and polling choice (Nikolay, Xiaoyao)
>          - Use GHCI-style names for TDCM error constants (Xiaoyao)
>          - Split TDCM helpers into a separate patch (Xiaoyao)
>          - Replace cleanup attributes with explicit cleanup paths (Rick, Xiaoyao)
>          - Keep one shared TDCM buffer per device, allocated at lock, and return
>            -EBUSY while a timed-out command is pending; leak the buffer only if
>            that command is still pending at unlock
>          - Check command and response sizes against the buffer at build time
> 
> diff --git a/drivers/virt/coco/tdx-guest/connect.c b/drivers/virt/coco/tdx-guest/connect.c
> index ead245c6b120..70bd68994259 100644
> --- a/drivers/virt/coco/tdx-guest/connect.c
> +++ b/drivers/virt/coco/tdx-guest/connect.c
> @@ -7,15 +7,94 @@
> 
>   #define pr_fmt(fmt)            KBUILD_MODNAME ": tdx_connect: " fmt
> 
> +#include <linux/iopoll.h>
>   #include <linux/pci.h>
>   #include <linux/pci-tsm.h>
> +#include <linux/set_memory.h>
>   #include <linux/tsm.h>
> +#include <linux/units.h>
>   #include <asm/tdx.h>
> 
>   #include "tdx-guest.h"
> 
> +enum tdcm_operation {
> +       TDCM_OP_CHECK_TEEIO_SUPP        = 1,
> +       TDCM_OP_BIND                    = 2,
> +       TDCM_OP_GET_DEV_INFO            = 3,
> +       TDCM_OP_GET_TDI_REPORT          = 4,
> +       TDCM_OP_START_TDI               = 5,
> +       TDCM_OP_GET_TDI_STATE           = 6,
> +       TDCM_OP_UNBIND                  = 7,
> +};
> +
> +enum tdcm_status {
> +       TDCM_STATUS_WAIT                = 0,
> +       TDCM_STATUS_COMPLETED           = 1,
> +       TDCM_STATUS_ERROR               = 2,
> +};
> +
> +enum tdcm_error {
> +       TDCM_TDX_MODULE_ERROR           = 10,
> +       TDCM_TDXIO_DEVICE_ERROR         = 11,
> +       TDCM_SPDM_MESSAGE_ERROR         = 12,
> +       TDCM_IDE_KM_MESSAGE_ERROR       = 13,
> +       TDCM_TDISP_MESSAGE_ERROR        = 14,
> +       TDCM_INVALID_STATE              = 15,
> +};
> +
> +struct tdvmcall_tdcm {
> +       __u16 operation;  /* See enum tdcm_operation */
> +       __u8  version;
> +       __u8  rsvd0;
> +       __u32 rsvd1;
> +
> +       __u8  status;     /* See enum tdcm_status */
> +       __u8  error;      /* See enum tdcm_error */
> +       __u16 rsvd2;
> +       __u32 rsvd3;
> +
> +       __u32 data_length;
> +
> +       __u8  data[];
> +} __packed;
> +
> +/*
> + * Size of the shared TDCM buffer, header included. Commands in this driver
> + * exchange at most a few bytes of data. Grow this if a command needs more,
> + * e.g. for TDI reports.
> + *
> + * Every command and response size passed to tdx_tdcm_run() is a constant
> + * that must fit in TDCM_MAX_DATA_SZ. Check each one with static_assert()
> + * next to its caller.
> + */
> +#define TDCM_BUF_SIZE          PAGE_SIZE
> +#define TDCM_MAX_DATA_SZ       (TDCM_BUF_SIZE - sizeof(struct tdvmcall_tdcm))
> +
> +/*
> + * The DOE response timeout is one second per exchange. Expect that 10 seconds
> + * is enough time for the host to complete a TDCM command.
> + */
> +#define TDCM_POLL_TIMEOUT_US   (10 * USEC_PER_SEC)
> +#define TDCM_POLL_DELAY_US     1000
> +
> +/**
> + * struct tdx_devsec - TDX Connect context for a locked TDI
> + * @pci: PCI TSM devsec context
> + * @tdcm_buf: Buffer shared with the VMM for TDCM commands, TDCM_BUF_SIZE
> + *           bytes. Allocated at lock and freed at unlock, so memory is not
> + *           converted between private and shared on every command. Only
> + *           tdx_tdcm_run() writes to it. Callers may read response data
> + *           after tdx_tdcm_run() returns 0.
> + * @tdcm_in_flight: An earlier command timed out and the VMM may still own
> + *                 @tdcm_buf. Cleared once the VMM reports its completion.
> + *
> + * All TDCM commands for a device run under its device lock, which serializes
> + * use of @tdcm_buf and @tdcm_in_flight.
> + */
>   struct tdx_devsec {
>          struct pci_tsm_devsec pci;
> +       struct tdvmcall_tdcm *tdcm_buf;
> +       bool tdcm_in_flight;
>   };
> 
>   static struct tdx_devsec *to_tdx_devsec(struct pci_tsm *tsm)
> @@ -23,6 +102,155 @@ static struct tdx_devsec *to_tdx_devsec(struct pci_tsm *tsm)
>          return container_of(tsm, struct tdx_devsec, pci.base_tsm);
>   }
> 
> +static struct pci_dev *tdx_devsec_pdev(struct tdx_devsec *tdevsec)
> +{
> +       return tdevsec->pci.base_tsm.pdev;
> +}
> +
> +static int tdx_tdcm_buf_alloc(struct tdx_devsec *tdevsec)
> +{
> +       struct tdvmcall_tdcm *buf;
> +
> +       buf = alloc_pages_exact(TDCM_BUF_SIZE, GFP_KERNEL);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       if (set_memory_decrypted((unsigned long)buf, PHYS_PFN(TDCM_BUF_SIZE))) {
> +               pci_err(tdx_devsec_pdev(tdevsec),
> +                       "Failed to convert TDCM buffer to shared, leaking it\n");
> +               return -EIO;
> +       }
> +
> +       tdevsec->tdcm_buf = buf;
> +       return 0;
> +}
> +
> +static void tdx_tdcm_buf_free(struct tdx_devsec *tdevsec)
> +{
> +       struct tdvmcall_tdcm *buf = tdevsec->tdcm_buf;
> +
> +       if (!buf)
> +               return;
> +
> +       tdevsec->tdcm_buf = NULL;
> +
> +       /*
> +        * A timed-out command may still be pending in the VMM. Leak the
> +        * buffer so a late response cannot land in the pages after they
> +        * are reused.
> +        */
> +       if (tdevsec->tdcm_in_flight &&
> +           READ_ONCE(buf->status) == TDCM_STATUS_WAIT) {

Is the read_once even needed in here?

> +               pci_warn(tdx_devsec_pdev(tdevsec),
> +                        "TDCM command still pending, leaking TDCM buffer\n");
> +               return;
> +       }
> +
> +       if (set_memory_encrypted((unsigned long)buf, PHYS_PFN(TDCM_BUF_SIZE))) {
> +               pci_err(tdx_devsec_pdev(tdevsec),
> +                       "Failed to convert TDCM buffer to private, leaking it\n");
> +               return;
> +       }
> +
> +       free_pages_exact(buf, TDCM_BUF_SIZE);
> +}
> +
> +/**
> + * tdx_tdcm_run() - Execute a TDCM command and wait for its completion
> + * @tdevsec: TDI context
> + * @operation: TDCM operation, see enum tdcm_operation
> + * @cmd_data: Command data to send, or NULL
> + * @cmd_data_sz: Size of @cmd_data in bytes, 0 for none
> + * @max_rsp_data_sz: Response data size, or the largest accepted if
> + *                  @rsp_data_sz is given. 0 for no response data.
> + * @rsp_data_sz: NULL for a fixed-size response, which must be exactly
> + *              @max_rsp_data_sz bytes. Otherwise accept 1 to
> + *              @max_rsp_data_sz bytes and return the size here.
> + *
> + * Copy @cmd_data into the shared buffer only once the VMM no longer owns it.
> + * On success, response data is at tdevsec->tdcm_buf->data. It is VMM-written:
> + * read it once or copy it out, before the next TDCM command reuses the buffer.
> + *
> + * Return: 0 on success, -EBUSY if an earlier timed-out command is still
> + * pending in the VMM, or another negative errno on failure.
> + */
> +static int __maybe_unused tdx_tdcm_run(struct tdx_devsec *tdevsec,
> +                                      u16 operation, const void *cmd_data,
> +                                      u32 cmd_data_sz, u32 max_rsp_data_sz,
> +                                      u32 *rsp_data_sz)
> +{
> +       struct pci_dev *pdev = tdx_devsec_pdev(tdevsec);
> +       struct tdvmcall_tdcm *buf = tdevsec->tdcm_buf;
> +       bool bad_len;
> +       u32 data_len;
> +       u8 status;
> +       int ret;
> +       u64 r;
> +
> +       /*
> +        * After a timeout the VMM may still be processing the earlier command
> +        * and writing its response. Don't touch the buffer until it is done,
> +        * as GetQuote does.
> +        */
> +       if (tdevsec->tdcm_in_flight) {
> +               if (READ_ONCE(buf->status) == TDCM_STATUS_WAIT)

Is it even needed in here, you don't poll on it to assume adjacent reads 
might be merged?

> +                       return -EBUSY;
> +               tdevsec->tdcm_in_flight = false;
> +       }
> +
> +       /*
> +        * Stale response data from an earlier command is VMM-written and never
> +        * trusted, so only the header needs resetting.
> +        */
> +       memset(buf, 0, sizeof(*buf));
> +       buf->operation = operation;
> +       buf->status = TDCM_STATUS_WAIT;
> +       buf->data_length = cmd_data_sz;
> +       if (cmd_data_sz)
> +               memcpy(buf->data, cmd_data, cmd_data_sz);
> +
> +       r = tdx_hcall_tdcm(pdev, virt_to_page(buf), TDCM_BUF_SIZE >> PAGE_SHIFT);
> +       if (r) {
> +               pci_err(pdev, "TDCM command %u submission failed: 0x%llx\n",
> +                       operation, r);
> +               return -EIO;
> +       }
> +
> +       ret = read_poll_timeout(READ_ONCE, status, status != TDCM_STATUS_WAIT,
Is it even needed in here as well, while you do poll it's in a fresh 
function call?

> +                               TDCM_POLL_DELAY_US, TDCM_POLL_TIMEOUT_US, false,
> +                               buf->status);
> +       if (ret) {
> +               /* The next command fails with -EBUSY until the VMM is done */
> +               tdevsec->tdcm_in_flight = true;
> +               pci_err(pdev, "TDCM command %u timed out\n", operation);
> +               return ret;
> +       }
> +
> +       /* Ensure subsequent data reads are ordered after the status observation */
> +       virt_rmb();
> +
> +       /* Read the VMM-controlled length once, then validate the cached copy */

nit: Pointless comment, it's obvious from the code what's going on.

> +       data_len = READ_ONCE(buf->data_length);
> +
> +       if (rsp_data_sz)
> +               bad_len = !data_len || data_len > max_rsp_data_sz;
> +       else
> +               bad_len = data_len != max_rsp_data_sz;
> +
> +       if (status != TDCM_STATUS_COMPLETED || bad_len) {
> +               pci_err(pdev,
> +                       "TDCM command %u failed: status 0x%x, error 0x%x, max_rsp_data_sz 0x%x, returned 0x%x\n",
> +                       operation, status, READ_ONCE(buf->error),
> +                       max_rsp_data_sz, data_len);
> +               return -EIO;
> +       }
> +
> +       if (rsp_data_sz)
> +               *rsp_data_sz = data_len;
> +
> +       return 0;
> +}
> +

<snip>