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

["Duan, Zhenzhong" <zhenzhong.duan@intel.com>]

>-----Original Message-----
>From: Nikolay Borisov <nik.borisov@suse.com>
>Subject: Re: [RFC PATCH 06/15] virt: tdx-guest: Support TDI bind and unbind
>operations
>
>
>
>On 24.09.26 г. 7:10 ч., Zhenzhong Duan wrote:
>> Introduce support for binding and unbinding the Trust Device Interface
>> (TDI) using the TDCM_OP_BIND and TDCM_OP_UNBIND commands.
>>
>> Executing TDCM_OP_BIND transitions the TDI state to a locked state
>> (TDI_STATE_CONFIG_LOCKED). Add a sanity check following the bind operation
>> to verify this state transition succeeded before finalizing device locking.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
><snip>
>
>>   	struct tdx_devsec *tdevsec __free(kfree) = kzalloc(sizeof(*tdevsec),
>GFP_KERNEL);
>> @@ -174,6 +233,19 @@ static struct pci_tsm *tdx_devsec_lock(struct tsm_dev
>*tsm_dev, struct pci_dev *
>>   	if (!tdx_is_dev_teeio_support(tdevsec))
>>   		return ERR_PTR(-EOPNOTSUPP);
>>
>> +	struct tdx_devsec *tdevsec_bind __free(tdx_tdi_unbind_dev) =
>tdx_tdi_bind_dev(tdevsec);
>> +	if (IS_ERR(tdevsec_bind))
>> +		return ERR_CAST(tdevsec_bind);
>> +
>> +	ret = tdx_tdi_read_state(tdevsec, &state);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	if (state != TDI_STATE_CONFIG_LOCKED)
>> +		return ERR_PTR(-EIO);
>
>While this does seem correct, it's not really following the spec or I'm being too
>rigid, namely:
>
>Since TDI can change the state any time, TD can't fully rely on the Known RUN
>state in any scenario. However, TD can rely on the Known
>RUN state after TDG.TDI.START as a confirmation of the Interface Start.

About TDI changing state any time, do you mean host changes TDI state,
e.g., through TDH.TDI.STOP? Or you mean TDI device goes into TDISP error state
due to hw error? Can't they happen even after TDG.TDI.START?

>
>But here we still haven't done a START, just a BIND, no ?

Yes

>
> From the Guest perspective, querying the INTERFACE_STATE is a correct way for
>TD to query the TDI status. If the call status
>is TDX_SUCCESS then the TDI is BOUND. Other statuses indicate that the TDI is
>UNBOUND:
>TDX_TDIMT_NOT_PRESENT or TDX_TDI_INVALID_METADATA mean the TDI is
>UNBOUND and the TDI control structure
>has been removed or reassigned.
>TDX_TDI_INVALID_STATE means TDI is UNBOUND or in the ERROR state (TDI in
>TDISP ERROR state is considered
>UNBOUND from TD's perspective).
>
>IIUC it should be sufficient to check whether tdx_tdi_read_state didn't return an
>error and that would imply the interface is BOUND?
Updated todo list

I understand this as a best-effort sanity check, not a guarantee that
the interface will remain in this state.

For example, suppose the host creates the TDI but incorrectly reports 
BIND complete without completing TDH.TDI.BIND. In the module 
implementation I’m looking at, TDG.TDI.RD(INTERFACE_STATE) returns the 
stored TDISP state without requiring the internal TDI state to be BOUND. 
A successful read could therefore return CONFIG_UNLOCKED.

At this point we have bound the interface but have not started it, so 
CONFIG_LOCKED is the state we expect. Checking it catches that 
inconsistency; checking only the TDCALL return code would not. This is 
still a snapshot, not proof against a subsequent state change.

BRs,
Zhenzhong