[PATCH v4] iommu/amd: Use report_iommu_fault()

[PATCH v4] iommu/amd: Use report_iommu_fault()

[Lennert Buytenhek]

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.

The latter point is the main aim of this patch, as it allows
rasdaemon-like daemons to be notified of I/O page faults, and to
possibly initiate corrective action in response.

A number of other IOMMU drivers already use report_iommu_fault(), and
I/O page faults on those IOMMUs therefore already trigger this
tracepoint -- but this isn't yet the case for AMD-Vi and Intel DMAR.

The AMD IOMMU specification suggests that the bit in an I/O page fault
event log entry that signals whether an I/O page fault was for a read
request or for a write request is only meaningful when the faulting
access was to a present page, but some testing on a Ryzen 3700X suggests
that this bit encodes the correct value even for I/O page faults to
non-present pages, and therefore, this patch passes the R/W information
up the stack even for I/O page faults to non-present pages.

Signed-off-by: Lennert Buytenhek <buytenh@arista.com>
---
Tested on v5.15-rc2 on a Ryzen 3700X, where it has the desired effect.

Changes for v4:
- Unconditionally pass through RW bit, after testing that suggests 
  that this bit is reliable even for I/O page faults to non-present
  pages.

Changes for v3:
- Test fault flags via macros.  (Suggested by Suravee Suthikulpanit.)

Changes for v2:
- Don't call report_iommu_fault() for IRQ remapping faults.
  (Suggested by Joerg Roedel.)

 drivers/iommu/amd/amd_iommu_types.h |  2 ++
 drivers/iommu/amd/iommu.c           | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
index 8dbe61e2b3c1..867535eb0ce9 100644
--- a/drivers/iommu/amd/amd_iommu_types.h
+++ b/drivers/iommu/amd/amd_iommu_types.h
@@ -138,6 +138,8 @@
 #define EVENT_DOMID_MASK_HI	0xf0000
 #define EVENT_FLAGS_MASK	0xfff
 #define EVENT_FLAGS_SHIFT	0x10
+#define EVENT_FLAG_RW		0x020
+#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 1722bb161841..7b592aba06f5 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -473,6 +473,12 @@ static void amd_iommu_report_rmp_fault(volatile u32 *event)
 		pci_dev_put(pdev);
 }
 
+#define IS_IOMMU_MEM_TRANSACTION(flags)		\
+	(((flags) & EVENT_FLAG_I) == 0)
+
+#define IS_WRITE_REQUEST(flags)			\
+	((flags) & EVENT_FLAG_RW)
+
 static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
 					u64 address, int flags)
 {
@@ -484,6 +490,18 @@ 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 && 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;
+	}
+
 	if (dev_data) {
 		if (__ratelimit(&dev_data->rs)) {
 			pci_err(pdev, "Event logged [IO_PAGE_FAULT domain=0x%04x address=0x%llx flags=0x%04x]\n",
@@ -495,6 +513,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
 			domain_id, address, flags);
 	}
 
+out:
 	if (pdev)
 		pci_dev_put(pdev);
 }
-- 
2.31.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Joerg Roedel]

On Sat, Sep 25, 2021 at 05:18:02PM +0300, Lennert Buytenhek wrote:
> +#define IS_IOMMU_MEM_TRANSACTION(flags)		\
> +	(((flags) & EVENT_FLAG_I) == 0)
> +
> +#define IS_WRITE_REQUEST(flags)			\
> +	((flags) & EVENT_FLAG_RW)
> +
>  static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
>  					u64 address, int flags)
>  {
> @@ -484,6 +490,18 @@ 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 && 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;

This is better placed inside the 'if (dev_data)' statement below.

Otherwise it looks good to me.


Regards,

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