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

> [ ... 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..

> [ ... 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"

> [ ... 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

-- 
Jason