On Fri, Jul 30, 2021 at 05:32:16AM +0300, Lennert Buytenhek wrote:
> > > This patch makes iommu/amd call report_iommu_fault() when an I/O page
> > > fault occurs, which has two effects:
> > >
> > > 1) It allows device drivers to register a callback to be notified of
> > > I/O page faults, via the iommu_set_fault_handler() API.
> > >
> > > 2) It triggers the io_page_fault tracepoint in report_iommu_fault()
> > > when an I/O page fault occurs.
> > >
> > > I'm mainly interested in (2). We have a daemon with some rasdaemon-like
> > > functionality for handling platform errors, and being able to be notified
> > > of I/O page faults for initiating corrective action is very useful -- and
> > > receiving such events via event tracing is a lot nicer than having to
> > > scrape them from kmsg.
> > >
> > > A number of other IOMMU drivers already use report_iommu_fault(), and
> > > I/O page faults on those IOMMUs therefore already seem to trigger this
> > > tracepoint -- but this isn't (yet) the case for AMD-Vi and Intel DMAR.
> > >
> > > I copied the logic from the other callers of report_iommu_fault(), where
> > > if that function returns zero, the driver will have handled the fault,
> > > in which case we avoid logging information about the fault to the printk
> > > buffer from the IOMMU driver.
> > >
> > > With this patch I see io_page_fault event tracing entries as expected:
> > >
> > > irq/24-AMD-Vi-48 [002] .... 978.554289: io_page_fault:
> > > IOMMU:[drvname] 0000:05:00.0 iova=0x0000000091482640 flags=0x0000
> > > irq/24-AMD-Vi-48 [002] .... 978.554294: io_page_fault:
> > > IOMMU:[drvname] 0000:05:00.0 iova=0x0000000091482650 flags=0x0000
> > > irq/24-AMD-Vi-48 [002] .... 978.554299: io_page_fault:
> > > IOMMU:[drvname] 0000:05:00.0 iova=0x0000000091482660 flags=0x0000
> > > irq/24-AMD-Vi-48 [002] .... 978.554305: io_page_fault:
> > > IOMMU:[drvname] 0000:05:00.0 iova=0x0000000091482670 flags=0x0000
> > > irq/24-AMD-Vi-48 [002] .... 978.554310: io_page_fault:
> > > IOMMU:[drvname] 0000:05:00.0 iova=0x0000000091482680 flags=0x0000
> > > irq/24-AMD-Vi-48 [002] .... 978.554315: io_page_fault:
> > > IOMMU:[drvname] 0000:05:00.0 iova=0x00000000914826a0 flags=0x0000
> > >
> > > For determining IOMMU_FAULT_{READ,WRITE}, I followed the AMD IOMMU
> > > spec, but I haven't tested that bit of the code, as the page faults I
> > > encounter are all to non-present (!EVENT_FLAG_PR) mappings, in which
> > > case EVENT_FLAG_RW doesn't make sense.
> > >
> > > Signed-off-by: Lennert Buytenhek <[email protected]>
> > > ---
> > > Changes since v1 RFC:
> > >
> > > - Don't call report_iommu_fault() for IRQ remapping faults.
> > > (Suggested by Joerg Roedel.)
> > >
> > > drivers/iommu/amd/amd_iommu_types.h | 4 ++++
> > > drivers/iommu/amd/iommu.c | 29 +++++++++++++++++++++++++++++
> > > 2 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/iommu/amd/amd_iommu_types.h
> > > b/drivers/iommu/amd/amd_iommu_types.h
> > > index 94c1a7a9876d..2f2c6630c24c 100644
> > > --- a/drivers/iommu/amd/amd_iommu_types.h
> > > +++ b/drivers/iommu/amd/amd_iommu_types.h
> > > @@ -138,6 +138,10 @@
> > > #define EVENT_DOMID_MASK_HI 0xf0000
> > > #define EVENT_FLAGS_MASK 0xfff
> > > #define EVENT_FLAGS_SHIFT 0x10
> > > +#define EVENT_FLAG_TR 0x100
> > > +#define EVENT_FLAG_RW 0x020
> > > +#define EVENT_FLAG_PR 0x010
> > > +#define EVENT_FLAG_I 0x008
> > > /* feature control bits */
> > > #define CONTROL_IOMMU_EN 0x00ULL
> > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> > > index a7d6d78147b7..d9fb2c22d44a 100644
> > > --- a/drivers/iommu/amd/iommu.c
> > > +++ b/drivers/iommu/amd/iommu.c
> >
> > What if we introduce:
> >
> > +/*
> > + * AMD I/O Virtualization Technology (IOMMU) Specification,
> > + * revision 3.00, section 2.5.3 ("IO_PAGE_FAULT Event") says
> > + * that the RW ("read-write") bit is only valid if the I/O
> > + * page fault was caused by a memory transaction request
> > + * referencing a page that was marked present.
> > + */
> > +#define IO_PAGE_FAULT_MEM_MASK \
> > + (EVENT_FLAG_TR | EVENT_FLAG_PR | EVENT_FLAG_I)
> > +#define IS_IOMMU_MEM_TRANSACTION(x) \
> > + ((x & IO_PAGE_FAULT_MEM_MASK) == EVENT_FLAG_PR)
> >
> > Note that this should have already checked w/ EVENT_FLAG_I == 0.
> >
> >
> > > @@ -484,6 +484,34 @@ static void amd_iommu_report_page_fault(u16 devid,
> > > u16 domain_id,
> > > if (pdev)
> > > dev_data = dev_iommu_priv_get(&pdev->dev);
> > > + /*
> > > + * If this is a DMA fault (for which the I(nterrupt) bit will
> > > + * be unset), allow report_iommu_fault() to prevent logging it.
> > > + */
> > > + if (dev_data && ((flags & EVENT_FLAG_I) == 0)) {
> > > + int report_flags;
> > > +
> > > + /*
> > > + * AMD I/O Virtualization Technology (IOMMU) Specification,
> > > + * revision 3.00, section 2.5.3 ("IO_PAGE_FAULT Event") says
> > > + * that the RW ("read-write") bit is only valid if the I/O
> > > + * page fault was caused by a memory transaction request
> > > + * referencing a page that was marked present.
> > > + */
> > > + report_flags = 0;
> > > + if ((flags & (EVENT_FLAG_TR | EVENT_FLAG_PR)) ==
> > > + EVENT_FLAG_PR) {
> > > + if (flags & EVENT_FLAG_RW)
> > > + report_flags |= IOMMU_FAULT_WRITE;
> > > + else
> > > + report_flags |= IOMMU_FAULT_READ;
> > > + }
> > > +
> > > + if (!report_iommu_fault(&dev_data->domain->domain,
> > > + &pdev->dev, address, report_flags))
> > > + goto out;
> > > + }
> > > +
> > > if (dev_data) {
> >
> > Here we do:
> >
> > + /*
> > + * Since report_iommu_fault() only report DMA-remapping
> > related fault,
> > + * convert AMD IO_PAGE_FAULT flags to IOMMU_FAULT_xx flags.
> > + */
> > + if (IS_IOMMU_MEM_TRANSACTION(flags) &&
> > + !report_iommu_fault(&dev_data->domain->domain,
> > &pdev->dev,
> > + address,
> > + (flags & EVENT_FLAG_RW) ?
> > + IOMMU_FAULT_WRITE :
> > IOMMU_FAULT_READ))
> > + goto out;
> > +
>
> 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
>
> - 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.
>
> 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))
> goto out;
> }
>
> ?
I sent out a v3 with this change.
Thanks,
Lennert
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
