Re: [PATCH v3 02/12] iommu/amd: Do not override PASID entry in GCR3 table

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

On 11/6/2023 11:21 PM, Jason Gunthorpe wrote:
> On Mon, Oct 16, 2023 at 10:43:41AM +0000, Vasant Hegde wrote:
>> Return error if PASID is already exists.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/iommu.c | 3 +++
>>  1 file changed, 3 insertions(+)
> 
> Why is this an error?
> 
> ops->set_dev_pasid() should replace any current translation at any
> PASID with the requested new translation. It is not an error to call
> it while the PASID already has a translation.

My bad. I did mess up this patch when I rebased. Check should be in __set_gcr3()
 (like below):

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 691c040216e3..558e0c196172 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1879,6 +1879,9 @@ static int __set_gcr3(struct iommu_dev_data *dev_data,
 	if (pte == NULL)
 		return -ENOMEM;

+	if (*pte & GCR3_VALID)
+		return -EBUSY;
+
 	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
 	amd_iommu_dev_flush_pasid_all(dev_data, pasid);



I believe as a sane design it should first remove existing mapping (that's why
we have remove_dev_pasid() interface) before adding new mapping. That's why I
introduced this patch.

-Vasant