RE: [RFC PATCH 02/15] x86/tdx: Add TDCM hypercall wrapper for TDX Connect

[Duan, Zhenzhong]

>-----Original Message-----
>From: Li, Xiaoyao <xiaoyao.li@xxxxxxxxx>
>Subject: Re: [RFC PATCH 02/15] x86/tdx: Add TDCM hypercall wrapper for TDX
>Connect
>
>On 9/24/2026 12:10 PM, Zhenzhong Duan wrote:
>> TDX Connect is a TDX feature that enables secure device assignment,
>> allowing a TDX guest to directly interact with TEE-IO capable devices.
>>
>> The TEE-IO Device Control and Management (TDCM) hypercall
>> (TDG.VP.VMCALL<TDCM>, defined in TDX GHCI v2.0 specification) is the
>> primary interface through which a TDX guest issues device management
>> commands to the host. The host processes these commands and returns
>> responses via a shared memory buffer.
>
>Could we add some clarification of why this guest-host hypercall is
>necessary?
>i.e., explain that some device management operations can only be performed
>by
>the host VMM.

OK, will explain this in the changelog:
Some device management operations require sending TDISP messages to the
TDI device, which the guest can not perform directly. For example, when
the TDI is a VF, it has no DOE mailbox of its own — TDISP messages must
go through the PF's DOE mailbox, which is inaccessible to the guest. The
host must therefore perform these operations on the guest's behalf.

>
>> Abstract the underlying leaf call and pass the shared buffer physical
>> address as a decrypted mapping. Add a runtime page alignment check
>> for the buffer to enforce correct usage.
>
>This doesn't match the implementation. The code only WARNs instead of
>enforcing
>correct usage.

You mean below code?

static void tdx_devsec_unlock(struct pci_tsm *tsm)
{
        struct tdx_devsec *tdevsec = to_tdx_devsec(tsm);
        struct device_evidence *evidence = tsm->evidence;

        if (WARN_ON(tdx_tdi_unbind_dev(tdevsec)))
                return;

I think the unlock() callback should be defined with a return value so
userspace could catch the error.

>
>> Place the implementation in a separate tdx_connect.c file to keep TDX
>> Connect specific code isolated from core tdx.c.
>>
>> CONFIG_TDX_CONNECT_GUEST referenced in the header guard will be
>> introduced in a later patch in this series.
>
>I'm not sure why the CONFIG cannot be introduced in this patch...

No fundamental reason it cannot. I'd like to group the Kconfig option
with the TDX Connect driver support so reviewers can see which feature
this CONFIG is used for easily. Just my preference, I'm not clear if
there is a hard rule on this.

>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@xxxxxxxxx>
>> ---
>>  arch/x86/coco/tdx/Makefile        |  2 ++
>>  arch/x86/coco/tdx/tdx_connect.c   | 32 +++++++++++++++++++++++++++++++
>>  arch/x86/include/asm/shared/tdx.h |  1 +
>>  arch/x86/include/asm/tdx.h        |  4 ++++
>>  4 files changed, 39 insertions(+)
>>  create mode 100644 arch/x86/coco/tdx/tdx_connect.c
>>
>> diff --git a/arch/x86/coco/tdx/Makefile b/arch/x86/coco/tdx/Makefile
>> index b3c47d3700e2..2ab2dbbdd3d2 100644
>> --- a/arch/x86/coco/tdx/Makefile
>> +++ b/arch/x86/coco/tdx/Makefile
>> @@ -1,3 +1,5 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>
>>  obj-y += debug.o tdcall.o tdx.o tdx-shared.o
>> +
>> +obj-$(CONFIG_TDX_CONNECT_GUEST) += tdx_connect.o
>> diff --git a/arch/x86/coco/tdx/tdx_connect.c
>b/arch/x86/coco/tdx/tdx_connect.c
>> new file mode 100644
>> index 000000000000..0c6ca650771a
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/tdx_connect.c
>> @@ -0,0 +1,32 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (C) 2026 Intel Corporation */
>> +
>> +#undef pr_fmt
>> +#define pr_fmt(fmt)     "tdx_connect: " fmt
>
>I think this is file is only intended to hold guest stuff. Thus maybe call
>out
>"guest" as
>
>  #define pr_fmt(fmt)     "tdx_connect guest: " fmt
>
>though arch/x86/coco/tdx/tdx.c doesn't print "guest":
>
>  #define pr_fmt(fmt)     "tdx: " fmt
>
>So this is just my preference, feel free to ignore it.

OK, I'd like to keep it as is, user can easily know it's a guest or host
by looking around other messages, appending each message with "guest"
looks unnecessary.

>
>> +
>> +#include <linux/io.h>
>> +#include <asm/tdx.h>
>> +#include <linux/mm.h>
>> +
>> +/*
>> + * tdx_hcall_tdcm - Send a TDCM command to the host via
>TDG.VP.VMCALL<TDCM>.
>> + *
>> + * @devid:  Device identifier of the target TEE-IO device.
>> + * @buf:    Shared, directly mapped buffer containing the TDCM command
>on
>> + *          input and the host response on output.
>> + * @size:   Size of @buf in bytes.
>> + * @vector: Event notification interrupt vector, or 0 for synchronous
>operation.
>> + *
>
>"0 for synchronous operation" isn't correct.
>
>vector 0 means "no vector. TD polls the status for completion". If the
>command
>doesn't finish when TDG.VP.VMCALL<TDCM> returns. It's not a synchronous
>operation.

You are absolutely right, will change the parameter description to:

 * @vector: Event notification interrupt vector, or 0 to poll for completion.

Thanks
Zhenzhong