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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Mar 11, 2024 at 04:41:51PM +0530, Vasant Hegde wrote:

> >> +	/* 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. :|
> 
> Yep. This is tricky path.
> 
> But for now this particular one just adding PASID to GCR3 table. So if we enter
> error path means we didn't add PASID to gcr3 table. So it should work fine.

Not quite. The HW could have cached the new GCR3 table entry even
though the invalidation failed.

So on the failure path you have to assume the HW is still using the
GCR3 which the failure path is busy freeing..

There is no way to make this work right. Either kill the kernel on
invalidate failures (ie try forever) or assume that the HW will
eventually become consistent and continue forward..

Jason