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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Li, Xiaoyao <xiaoyao.li@xxxxxxxxx>
>Subject: Re: [RFC PATCH 06/15] virt: tdx-guest: Support TDI bind and unbind
>operations
>
>On 10/19/2026 10:16 AM, Duan, Zhenzhong wrote:
>>>> +
>>>>   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.
>
>please remember raising this when folks are discussing generic guest TSM
>framework series.

Sure.

>
>Btw, for TDX specific tdx_devsec_unlock(), the WARN_ON() is not proper
>IMHO.
>the failure can happen for various reasons, and the WARN can be treated as
>fatal
>when panic_on_warn is true. I think pr_warn_ratelimit() would suffice?

OK, but unlock() isn't a loop operation, I prefer to use pr_warn().

Thanks
Zhenzhong