Re: [RFC PATCH 27/27] coco/tdx-host: Implement IDE stream setup/teardown

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 19 Sep 2025 07:22:36 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> From: Xu Yilun <yilun.xu@linux.intel.com>
> 
> Implementation for a most straightforward Selective IDE stream setup.
> Hard code all parameters for Stream Control Register. And no IDE Key
> Refresh support.
> 
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
A few small things in here.

Jonathan

> ---
>  drivers/virt/coco/tdx-host/tdx-host.c | 271 +++++++++++++++++++++++++-
>  1 file changed, 270 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index 258539cf0cdf..7f156d219cee 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c


> +static void tdx_ide_stream_delete(struct tdx_link *tlink)
> +{
> +	struct pci_dev *pdev = tlink->pci.base_tsm.pdev;
> +	unsigned int nr_released;
> +	u64 released_hpa, r;
> +
> +	r = tdh_ide_stream_block(tlink->spdm_id, tlink->stream_id);
> +	if (r) {
> +		pci_err(pdev, "ide stream block fail %llx\n", r);
> +		goto leak;
> +	}
> +
> +	r = tdh_ide_stream_delete(tlink->spdm_id, tlink->stream_id,
> +				  tlink->stream_mt, &nr_released,
> +				  &released_hpa);
> +	if (r) {
> +		pci_err(pdev, "ide stream delete fail %llx\n", r);
> +		goto leak;
> +	}
> +
> +	if (tdx_page_array_ctrl_release(tlink->stream_mt, nr_released,
> +					released_hpa)) {
> +		pci_err(pdev, "fail to release IDE stream metadata pages\n");
> +		goto leak;
> +	}
> +
> +	goto out;
> +
> +leak:
> +	tdx_page_array_ctrl_leak(tlink->stream_mt);
> +out:
> +	tlink->stream_mt = NULL;

Similar to the other case below. I'd just duplicate this last line
in the interests of simpler code flow.

> +}
> +
>  static void tdx_ide_stream_teardown(struct tdx_link *tlink)
>  {
> +	struct pci_dev *pdev = tlink->pci.base_tsm.pdev;
> +	struct pci_ide *ide = tlink->ide;
> +
> +	if (!ide)
> +		return;
> +
> +	pci_ide_stream_disable(pdev, ide);
> +	tsm_ide_stream_unregister(ide);
> +	tdx_ide_stream_key_stop(tlink);
> +	pci_ide_stream_teardown(pdev, ide);
> +	pci_ide_stream_unregister(ide);
> +	tdx_ide_stream_delete(tlink);
> +	pci_ide_stream_free(tlink->ide);
Use ide local variable

> +	tlink->ide = NULL;
Can you do this earlier so it's reverse of ordering in the
setup function?
>  }
>  
>  static int tdx_ide_stream_setup(struct tdx_link *tlink)
>  {
> -	return -EOPNOTSUPP;
> +	struct pci_dev *pdev = tlink->pci.base_tsm.pdev;
> +	struct pci_ide *ide;
> +	int ret;
> +
> +	ide = pci_ide_stream_alloc(pdev);
> +	if (!ide)
> +		return -ENOMEM;
> +
> +	/* Configure IDE capability for RP & get stream_id */
> +	ret = tdx_ide_stream_create(tlink, ide);
> +	if (ret)
> +		goto stream_free;
> +
> +	ide->stream_id = tlink->stream_id;
> +	ret = pci_ide_stream_register(ide);
> +	if (ret)
> +		goto tdx_stream_delete;
> +
> +	/* Configure IDE capability for target device */
> +	pci_ide_stream_setup(pdev, ide);
> +
> +	/* Key Programming for RP & target device, enable IDE stream for RP */
> +	ret = tdx_ide_stream_key_program(tlink);
> +	if (ret)
> +		goto stream_teardown;
> +
> +	ret = tsm_ide_stream_register(ide);
> +	if (ret)
> +		goto tdx_key_stop;
> +
> +	/* Enable IDE stream for target device */
> +	pci_ide_stream_enable(pdev, ide);
> +
> +	tlink->ide = ide;
> +
> +	return 0;
> +
> +tdx_key_stop:
> +	tdx_ide_stream_key_stop(tlink);
> +stream_teardown:
> +	pci_ide_stream_teardown(pdev, ide);
> +	pci_ide_stream_unregister(ide);
> +tdx_stream_delete:
> +	tdx_ide_stream_delete(tlink);
> +stream_free:
> +	pci_ide_stream_free(tlink->ide);
(ide) I think because...
> +	tlink->ide = NULL;
How is this set in any path that gets here?  Looks
like it is only assigned right at the end after all error paths.

> +	return ret;
>  }
>  
>  static void __tdx_link_disconnect(struct tdx_link *tlink)