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

["Suthikulpanit, Suravee" <suravee.suthikulpanit@amd.com>]

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