Patchwork MT76x2U crashes XHCI driver on AMD Ryzen system

login
register
mail settings
Submitter Stanislaw Gruszka
Date Feb. 28, 2019, 10:42 a.m.
Message ID <20190228104223.GA2749@redhat.com>
Download mbox | patch
Permalink /patch/737821/
State New
Headers show

Comments

Stanislaw Gruszka - Feb. 28, 2019, 10:42 a.m.
On Thu, Feb 28, 2019 at 10:04:12AM +0100, Stanislaw Gruszka wrote:
> On Tue, Feb 26, 2019 at 12:24:08PM +0100, Stanislaw Gruszka wrote:
> > On Tue, Feb 26, 2019 at 11:44:13AM +0100, Joerg Roedel wrote:
> > > On Tue, Feb 26, 2019 at 11:34:51AM +0100, Stanislaw Gruszka wrote:
> > > > On Tue, Feb 26, 2019 at 11:05:36AM +0100, Joerg Roedel wrote:
> > > > If sg->offset > PAGE_SIZE is fine then most likely we have problem with
> > > > alignment.
> > > 
> > > The map_sg implementation in the AMD IOMMU driver uses sg_phys() which
> > > handles the sg->page + sg->offset calculation fine.
> > > 
> > > > Note hat issue is with dma_map_sg(), switching to dma_map_single()
> > > > by using urb->transfer_buffer instead of urb->sg make things work
> > > > on AMD IOMMU.
> > > 
> > > On the other hand this points to a bug in the driver, I'll look further
> > > if I can spot something there.
> > 
> > I think so too. And I have done some changes that avoid strange allocation
> > scheme and use usb synchronous messages instead of allocating buffers
> > with unaligned sizes. However things work ok on Intel IOMMU and 
> > there is no documentation what are dma_map_sg() requirement versus
> > dma_map_single() which works. I think there are some unwritten
> > requirements and things can work on some platforms and fails on others
> > (different IOMMUs, no-IOMMU on some ARCHes)  
> 
> For the record: we have another bug report with this issue:
> https://bugzilla.kernel.org/show_bug.cgi?id=202673
> 
> I provided there patch that change alignment for page_frag_alloc() and
> it did not fixed the problem. So this is not alignment issue.
> Now I think it could be page->refcount issue ...

I looked at the map_sg() in amd_iommu.c code and one line looks suspicious
to me, seems we can use not correctly initialized s->dma_address (should be 0,
but I think can be non-zero if SG was reused). The code also seems do
not do correct thing if there is more than one SG with multiple pages
on individual segments. Something like in below patch seems to be more
appropriate to me (not tested nor compiled).

Stanislaw
Stanislaw Gruszka - Feb. 28, 2019, 12:19 p.m.
On Thu, Feb 28, 2019 at 11:42:24AM +0100, Stanislaw Gruszka wrote:
> On Thu, Feb 28, 2019 at 10:04:12AM +0100, Stanislaw Gruszka wrote:
> > On Tue, Feb 26, 2019 at 12:24:08PM +0100, Stanislaw Gruszka wrote:
> > > On Tue, Feb 26, 2019 at 11:44:13AM +0100, Joerg Roedel wrote:
> > > > On Tue, Feb 26, 2019 at 11:34:51AM +0100, Stanislaw Gruszka wrote:
> > > > > On Tue, Feb 26, 2019 at 11:05:36AM +0100, Joerg Roedel wrote:
> > > > > If sg->offset > PAGE_SIZE is fine then most likely we have problem with
> > > > > alignment.
> > > > 
> > > > The map_sg implementation in the AMD IOMMU driver uses sg_phys() which
> > > > handles the sg->page + sg->offset calculation fine.
> > > > 
> > > > > Note hat issue is with dma_map_sg(), switching to dma_map_single()
> > > > > by using urb->transfer_buffer instead of urb->sg make things work
> > > > > on AMD IOMMU.
> > > > 
> > > > On the other hand this points to a bug in the driver, I'll look further
> > > > if I can spot something there.
> > > 
> > > I think so too. And I have done some changes that avoid strange allocation
> > > scheme and use usb synchronous messages instead of allocating buffers
> > > with unaligned sizes. However things work ok on Intel IOMMU and 
> > > there is no documentation what are dma_map_sg() requirement versus
> > > dma_map_single() which works. I think there are some unwritten
> > > requirements and things can work on some platforms and fails on others
> > > (different IOMMUs, no-IOMMU on some ARCHes)  
> > 
> > For the record: we have another bug report with this issue:
> > https://bugzilla.kernel.org/show_bug.cgi?id=202673
> > 
> > I provided there patch that change alignment for page_frag_alloc() and
> > it did not fixed the problem. So this is not alignment issue.
> > Now I think it could be page->refcount issue ...
> 
> I looked at the map_sg() in amd_iommu.c code and one line looks suspicious
> to me, seems we can use not correctly initialized s->dma_address (should be 0,
> but I think can be non-zero if SG was reused). The code also seems do
> not do correct thing if there is more than one SG with multiple pages
> on individual segments. Something like in below patch seems to be more
> appropriate to me (not tested nor compiled).

