Re: [PATCH v2 4/4] coco/tdx-host: Initialize trusted DMA on all capable IOMMUs

[Baolu Lu <baolu.lu@linux.intel.com>]

On 10/15/2026 9:31 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] virt/coco/tdx-host: desynchronized host/hardware state on xa_insert failure
> --
> 
> commit 83cda44603ec97cb10640905999d9a617eced100
> Author: Lu Baolu <baolu.lu@linux.intel.com>
> 
> coco/tdx-host: Initialize trusted DMA on all capable IOMMUs
> 
> Assigning a device to a TD for private memory access requires that DMA
> from that device be translated by structures the TDX module controls.
> An Intel IOMMU that implements the trusted DMA extension provides a
> parallel set of translation and invalidation resources for this purpose.
> 
>> diff --git a/drivers/virt/coco/tdx-host/iommu.c b/drivers/virt/coco/tdx-host/iommu.c
>> new file mode 100644
>> index 0000000000000..a80e57bf26a12
>> --- /dev/null
>> +++ b/drivers/virt/coco/tdx-host/iommu.c
> [ ... ]
>> +static int tdx_iommu_setup(struct dmar_drhd_unit *drhd)
>> +{
> [ ... ]
>> +	state = no_free_ptr(iommu_state);
>> +	state->drhd = drhd;
>> +	state->tdx_iommu_id = tdx_iommu_id;
>> +	state->mt_pages = no_free_ptr(iommu_mt);
>> +	ret = xa_insert(&tdx_iommu_xa, (unsigned long)tdx_iommu_id, state, GFP_KERNEL);
>> +	if (ret) {
>> +		WARN_ON(tdx_iommu_clear(state));
>> +		kfree(state);
>> +		return ret;
>> +	}
> 
> [Severity: High]
> If xa_insert() fails and tdx_iommu_clear(state) also fails, this code
> unconditionally frees state and returns an error. Does this leave the
> hardware state desynchronized?
> 
> When this function returns an error, the caller dmar_tdxcs_iommu_init_all() in
> drivers/iommu/intel/dmar.c restores iommu->max_domain_id to its full un-halved
> range and leaves drhd->tdx_mode = false:
> 
> 	ret = setup(drhd);
> 	...
> 	if (ret) {
> 		scoped_guard(mutex, &iommu->did_lock)
> 			iommu->max_domain_id = ndoms;
> 		...
> 		return ret;
> 	}
> 
> Because tdx_iommu_clear() failed, the physical hardware is permanently left in
> Secure TDX Mode, but the software state now believes it is not. Will this
> cause hardware faults or fatal domain ID conflicts when the host attempts to
> use domain IDs that now belong exclusively to TDX?
> 
> Unlike tdx_iommu_disable_all() which safely detects a clear failure and leaves
> tdx_mode true to confine the host to the lower half of the DID space, this
> error path completely loses track of the hardware state, breaking isolation.

That's correct in theory, but I don't think the extra complexity is
worth it here.

The path needs two independent failures happening together:

  - xa_insert() has to fail. In practice that means a small GFP_KERNEL
    allocation failing, or the TDX module returning a duplicate
    tdx_iommu_id, which would be a TDX module bug.

  - TDH.IOMMU.CLEAR then has to fail on an IOMMU the TDX module set up
    successfully moments earlier, with nothing in between.

If both happen, the system is already badly broken, and the WARN_ON()
makes it loud. Adding more tracking just for this case would complicate
the code without making a working system more robust.

Thanks,
baolu