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

[Xu Yilun <yilun.xu@linux.intel.com>]

On Mon, Nov 10, 2025 at 04:51:21PM -0800, dan.j.williams@intel.com wrote:
> 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);

It works for sure. I have also thought about this solution, but dropped.
I don't wanna see the usage of auto-cleanup impose too much influences
to the code/structure design, even if it shows better modularity.

It is normal in kernel that a base structure contains several
sub-features and we just group them with blank lines. Some fields may be
used accoss 1-2 features and doesn't have a clear owner. If we ask for
strict structure/flow design for auto-cleanup, people may reluctant to
switch to auto-cleanup, thought is it over-engineering?

IOW, I like this current piece of code cause it is in perfect balance.
I don't have to change the mindset much for code design. I get the benifit
of auto-cleanup, and the local cleanup handlers (tlink_spdm, tlink_ide)
are cheap but clearly tell me what will happen if any step fails.

> 
> > 	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,

So if we don't do auto-cleanup for the last one, do we still need a
structure for that? If not,

 struct tdx_link {
	struct tdx_spdm *spdm;
	
	int ide_stream_field1;
	int ide_stream_field2;
	...
 }

seems so wierd.

> but maybe there are other PCI/TSM core things to do that
> are not shown.

There is no following items for now but I think cleanup for the last one
is good. Otherwise we may face with the same problem as goto, that we
see unrelated changes (add cleanup for previous one) when we add a new
step.

Thanks,
Yilun