Nevermind, the patch is wrong, s->dma_address is initalized in sg_num_pages().

Stanislaw
Joerg Roedel - Feb. 28, 2019, 1:40 p.m.
On Thu, Feb 28, 2019 at 01:19:48PM +0100, Stanislaw Gruszka wrote:
> Nevermind, the patch is wrong, s->dma_address is initalized in sg_num_pages().

Yes, it is. In sg_num_pages() the offset into the IOMMU mapping is
stored in s->dma_address, taking also the segment boundary mask into
account. map_sg() later only adds the base-address to that.

Regards,

	Joerg
Stanislaw Gruszka - March 4, 2019, 7:10 a.m.
On Thu, Feb 28, 2019 at 02:40:29PM +0100, Joerg Roedel wrote:
> On Thu, Feb 28, 2019 at 01:19:48PM +0100, Stanislaw Gruszka wrote:
> > Nevermind, the patch is wrong, s->dma_address is initalized in sg_num_pages().
> 
> Yes, it is. In sg_num_pages() the offset into the IOMMU mapping is
> stored in s->dma_address, taking also the segment boundary mask into
> account. map_sg() later only adds the base-address to that.

I have some more info about the issues in
https://bugzilla.kernel.org/show_bug.cgi?id=202673 

We have some bugs in mt76. Apparently we should not use
page_frag_alloc() with size bigger than PAGE_SIZE as page_frag_alloc()
can fallback to single page allocation. And also we should not make
sizes unaligned as pointed in commit:
3bed3cc4156e ("net: Do not allocate page fragments that are not skb aligned"

However after fixing that mt76usb still did not work. To make things
work we had to change rx frag size from 2048 to PAGE_SIZE and change
virt_to_head_page() to virt_to_page() when setting SG's.

I think I understand why first change was needed. If we do 2 separate
dma maps of 2 different buffers in single page i.e (PAGE + off=0
and PAGE + off=2048) it causes problem. So either map_sg() return
error which mt76usb does not handle correctly or there is issue
in AMD IOMMU because two dma maps use the same page.

But I don't understand why the second change was needed. Without
it we have issue with incorrect page->_refcount . It is somehow
related with AMD IOMMU, because on different platforms we do not
have such problems.

Joerg, could you look at this ? Thanks. 

Stanislaw
Rosen Penev - March 4, 2019, 7:20 a.m.
On Sun, Mar 3, 2019 at 11:10 PM Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>
> On Thu, Feb 28, 2019 at 02:40:29PM +0100, Joerg Roedel wrote:
> > On Thu, Feb 28, 2019 at 01:19:48PM +0100, Stanislaw Gruszka wrote:
> > > Nevermind, the patch is wrong, s->dma_address is initalized in sg_num_pages().
> >
> > Yes, it is. In sg_num_pages() the offset into the IOMMU mapping is
> > stored in s->dma_address, taking also the segment boundary mask into
> > account. map_sg() later only adds the base-address to that.
>
> I have some more info about the issues in
> https://bugzilla.kernel.org/show_bug.cgi?id=202673
>
> We have some bugs in mt76. Apparently we should not use
> page_frag_alloc() with size bigger than PAGE_SIZE as page_frag_alloc()
> can fallback to single page allocation. And also we should not make
> sizes unaligned as pointed in commit:
> 3bed3cc4156e ("net: Do not allocate page fragments that are not skb aligned"
As a small and totally unrelated note, page_frag_alloc is only used in
mt76 and the nvme driver ;)
>
> However after fixing that mt76usb still did not work. To make things
> work we had to change rx frag size from 2048 to PAGE_SIZE and change
> virt_to_head_page() to virt_to_page() when setting SG's.
>
> I think I understand why first change was needed. If we do 2 separate
> dma maps of 2 different buffers in single page i.e (PAGE + off=0
> and PAGE + off=2048) it causes problem. So either map_sg() return
> error which mt76usb does not handle correctly or there is issue
> in AMD IOMMU because two dma maps use the same page.
>
> But I don't understand why the second change was needed. Without
> it we have issue with incorrect page->_refcount . It is somehow
> related with AMD IOMMU, because on different platforms we do not
> have such problems.
>
> Joerg, could you look at this ? Thanks.
>
> Stanislaw
Stanislaw Gruszka - March 11, 2019, 8:43 a.m.
On Sun, Mar 03, 2019 at 11:20:45PM -0800, Rosen Penev wrote:
> On Sun, Mar 3, 2019 at 11:10 PM Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> >
> > On Thu, Feb 28, 2019 at 02:40:29PM +0100, Joerg Roedel wrote:
> > > On Thu, Feb 28, 2019 at 01:19:48PM +0100, Stanislaw Gruszka wrote:
> > > > Nevermind, the patch is wrong, s->dma_address is initalized in sg_num_pages().
> > >
> > > Yes, it is. In sg_num_pages() the offset into the IOMMU mapping is
> > > stored in s->dma_address, taking also the segment boundary mask into
> > > account. map_sg() later only adds the base-address to that.
> >
> > I have some more info about the issues in
> > https://bugzilla.kernel.org/show_bug.cgi?id=202673
> >
> > We have some bugs in mt76. Apparently we should not use
> > page_frag_alloc() with size bigger than PAGE_SIZE as page_frag_alloc()
> > can fallback to single page allocation. And also we should not make
> > sizes unaligned as pointed in commit:
> > 3bed3cc4156e ("net: Do not allocate page fragments that are not skb aligned"
> As a small and totally unrelated note, page_frag_alloc is only used in
> mt76 and the nvme driver ;)

And there is nvme problem on AMD IOMMU:
https://bugzilla.kernel.org/show_bug.cgi?id=202665

While page_frag_alloc() should be used with cautious, at least care of
size alignment with ARCH_DMA_MINALIGN (not for IOMMU, but standard arch
dma), at this point I think some of those issues are AMD IOMMU problems.

> > However after fixing that mt76usb still did not work. To make things
> > work we had to change rx frag size from 2048 to PAGE_SIZE and change
> > virt_to_head_page() to virt_to_page() when setting SG's.
>
> > I think I understand why first change was needed. If we do 2 separate
> > dma maps of 2 different buffers in single page i.e (PAGE + off=0
> > and PAGE + off=2048) it causes problem. So either map_sg() return
> > error which mt76usb does not handle correctly or there is issue
> > in AMD IOMMU because two dma maps use the same page.

Any comment on that? Is fine or not to do 2 or more dma mappings
within the same single page on AMD IOMMU? If not, is there any
mechanism for drivers to find out about this limitation to prevent
to prepare wrong SG buffers?

> > But I don't understand why the second change was needed. Without
> > it we have issue with incorrect page->_refcount . It is somehow
> > related with AMD IOMMU, because on different platforms we do not
> > have such problems.

I think I found a bug in amd iommu code when setting sg->dma_address
with sg->offset > PAGE_SIZE. Will post fix shortly. 

Stanislaw
Stanislaw Gruszka - March 12, 2019, 7:13 a.m.
On Mon, Mar 11, 2019 at 09:43:19AM +0100, Stanislaw Gruszka wrote:
> > > However after fixing that mt76usb still did not work. To make things
> > > work we had to change rx frag size from 2048 to PAGE_SIZE and change
> > > virt_to_head_page() to virt_to_page() when setting SG's.
> >
> > > I think I understand why first change was needed. If we do 2 separate
> > > dma maps of 2 different buffers in single page i.e (PAGE + off=0
> > > and PAGE + off=2048) it causes problem. So either map_sg() return
> > > error which mt76usb does not handle correctly or there is issue
> > > in AMD IOMMU because two dma maps use the same page.
> 
> Any comment on that? Is fine or not to do 2 or more dma mappings
> within the same single page on AMD IOMMU? If not, is there any
> mechanism for drivers to find out about this limitation to prevent
> to prepare wrong SG buffers?

FTR: it was confirmed by Jan (bug reporter) the 2 or more dma mappings
within single page works with AMD IOMMU. Most likely it was needed
previously to workaround this sg->offset problem until proper fix to
AMD IOMMU was applied.

Stanislaw

Patch

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 34c9aa76a7bd..9c8887250b82 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2517,6 +2517,7 @@  static int map_sg(struct device *dev, struct scatterlist *sglist,
 	prot = dir2prot(direction);
 
 	/* Map all sg entries */
+	npages = 0;
 	for_each_sg(sglist, s, nelems, i) {
 		int j, pages = iommu_num_pages(sg_phys(s), s->length, PAGE_SIZE);
 
@@ -2524,7 +2525,7 @@  static int map_sg(struct device *dev, struct scatterlist *sglist,
 			unsigned long bus_addr, phys_addr;
 			int ret;
 
-			bus_addr  = address + s->dma_address + (j << PAGE_SHIFT);
+			bus_addr  = address + ((npages + j) << PAGE_SHIFT);
 			phys_addr = (sg_phys(s) & PAGE_MASK) + (j << PAGE_SHIFT);
 			ret = iommu_map_page(domain, bus_addr, phys_addr, PAGE_SIZE, prot, GFP_ATOMIC);
 			if (ret)
@@ -2532,6 +2533,8 @@  static int map_sg(struct device *dev, struct scatterlist *sglist,
 
 			mapped_pages += 1;
 		}
+
+		npages += mapped_pages;
 	}
 
 	/* Everything is mapped - write the right values into s->dma_address */