Re: [PATCH v6 13/15] iommu/amd: Initial SVA support for AMD IOMMU

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Feb 09, 2024 at 11:29:28AM +0000, Vasant Hegde wrote:
> @@ -560,6 +567,16 @@ enum protection_domain_mode {
>  	PD_MODE_V2,
>  };
>  
> +/* Track dev_data/PASID list for the protection domain */
> +struct pdom_dev_data {
> +	/* Points to attached device data */
> +	struct iommu_dev_data *dev_data;
> +	/* PASID attached to the protection domain */
> +	ioasid_t pasid;
> +	/* For protection_domain->dev_data_list */
> +	struct list_head list;

'item' is a much better name than 'list'

> +int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
> +			    struct device *dev, ioasid_t pasid)
> +{
> +	struct pdom_dev_data *pdom_dev_data;
> +	struct protection_domain *sva_pdom = to_pdomain(domain);
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> +	unsigned long flags;
> +	int ret = -EINVAL;
> +
> +	/* PASID zero is used for requests from the I/O device without PASID */
> +	if (!is_pasid_valid(dev_data, pasid))
> +		return ret;
> +
> +	/* Make sure PASID is enabled */
> +	if (!is_pasid_enabled(dev_data))
> +		return ret;
> +
> +	/* Add PASID to protection domain pasid list */
> +	pdom_dev_data = kzalloc(sizeof(*pdom_dev_data), GFP_KERNEL);
> +	if (pdom_dev_data == NULL)
> +		return ret;
> +
> +	pdom_dev_data->pasid = pasid;
> +	pdom_dev_data->dev_data = dev_data;
> +
> +	spin_lock_irqsave(&sva_pdom->lock, flags);
> +
> +	/* Setup GCR3 table */
> +	ret = amd_iommu_set_gcr3(dev_data, pasid,
> +				 iommu_virt_to_phys(domain->mm->pgd));
> +	if (ret) {
> +		kfree(pdom_dev_data);
> +		goto out_unlock;

BTW, I'm not confident in any of the error unwinds around command
execution failure. The sync failed, it doesn't mean the HW didn't
already load the new CD table entry. The driver is basically totally
wrecked at this point as it can't assume the new entry hasn't been
read and it can't assume the old entry is flushed out. :|

Otherwise looks good:

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason