dmar, dma_pool, etc

[Tycho Nightingale]

Hi,

> On Apr 29, 2019, at 4:24 PM, Tycho Nightingale <tychon at freebsd.org> wrote:
> 
> 
>> On Apr 29, 2019, at 2:34 PM, Niclas Zeising <zeising at freebsd.org> wrote:
>> 
>> On 2019-04-29 20:27, Niclas Zeising wrote:
>>> On 2019-04-29 18:00, Tycho Nightingale wrote:
>>>>> On Apr 29, 2019, at 11:06 AM, Niclas Zeising <zeising at FreeBSD.org> wrote:
>>>>> 
>>>>> As a side note, I can readily reproduce the hang on a spare laptop, please let me know if I can help in testing or diagnosing in any way.
>>>> 
>>>> 
>>>> If you can readily reproduce the hang, since there are 2 halves that comprised the fix (the DMA pool and non-pool mappings) it would be instructive to try reverting either dmapool.h or dma-mapping.h independently to see if that helps.
>>>> 
>>> Hi!
>>> I will test this and report back.  Thank you!
>>> Regards
>> 
>> Hi!
>> Just reverting dmapool.h or dma-mapping.h doesn't work, it won't build. I need to revert more than that.  Can you help me figure out what to revert in either case, or provide a patch?
> 
> Thanks for trying.  I managed to setup my HW and reproduce this and I see your point that it doesn’t cleave in half perfectly.
> 
> Since I’ve got a (non)-working test case, let me investigate a bit further.

I believe I’ve figured this out.  At least in my case I’ve been able to eliminate the hangs with the patch included at the bottom of this email.

The issue stems from the implementation of dma_map_sg().  According to the linux DMA documentation[1], entries of the scatter/gather list may be coalesced:

	int
	dma_map_sg(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction direction)

	Returns: the number of DMA address segments mapped (this may be shorter
	than <nents> passed in if some elements of the scatter/gather list are
	physically or virtually adjacent and an IOMMU maps them with a single
	entry).

My implementation of dma_map_sg() does just that.  As it turns out there are several consumers of dma_map_sg(), e.g. i915_gem_map_dma_buf() and i915_gem_gtt_prepare_pages(), and mock_map_dma_buf() among others that aren’t compliant with this documented API.  Going back to the non-coalesced version is likely the only path forward as bugs in the callee redefine this API de facto.

If this addresses the hangs you are seeing, I will post this on Phabricator.

Tycho

[1] https://www.kernel.org/doc/Documentation/DMA-API.txt

Index: sys/compat/linuxkpi/common/src/linux_pci.c
===================================================================
--- sys/compat/linuxkpi/common/src/linux_pci.c	(revision 346687)
+++ sys/compat/linuxkpi/common/src/linux_pci.c	(working copy)
@@ -565,10 +565,8 @@
 {
 	struct linux_dma_priv *priv;
 	struct linux_dma_obj *obj;
-	struct scatterlist *dma_sg, *sg;
-	int dma_nents, error, nseg;
-	size_t seg_len;
-	vm_paddr_t seg_phys, prev_phys_end;
+	struct scatterlist *sg;
+	int error, i, nseg;
 	bus_dma_segment_t seg;
 
 	priv = dev->dma_priv;
@@ -580,25 +578,11 @@
 		return (0);
 	}
 
-	sg = sgl;
-	dma_sg = sg;
-	dma_nents = 0;
-	while (nents > 0) {
-		seg_phys = sg_phys(sg);
-		seg_len = sg->length;
-		while (--nents > 0) {
-			prev_phys_end = sg_phys(sg) + sg->length;
-			sg = sg_next(sg);
-			if (prev_phys_end != sg_phys(sg))
-				break;
-			seg_len += sg->length;
-		}
-
+	for_each_sg(sgl, sg, nents, i) {
 		nseg = -1;
 		mtx_lock(&priv->dma_lock);
 		if (_bus_dmamap_load_phys(priv->dmat, obj->dmamap,
-		    seg_phys, seg_len, BUS_DMA_NOWAIT,
-		    &seg, &nseg) != 0) {
+		    sg_phys(sg), sg->length, 0, &seg, &nseg) != 0) {
 			bus_dmamap_unload(priv->dmat, obj->dmamap);
 			bus_dmamap_destroy(priv->dmat, obj->dmamap);
 			mtx_unlock(&priv->dma_lock);
@@ -607,14 +591,9 @@
 		}
 		mtx_unlock(&priv->dma_lock);
 		KASSERT(++nseg == 1, ("More than one segment (nseg=%d)", nseg));
+		sg_dma_address(sg) = seg.ds_addr;
+	}
 
-		sg_dma_address(dma_sg) = seg.ds_addr;
-		sg_dma_len(dma_sg) = seg.ds_len;
-
-		dma_sg = sg_next(dma_sg);
-		dma_nents++;
-        }
-
 	obj->dma_addr = sg_dma_address(sgl);
 
 	mtx_lock(&priv->ptree_lock);
@@ -629,7 +608,7 @@
 		return (0);
 	}
 
-	return (dma_nents);
+	return (nents);
 }
 
 void