| Message ID | alpine.DEB.2.10.1701191033210.10192@sstabellini-ThinkPad-X260 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/19/2017 01:39 PM, Stefano Stabellini wrote: > In xen_swiotlb_map_page and xen_swiotlb_map_sg_attrs, if the original > page is not suitable, we swap it for another page from the swiotlb > pool. > > In these cases, we don't update the previously calculated dma address > for the page before calling xen_dma_map_page. Thus, we end up calling > xen_dma_map_page passing the wrong dev_addr, resulting in > xen_dma_map_page mistakenly assuming that the page is foreign when it is > local. > > Fix the bug by updating dev_addr appropriately. > > This change has no effect on x86, because xen_dma_map_page is a stub > there. > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > Tested-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index f905d6e..f8afc6d 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -414,9 +414,9 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, > if (map == SWIOTLB_MAP_ERROR) > return DMA_ERROR_CODE; > > + dev_addr = xen_phys_to_bus(map); > xen_dma_map_page(dev, pfn_to_page(map >> PAGE_SHIFT), > dev_addr, map & ~PAGE_MASK, size, dir, attrs); > - dev_addr = xen_phys_to_bus(map); > > /* > * Ensure that the address returned is DMA'ble > @@ -575,13 +575,14 @@ void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr, Interesting that git diff picked xen_swiotlb_unmap_page() here instead of xen_swiotlb_map_sg_attrs() -boris > sg_dma_len(sgl) = 0; > return 0; > } > + dev_addr = xen_phys_to_bus(map); > xen_dma_map_page(hwdev, pfn_to_page(map >> PAGE_SHIFT), > dev_addr, > map & ~PAGE_MASK, > sg->length, > dir, > attrs); > - sg->dma_address = xen_phys_to_bus(map); > + sg->dma_address = dev_addr; > } else { > /* we are not interested in the dma_addr returned by > * xen_dma_map_page, only in the potential cache flushes executed
On Thu, Jan 19, 2017 at 06:58:46PM -0500, Boris Ostrovsky wrote: > On 01/19/2017 01:39 PM, Stefano Stabellini wrote: > > In xen_swiotlb_map_page and xen_swiotlb_map_sg_attrs, if the original > > page is not suitable, we swap it for another page from the swiotlb > > pool. > > > > In these cases, we don't update the previously calculated dma address > > for the page before calling xen_dma_map_page. Thus, we end up calling > > xen_dma_map_page passing the wrong dev_addr, resulting in > > xen_dma_map_page mistakenly assuming that the page is foreign when it is > > local. > > > > Fix the bug by updating dev_addr appropriately. > > > > This change has no effect on x86, because xen_dma_map_page is a stub > > there. > > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > > Tested-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> I can carry it via my swiotlb.git tree or if there are some extra things on the Xen tree - it can go through that?
On Thu, 19 Jan 2017, Konrad Rzeszutek Wilk wrote: > On Thu, Jan 19, 2017 at 06:58:46PM -0500, Boris Ostrovsky wrote: > > On 01/19/2017 01:39 PM, Stefano Stabellini wrote: > > > In xen_swiotlb_map_page and xen_swiotlb_map_sg_attrs, if the original > > > page is not suitable, we swap it for another page from the swiotlb > > > pool. > > > > > > In these cases, we don't update the previously calculated dma address > > > for the page before calling xen_dma_map_page. Thus, we end up calling > > > xen_dma_map_page passing the wrong dev_addr, resulting in > > > xen_dma_map_page mistakenly assuming that the page is foreign when it is > > > local. > > > > > > Fix the bug by updating dev_addr appropriately. > > > > > > This change has no effect on x86, because xen_dma_map_page is a stub > > > there. > > > > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > > Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > > > Tested-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > I can carry it via my swiotlb.git tree or if there are some extra > things on the Xen tree - it can go through that? It might be best to go via swiotlb.git: I don't think we have anything on the Xen tree at the moment -- Juergen has just sent a pull request for it.
On Thu, Jan 19, 2017 at 04:29:19PM -0800, Stefano Stabellini wrote: > On Thu, 19 Jan 2017, Konrad Rzeszutek Wilk wrote: > > On Thu, Jan 19, 2017 at 06:58:46PM -0500, Boris Ostrovsky wrote: > > > On 01/19/2017 01:39 PM, Stefano Stabellini wrote: > > > > In xen_swiotlb_map_page and xen_swiotlb_map_sg_attrs, if the original > > > > page is not suitable, we swap it for another page from the swiotlb > > > > pool. > > > > > > > > In these cases, we don't update the previously calculated dma address > > > > for the page before calling xen_dma_map_page. Thus, we end up calling > > > > xen_dma_map_page passing the wrong dev_addr, resulting in > > > > xen_dma_map_page mistakenly assuming that the page is foreign when it is > > > > local. > > > > > > > > Fix the bug by updating dev_addr appropriately. > > > > > > > > This change has no effect on x86, because xen_dma_map_page is a stub > > > > there. > > > > > > > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> > > > > Signed-off-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > > > > Tested-by: Pooya Keshavarzi <Pooya.Keshavarzi@de.bosch.com> > > > > > > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > > > > Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > > > I can carry it via my swiotlb.git tree or if there are some extra > > things on the Xen tree - it can go through that? > > It might be best to go via swiotlb.git: I don't think we have anything > on the Xen tree at the moment -- Juergen has just sent a pull request > for it. OK, let me queue it up and run it through the tests. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index f905d6e..f8afc6d 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -414,9 +414,9 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, if (map == SWIOTLB_MAP_ERROR) return DMA_ERROR_CODE; + dev_addr = xen_phys_to_bus(map); xen_dma_map_page(dev, pfn_to_page(map >> PAGE_SHIFT), dev_addr, map & ~PAGE_MASK, size, dir, attrs); - dev_addr = xen_phys_to_bus(map); /* * Ensure that the address returned is DMA'ble @@ -575,13 +575,14 @@ void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr, sg_dma_len(sgl) = 0; return 0; } + dev_addr = xen_phys_to_bus(map); xen_dma_map_page(hwdev, pfn_to_page(map >> PAGE_SHIFT), dev_addr, map & ~PAGE_MASK, sg->length, dir, attrs); - sg->dma_address = xen_phys_to_bus(map); + sg->dma_address = dev_addr; } else { /* we are not interested in the dma_addr returned by * xen_dma_map_page, only in the potential cache flushes executed