Re: [PATCH v2 23/31] coco/tdx-host: Setup all trusted IOMMUs on TDX Connect init

[Ackerley Tng]

Xu Yilun <yilun.xu@xxxxxxxxxxxxxxx> writes:

> Setup all trusted IOMMUs on TDX Connect initialization and clear all on
> TDX Connect removal.
>
> Trusted IOMMU setup is the pre-condition for all following TDX Connect
> operations such as SPDM/IDE setup. It is more of a platform
> configuration than a standalone IOMMU configuration, so put the
> implementation in tdx-host driver.
>

I was wondering about hotplug. There's dmar_device_hotplug(). Does it
make sense to do TDH.IOMMU.SETUP from there?

For boot, is the platform-level configuration due to dependencies? Is it
because TDX is set up after IOMMUs and hence TDH.IOMMU.SETUP is done
from the creation of the faux device?

> There is no dedicated way to enumerate which IOMMU devices support
> trusted operations. The host has to call TDH.IOMMU.SETUP on all IOMMU
> devices and tell their trusted capability by the return value.
>
>
> [...snip...]
>
> @@ -119,6 +120,82 @@ static void unregister_link_tsm(void *link)
>  	tsm_unregister(link);
>  }
>
> +static DEFINE_XARRAY(tlink_iommu_xa);
> +
> +static void tdx_iommu_clear(u64 iommu_id, struct tdx_page_array *iommu_mt)
> +{
> +	u64 r;
> +
> +	r = tdh_iommu_clear(iommu_id, iommu_mt);
> +	if (r) {
> +		pr_err("fail to clear tdx iommu 0x%llx\n", r);
> +		goto leak;
> +	}
> +
> +	if (tdx_page_array_ctrl_release(iommu_mt, iommu_mt->nr_pages,
> +					virt_to_phys(iommu_mt->root))) {
> +		pr_err("fail to release iommu_mt pages\n");
> +		goto leak;
> +	}
> +
> +	return;
> +
> +leak:
> +	tdx_page_array_ctrl_leak(iommu_mt);
> +}
> +
> +static int tdx_iommu_enable_one(struct dmar_drhd_unit *drhd)
> +{
> +	unsigned int nr_pages = tdx_sysinfo->connect.iommu_mt_page_count;
> +	u64 r, iommu_id;
> +	int ret;
> +
> +	struct tdx_page_array *iommu_mt __free(tdx_page_array_free) =
> +		tdx_page_array_create_iommu_mt(1, nr_pages);
> +	if (!iommu_mt)
> +		return -ENOMEM;
> +
> +	r = tdh_iommu_setup(drhd->reg_base_addr, iommu_mt, &iommu_id);
> +	/* This drhd doesn't support tdx mode, skip. */
> +	if ((r & TDX_SEAMCALL_STATUS_MASK)  == TDX_OPERAND_INVALID)

Extra space here before ==.

Is there a chance that TDX_OPERAND_INVALID might be returned for some
other reason other than that the drhd doesn't support TDX? I'm concerned
that some drhd would be silently skipped for some other error.

> +		return 0;
> +
> +	if (r) {
> +		pr_err("fail to enable tdx mode for DRHD[0x%llx]\n",
> +		       drhd->reg_base_addr);
> +		return -EFAULT;
> +	}
> +
> +	ret = xa_insert(&tlink_iommu_xa, (unsigned long)iommu_id,
> +			no_free_ptr(iommu_mt), GFP_KERNEL);
> +	if (ret) {
> +		tdx_iommu_clear(iommu_id, iommu_mt);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void tdx_iommu_disable_all(void *data)
> +{
> +	struct tdx_page_array *iommu_mt;
> +	unsigned long iommu_id;
> +
> +	xa_for_each(&tlink_iommu_xa, iommu_id, iommu_mt)
> +		tdx_iommu_clear(iommu_id, iommu_mt);
> +}
> +

Is there an equivalent teardown operation? Like on kexec, if TDX is torn
down, would tdx_iommu_disable_all() need to be called to undo
everything, so that after TDX is started again tdx_iommu_enable_all()
would be set up again?

For hot-unplug, would the TDX IOMMU config need to be disabled?

> +static int tdx_iommu_enable_all(void)
> +{
> +	int ret;
> +
> +	ret = do_for_each_drhd_unit(tdx_iommu_enable_one);
> +	if (ret)
> +		tdx_iommu_disable_all(NULL);
> +
> +	return ret;
> +}
> +
>  static int __maybe_unused tdx_connect_init(struct device *dev)
>  {
>  	struct tsm_dev *link;
> @@ -130,6 +207,14 @@ static int __maybe_unused tdx_connect_init(struct device *dev)
>  	if (!(tdx_sysinfo->features.tdx_features0 & TDX_FEATURES0_TDXCONNECT))
>  		return 0;
>
> +	ret = tdx_iommu_enable_all();
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Enable tdx iommu failed\n");
> +
> +	ret = devm_add_action_or_reset(dev, tdx_iommu_disable_all, NULL);
> +	if (ret)
> +		return ret;
> +
>  	link = tsm_register(dev, &tdx_tsm_link_ops);
>  	if (IS_ERR(link))
>  		return dev_err_probe(dev, PTR_ERR(link),
> --
> 2.25.1