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

[Lennert Buytenhek <buytenh@wantstofly.org>]

On Thu, Aug 05, 2021 at 11:26:25AM -0500, Suthikulpanit, Suravee wrote:

> Lennert,

Hi Suravee,


> > - 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?

I don't think that we should just altogether avoid logging the subset
of page faults for which we can't determine the read/write direction
on AMD platforms.

E.g. "access to an unmapped address" (which will have PR=0, and thus we
won't know if it was a read or a write access) is just as much of a page
fault as "write to a read-only page" (which will have PR=1, and thus the
RW bit will be accurate) is, and for RAS purposes, both events are
equally interesting, and important to know about.

It's true that we currently don't have a way of signaling to
report_iommu_fault() (and by extension, to the io_page_fault
tracepoint) that we're not sure whether the offending access was a read
or a write, but I think we can just add a bit to include/linux/iommu.h
to indicate that, something along the lines of:

	 /* iommu fault flags */
	 #define IOMMU_FAULT_READ        0x0
	 #define IOMMU_FAULT_WRITE       0x1
	+#define IOMMU_FAULT_RW_UNKNOWN  0x2

(Cc'ing Ohad Ben-Cohen, who originally added this API.)

I don't think that it would be a good idea to just not signal the page
faults for which we don't know the read/write direction.


Thanks,
Lennert



> > - 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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu