Re: [PATCH v2] iommu/amd: Use report_iommu_fault()

["Suthikulpanit, Suravee via iommu" <iommu@lists.linux-foundation.org>]

Lennert,

On 7/29/2021 9:32 PM, Lennert Buytenhek wrote:
> We have three cases to handle:
> 
> - EVENT_FLAG_I set: IRQ remapping fault, don't call report_iommu_fault()
> 
> - EVENT_FLAG_I unset, but the request was a translation request
>    (EVENT_FLAG_TR set) or the target page was not present (EVENT_FLAG_PR
>    unset): call report_iommu_fault(), but the RW bit will be invalid, so
>    don't try to map it to a IOMMU_FAULT_{READ,WRITE} code

So, why do we need to call report_iommu_fault() for this case?
My understanding is we only have IOMMU_FAULT_[READ|WRITE].
So, if we can't identify whether the DMA is read / write,
we should not need to call report_iommu_fauilt(), is it?

> - EVENT_FLAG_I unset, the request is a transaction request (EVENT_FLAG_TR
>    unset) and the target page was present (EVENT_FLAG_PR set): call
>    report_iommu_fault(), and use the RW bit to set IOMMU_FAULT_{READ,WRITE}
> 
> So I don't think we can merge the test for EVENT_FLAG_I with the
> test for EVENT_FLAG_TR/EVENT_FLAG_PR.

The only condition that we would report_iommu_fault is
I=0, TR=0, PR=1, isn't it. So we should be able to just check if PR=1.


> We could do something like this, if you'd prefer:
> 
> 	#define IS_IOMMU_MEM_TRANSACTION(flags)	\
> 		(((flags) & EVENT_FLAG_I) == 0)
> 
> 	#define IS_RW_FLAG_VALID(flags)		\
> 		(((flags) & (EVENT_FLAG_TR | EVENT_FLAG_PR)) == EVENT_FLAG_PR)
> 
> 	#define IS_WRITE_REQUEST(flags)		\
> 		(IS_RW_FLAG_VALID(flags) && (flags & EVENT_FLAG_RW))
> 
> And then do something like:
> 
> 	if (dev_data && IS_IOMMU_MEM_TRANSACTION(flags)) {
> 		if (!report_iommu_fault(&dev_data->domain->domain, &pdev->dev,
> 					address,
> 					IS_WRITE_REQUEST(flags) ?
> 					IOMMU_FAULT_WRITE : IOMMU_FAULT_READ))

Actually, IS_WRITE_REQUEST() == 0 could mean:
- I=0, TR=0, PR=1 and RW=0: This is fine.
- I=0, (TR=1 or PR=0), and we should not be calling report_iommu_fault() here
   since we cannot specify READ/WRITE here.

Thanks,
Suravee

> 			goto out;
> 	}

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu