On Mon, Apr 15, 2019 at 02:07:31PM -0700, Guenter Roeck wrote:
> On Mon, Apr 15, 2019 at 10:52:42PM +0200, Christoph Hellwig wrote:
> > On Mon, Apr 15, 2019 at 12:44:35PM -0700, Guenter Roeck wrote:
> > > This patch causes crashes with various boot tests. Most sparc tests
> > > crash, as
> > > well as several arm tests. Bisect results in both cases point to this
> > > patch.
> >
> > That just means we trigger an existing bug more easily now. I'll see
> > if I can help with the issues.
>
> Code which previously worked reliably no longer does. I would be quite
> hesitant to call this "trigger an existing bug more easily". "Regression"
> seems to be a more appropriate term - even more so as it seems to cause
> 'init' crashes, at least on arm.
Well, we have these sgls in the wild already, it just is that they
are fairly rare. For a related fix on a mainstream platform see
here for example:
https://lore.kernel.org/patchwork/patch/1050367/
Below is a rework of the sparc32 iommu code that should avoid your
reported problem. Please send any other reports to me as well.
diff --git a/arch/sparc/mm/iommu.c b/arch/sparc/mm/iommu.c
index e8d5d73ca40d..93c2fc440cb0 100644
--- a/arch/sparc/mm/iommu.c
+++ b/arch/sparc/mm/iommu.c
@@ -175,16 +175,38 @@ static void iommu_flush_iotlb(iopte_t *iopte, unsigned
int niopte)
}
}
-static u32 iommu_get_one(struct device *dev, struct page *page, int npages)
+static u32 __sbus_iommu_map_page(struct device *dev, struct page *page,
unsigned offset,
+ unsigned len, bool need_flush)
{
struct iommu_struct *iommu = dev->archdata.iommu;
+ phys_addr_t paddr = page_to_phys(page) + offset, p;
+ unsigned long pfn = __phys_to_pfn(paddr);
+ unsigned long off = (unsigned long)paddr & ~PAGE_MASK;
+ unsigned long npages = (off + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
int ioptex;
iopte_t *iopte, *iopte0;
unsigned int busa, busa0;
int i;
+ /* XXX So what is maxphys for us and how do drivers know it? */
+ if (!len || len > 256 * 1024)
+ return DMA_MAPPING_ERROR;
+
+ /*
+ * We expect unmapped highmem pages to be not in the cache.
+ * XXX Is this a good assumption?
+ * XXX What if someone else unmaps it here and races us?
+ */
+ if (need_flush && !PageHighMem(page)) {
+ for (p = paddr & PAGE_MASK; p < paddr + len; p += PAGE_SIZE) {
+ unsigned long vaddr = (unsigned long)phys_to_virt(p);
+
+ flush_page_for_dma(vaddr);
+ }
+ }
+
/* page color = pfn of page */
- ioptex = bit_map_string_get(&iommu->usemap, npages, page_to_pfn(page));
+ ioptex = bit_map_string_get(&iommu->usemap, npages, pfn);
if (ioptex < 0)
panic("iommu out");
busa0 = iommu->start + (ioptex << PAGE_SHIFT);
@@ -193,11 +215,11 @@ static u32 iommu_get_one(struct device *dev, struct page
*page, int npages)
busa = busa0;
iopte = iopte0;
for (i = 0; i < npages; i++) {
- iopte_val(*iopte) = MKIOPTE(page_to_pfn(page), IOPERM);
+ iopte_val(*iopte) = MKIOPTE(pfn, IOPERM);
iommu_invalidate_page(iommu->regs, busa);
busa += PAGE_SIZE;
iopte++;
- page++;
+ pfn++;
}
iommu_flush_iotlb(iopte0, npages);
@@ -205,99 +227,62 @@ static u32 iommu_get_one(struct device *dev, struct page
*page, int npages)
return busa0;
}
-static dma_addr_t __sbus_iommu_map_page(struct device *dev, struct page *page,
- unsigned long offset, size_t len)
-{
- void *vaddr = page_address(page) + offset;
- unsigned long off = (unsigned long)vaddr & ~PAGE_MASK;
- unsigned long npages = (off + len + PAGE_SIZE - 1) >> PAGE_SHIFT;
-
- /* XXX So what is maxphys for us and how do drivers know it? */
- if (!len || len > 256 * 1024)
- return DMA_MAPPING_ERROR;
- return iommu_get_one(dev, virt_to_page(vaddr), npages) + off;
-}
-
static dma_addr_t sbus_iommu_map_page_gflush(struct device *dev,
struct page *page, unsigned long offset, size_t len,
enum dma_data_direction dir, unsigned long attrs)
{
flush_page_for_dma(0);
- return __sbus_iommu_map_page(dev, page, offset, len);
+ return __sbus_iommu_map_page(dev, page, offset, len, false);
}
static dma_addr_t sbus_iommu_map_page_pflush(struct device *dev,
struct page *page, unsigned long offset, size_t len,
enum dma_data_direction dir, unsigned long attrs)
{
- void *vaddr = page_address(page) + offset;
- unsigned long p = ((unsigned long)vaddr) & PAGE_MASK;
-
- while (p < (unsigned long)vaddr + len) {
- flush_page_for_dma(p);
- p += PAGE_SIZE;
- }
-
- return __sbus_iommu_map_page(dev, page, offset, len);
+ return __sbus_iommu_map_page(dev, page, offset, len, true);
}
-static int sbus_iommu_map_sg_gflush(struct device *dev, struct scatterlist
*sgl,
- int nents, enum dma_data_direction dir, unsigned long attrs)
+static int __sbus_iommu_map_sg(struct device *dev, struct scatterlist *sgl,
+ int nents, enum dma_data_direction dir, unsigned long attrs,
+ bool need_flush)
{
struct scatterlist *sg;
- int i, n;
-
- flush_page_for_dma(0);
+ int i;
for_each_sg(sgl, sg, nents, i) {
- n = (sg->length + sg->offset + PAGE_SIZE-1) >> PAGE_SHIFT;
- sg->dma_address = iommu_get_one(dev, sg_page(sg), n) +
sg->offset;
+ sg->dma_address = __sbus_iommu_map_page(dev, sg_page(sg),
+ sg->offset, sg->length, need_flush);
+ if (sg->dma_address == DMA_MAPPING_ERROR)
+ return 0;
sg->dma_length = sg->length;
}
return nents;
}
-static int sbus_iommu_map_sg_pflush(struct device *dev, struct scatterlist
*sgl,
+static int sbus_iommu_map_sg_gflush(struct device *dev, struct scatterlist
*sgl,
int nents, enum dma_data_direction dir, unsigned long attrs)
{
- unsigned long page, oldpage = 0;
- struct scatterlist *sg;
- int i, j, n;
-
- for_each_sg(sgl, sg, nents, j) {
- n = (sg->length + sg->offset + PAGE_SIZE-1) >> PAGE_SHIFT;
-
- /*
- * We expect unmapped highmem pages to be not in the cache.
- * XXX Is this a good assumption?
- * XXX What if someone else unmaps it here and races us?
- */
- if ((page = (unsigned long) page_address(sg_page(sg))) != 0) {
- for (i = 0; i < n; i++) {
- if (page != oldpage) { /* Already flushed? */
- flush_page_for_dma(page);
- oldpage = page;
- }
- page += PAGE_SIZE;
- }
- }
-
- sg->dma_address = iommu_get_one(dev, sg_page(sg), n) +
sg->offset;
- sg->dma_length = sg->length;
- }
+ flush_page_for_dma(0);
+ return __sbus_iommu_map_sg(dev, sgl, nents, dir, attrs, false);
+}
- return nents;
+static int sbus_iommu_map_sg_pflush(struct device *dev, struct scatterlist
*sgl,
+ int nents, enum dma_data_direction dir, unsigned long attrs)
+{
+ return __sbus_iommu_map_sg(dev, sgl, nents, dir, attrs, true);
}
-static void iommu_release_one(struct device *dev, u32 busa, int npages)
+static void __sbus_iommu_unmap_page(struct device *dev, dma_addr_t dma_addr,
+ size_t len)
{
struct iommu_struct *iommu = dev->archdata.iommu;
- int ioptex;
- int i;
+ unsigned busa, npages, ioptex, i;
+ busa = dma_addr & PAGE_MASK;
BUG_ON(busa < iommu->start);
ioptex = (busa - iommu->start) >> PAGE_SHIFT;
+ npages = ((dma_addr & ~PAGE_MASK) + len + PAGE_SIZE-1) >> PAGE_SHIFT;
for (i = 0; i < npages; i++) {
iopte_val(iommu->page_table[ioptex + i]) = 0;
iommu_invalidate_page(iommu->regs, busa);
@@ -309,22 +294,17 @@ static void iommu_release_one(struct device *dev, u32
busa, int npages)
static void sbus_iommu_unmap_page(struct device *dev, dma_addr_t dma_addr,
size_t len, enum dma_data_direction dir, unsigned long attrs)
{
- unsigned long off = dma_addr & ~PAGE_MASK;
- int npages;
-
- npages = (off + len + PAGE_SIZE-1) >> PAGE_SHIFT;
- iommu_release_one(dev, dma_addr & PAGE_MASK, npages);
+ __sbus_iommu_unmap_page(dev, dma_addr, len);
}
static void sbus_iommu_unmap_sg(struct device *dev, struct scatterlist *sgl,
int nents, enum dma_data_direction dir, unsigned long attrs)
{
struct scatterlist *sg;
- int i, n;
+ int i;
for_each_sg(sgl, sg, nents, i) {
- n = (sg->length + sg->offset + PAGE_SIZE-1) >> PAGE_SHIFT;
- iommu_release_one(dev, sg->dma_address & PAGE_MASK, n);
+ __sbus_iommu_unmap_page(dev, sg->dma_address, sg->length);
sg->dma_address = 0x21212121;
}
}
