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

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,


On 3/19/2024 11:26 PM, Jason Gunthorpe wrote:
> 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..

Currently we ignore invalidation return value. So we go to failure path only if
we fail to add PASID to GCR3 table.

-Vasant