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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Nikolay Borisov <nik.borisov@xxxxxxxx>
>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@xxxxxxxxx>
>> ---
>>   drivers/virt/coco/tdx-guest/connect.c | 75 +++++++++++++++++++++++++++
>>   1 file changed, 75 insertions(+)
>>
>> diff --git a/drivers/virt/coco/tdx-guest/connect.c
>b/drivers/virt/coco/tdx-guest/connect.c
>> index 1f52a187b30f..e36515fc43ef 100644
>> --- a/drivers/virt/coco/tdx-guest/connect.c
>> +++ b/drivers/virt/coco/tdx-guest/connect.c
>> @@ -159,8 +159,67 @@ static bool tdx_is_dev_teeio_support(struct
>tdx_devsec *tdevsec)
>>   	return !!rsp->is_supported;
>>   }
>>
>
><snip>
>
>> +
>>   static struct pci_tsm *tdx_devsec_lock(struct tsm_dev *tsm_dev, struct
>pci_dev *pdev)
>>   {
>> +	u8 state;
>>   	int ret;
>>
>>   	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);
>> +
>> +	retain_and_null_ptr(tdevsec_bind);
>> +
>>   	return &no_free_ptr(tdevsec)->pci.base_tsm;
>>   }
>>
>> @@ -181,6 +253,9 @@ static void tdx_devsec_unlock(struct pci_tsm *tsm)
>>   {
>>   	struct tdx_devsec *tdevsec = to_tdx_devsec(tsm);
>>
>> +	if (WARN_ON(tdx_tdi_unbind_dev(tdevsec)))
>> +		return;
>
><Thinking out loud>
>
>So unlock involves calling TDCM_UNBIND which in turn might fail for any
>number of reasons and yet we have no way of communicating that. So after
>an unlock a TD might want to rebind and issue bind on a device which is
>already bound because prior unbind failed. Is this something we should
>be worried about ?

I completely agree with your point here.
We need pci_tsm_ops::unlock() in TSM framework to return an error status
(e.g., int) so that failures in the underlying hardware-level unbind can
be bubbled up to the caller rather than silently ignored.

Thanks
Zhenzhong