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

[Lennert Buytenhek <buytenh@wantstofly.org>]

On Sat, Aug 21, 2021 at 06:44:39PM +0300, Lennert Buytenhek wrote:

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

I had another look at this, and from some testing, it seems that,
contrary to what the datasheet suggests, the RW (read/write direction)
bit in logged I/O page faults is actually accurate for both faulting
accesses to present pages and faulting accesses to non-present pages.

I made a few hacks to the ixgbe driver to intentionally cause several
different types of I/O page faults, thereby turning an ixgbe NIC into
a poor man's I/O page fault generator, and these are the resulting
logged I/O page faults, on a Ryzen 3700X system:

Read from non-present page:
ixgbe 0000:25:00.1: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0010 address=0x1bbc8f040 flags=0x0000]
=> flags indicate PR(esent)=0, RW=0

Write to non-present page:
ixgbe 0000:25:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0010 address=0x1bdcb70c0 flags=0x0020]
=> flags indicate PR(esent)=0, RW=1

Read from write-only page:
ixgbe 0000:25:00.1: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0010 address=0xbbcc1c40 flags=0x0050]
=> flags indicate PR(esent)=1, RW=0  (and PE(rmission violation)=1)

Write to read-only page:
ixgbe 0000:25:00.0: AMD-Vi: Event logged [IO_PAGE_FAULT domain=0x0010 address=0xbdcb70c0 flags=0x0070]
=> flags indicate PR(esent)=1, RW=1  (and PE(rmission violation)=1)

In other words, it seems that the RW bit is reliable even for PR=0 type
faults.

I assume that the restriction mentioned in the docs regarding RW and
PR ("RW is only meaningful when PR=1, TR=0, and I=0" from AMD I/O
Virtualization Technology (IOMMU) Specification, revision 3.00, Table
55: IO_PAGE_FAULT Event Log Buffer Entry Fields) is either confused
or refers to a restriction in older hardware that has since been lifted.

I'll resubmit the patch to unconditionally pass through the RW bit.




ixgbe hacks:

to cause reads from non-present pages (in the TX path):

--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8232,7 +8232,7 @@ static int ixgbe_tx_map(struct ixgbe_ring *tx_ring,
 		dma_unmap_len_set(tx_buffer, len, size);
 		dma_unmap_addr_set(tx_buffer, dma, dma);
 
-		tx_desc->read.buffer_addr = cpu_to_le64(dma);
+		tx_desc->read.buffer_addr = cpu_to_le64(dma + BIT(32));
 
 		while (unlikely(size > IXGBE_MAX_DATA_PER_TXD)) {
 			tx_desc->read.cmd_type_len =


to cause writes to non-present pages (in the RX path):

--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1604,7 +1604,7 @@ void ixgbe_alloc_rx_buffers(struct ixgbe_ring *rx_ring, u16 cleaned_count)
 		 * Refresh the desc even if buffer_addrs didn't change
 		 * because each write-back erases this info.
 		 */
-		rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset);
+		rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset + BIT(32));
 
 		rx_desc++;
 		bi++;


to cause reads from write-only pages (in the TX path):

--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -8220,7 +8220,7 @@ static int ixgbe_tx_map(struct ixgbe_ring *tx_ring,
 	}
 
 #endif
-	dma = dma_map_single(tx_ring->dev, skb->data, size, DMA_TO_DEVICE);
+	dma = dma_map_single(tx_ring->dev, skb->data, size, DMA_FROM_DEVICE);
 
 	tx_buffer = first;
 

to cause writes to read-only pages (in the RX path):

--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1545,7 +1545,7 @@ static bool ixgbe_alloc_mapped_page(struct ixgbe_ring *rx_ring,
 	/* map page for use */
 	dma = dma_map_page_attrs(rx_ring->dev, page, 0,
 				 ixgbe_rx_pg_size(rx_ring),
-				 DMA_FROM_DEVICE,
+				 DMA_TO_DEVICE,
 				 IXGBE_RX_DMA_ATTR);
 
 	/*
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu