Re: [PATCH 13/24] iommu/amd: Assign IOMMU Private Address domain to IOMMU

[Suthikulpanit, Suravee]

On 8/25/2026 12:24 AM, Jason Gunthorpe wrote:
> > [ ... 39 lines skipped ... ]
> > +void amd_iommu_free_dev_data(struct amd_iommu *iommu,
> > +			     struct iommu_dev_data *dev_data)
> > +{
> > +	struct amd_iommu_pci_seg *pci_seg = iommu->pci_seg;
> > +	struct llist_node *prev = NULL, *node;
> > +
> > +	if (!dev_data)
> > +		return;
> > +
> > +	for (node = pci_seg->dev_data_list.first; node;
> > +	     prev = node, node = node->next) {
> > +		if (node == &dev_data->dev_data_list) {
> > +			if (prev)
> > +				prev->next = node->next;
> > +			else
> > +				pci_seg->dev_data_list.first = node->next;
> > +			break;
>
> Why is this open coding llist manipulation? That stuff is tricky, if
> you can't do the mutation you want using the existing helpers then you
> probably shouldn't be using llist..

For v5, I'm replacing the per-segment llist of iommu_dev_data with an 
xarray keyed by devid so lookup is O(1) and a given id can be removed on 
teardown.

> > [ ... 45 lines skipped ... ]
> > +static void set_dte_ipa(struct amd_iommu *iommu, struct dev_table_entry *new)
> > +{
>
> This is a strange name for a function that fills a DTE with the paging
> domain for the "viommu_pdom"

Removing this in V5.

> > [ ... 7 lines skipped ... ]
> >   int __init amd_viommu_init(struct amd_iommu *iommu)
> >   {
> >   	int ret;
> > +	bool dte_set = false;
> > +	struct dev_table_entry new = {};
> >   
> >   	if (!amd_iommu_viommu ||
> >   	    !check_feature(FEATURE_VIOMMU))
> >   		return 0;
> >   
> > +	iommu->viommu_dev_data = amd_iommu_alloc_dev_data(iommu, iommu->devid);
> > +	if (!iommu->viommu_dev_data) {
> > +		pr_err("%s: Failed to allocate dev_data\n", __func__);
> > +		return -ENOMEM;
> > +	}
> > +	iommu->viommu_dev_data->dev = &iommu->dev->dev;
> > +
> >   	ret = viommu_init_pci_vsc(iommu);
> >   	if (ret)
> > -		return ret;
> > +		goto err_dev_data;
> >   
> >   	ret = viommu_vf_vfcntl_init(iommu);
> >   	if (ret)
> > -		return ret;
> > +		goto err_dev_data;
> >   
> >   	amd_viommu_gid_ida_init(iommu);
> >   
> > @@ -318,5 +355,15 @@ int __init amd_viommu_init(struct amd_iommu *iommu)
> >   	if (ret)
> >   		return ret;
> >   
> > +	/* Set DTE for IOMMU device */
> > +	amd_iommu_make_clear_dte(iommu, iommu->devid, &new);
> > +	set_dte_ipa(iommu, &new);
> > +	amd_iommu_update_dte(iommu, iommu->viommu_dev_data, &new);
> > +	dte_set = true;
>
> Why split this so far from the alloc_dev_data() ? The dte set cannot
> even fail?
>
> If you make the alloc and set one function then the free function
> viommu_free_self_dev_data() will naturally pair and no need for the
> weird dte_set

Ok, I'll clean up this part in V5.

Thanks,
Suravee