Re: [RFC PATCH 20/27] coco/tdx-host: Add connect()/disconnect() handlers prototype

[<dan.j.williams@intel.com>]

Xu Yilun wrote:
> On Mon, Nov 03, 2025 at 03:34:15PM -0800, dan.j.williams@intel.com wrote:
> > Jonathan Cameron wrote:
> > > On Fri, 19 Sep 2025 07:22:29 -0700
> > > Dan Williams <dan.j.williams@intel.com> wrote:
> > > 
> > > > From: Xu Yilun <yilun.xu@linux.intel.com>
> > > > 
> > > > Add basic skeleton for connect()/disconnect() handlers. The major steps
> > > > are SPDM setup first and then IDE selective stream setup.
> > > > 
> > > > No detailed TDX Connect implementation.
> > > > 
> > > > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > Feels like use of __free() in here is inappropriate to me.
> > > 
> > > > ---
> > > >  drivers/virt/coco/tdx-host/tdx-host.c | 49 ++++++++++++++++++++++++++-
> > > >  1 file changed, 48 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> > > > index f5a869443b15..0d052a1acf62 100644
> > > > --- a/drivers/virt/coco/tdx-host/tdx-host.c
> > > > +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> > > > @@ -104,13 +104,60 @@ static int __maybe_unused tdx_spdm_msg_exchange(struct tdx_link *tlink,
> > > >  	return ret;
> > > 
> > > > +
> > > > +static void __tdx_link_disconnect(struct tdx_link *tlink)
> > > > +{
> > > > +	tdx_ide_stream_teardown(tlink);
> > > > +	tdx_spdm_session_teardown(tlink);
> > > > +}
> > > > +
> > > > +DEFINE_FREE(__tdx_link_disconnect, struct tdx_link *, if (_T) __tdx_link_disconnect(_T))
> > > > +
> > > >  static int tdx_link_connect(struct pci_dev *pdev)
> > > >  {
> > > > -	return -ENXIO;
> > > > +	struct tdx_link *tlink = to_tdx_link(pdev->tsm);
> > > > +	int ret;
> > > > +
> > > > +	struct tdx_link *__tlink __free(__tdx_link_disconnect) = tlink;
> > > I'm not a fan on an ownership pass like this just for purposes of cleaning up.
> > 
> > Yeah this needs a rethink. The session and the stream are independent
> > resources. It can be a composite object that encapsulates both
> > resources, but not tlink directly.
> > 
> > ...chalk this up to RFC expediency.
> > 
> > > I'd be a bit happier if you could make it
> > > 	struct tdx_link *tlink __free(__tdx_link_disconnect) = to_tdx_link(pdev->dsm);
> > > 
> > > but I still don't really like it.  I think I'd just not use __free and stick
> > > to traditional cleanup in via a goto. 
> > 
> > I would not go that far, but certainly I can see that being preferable
> > than reusing the existing base 'struct tdx_link *' as the cleanup
> > variable.
> 
> The latest implementation internally is as follows. tlink_spdm &
> tlink_ide represent independent resources though they point to the same
> instance. I'm already comfortable about this code:
> 
> static int tdx_link_connect(struct pci_dev *pdev)
> {
> 	struct tdx_link *tlink = to_tdx_link(pdev->tsm);
> 
> 	struct tdx_link *tlink_spdm __free(tdx_spdm_session_teardown) =
> 		tdx_spdm_session_setup(tlink);

The question I have is why does tdx_spdm_session_setup() return a
'struct tdx_link' instance and not a new 'struct tdx_spdm' object to
represent the new resources that were acquired? 'struct tdx_link' is
base infrastructure created by ->probe(). Perhaps 'struct tdx_spdm'
could be:

struct tdx_spdm {
       u64 spdm_id;
       struct page *spdm_conf;
       struct tdx_page_array *spdm_mt;
}

...and then tdx_link becomes:

struct tdx_link {
	...
	struct tdx_spdm spdm;
};

...and you can do:

       struct tdx_spdm *spdm __free(tdx_spdm_session_teardown) =
               tdx_spdm_session_setup(tlink);

tlink->spdm = *no_free_ptr(spdm);

...to assign it back to the preallocated space in @tlink, or make
it dynamically allocated.

struct tdx_link {
	...
	struct tdx_spdm *spdm;
};

tlink->spdm = no_free_ptr(spdm);

> 	if (IS_ERR(tlink_spdm)) {
> 		pci_err(pdev, "fail to setup spdm session\n");
> 		return PTR_ERR(tlink_spdm);
> 	}
> 
> 	struct tdx_link *tlink_ide __free(tdx_ide_stream_teardown) =
> 		tdx_ide_stream_setup(tlink);
> 	if (IS_ERR(tlink_ide)) {
> 		pci_err(pdev, "fail to setup ide stream\n");
> 		return PTR_ERR(tlink_ide);
> 	}

No strict need for scope-based cleanup if this is the last resource
acquisition, but maybe there are other PCI/TSM core things to do that
are not shown.