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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
