Re: [PATCH v6 7/7] dma-iommu: account for min_align_mask

[David Stevens <stevensd@chromium.org>]

  On Thu, Aug 19, 2021 at 6:03 PM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-17 02:38, David Stevens wrote:
> > From: David Stevens <stevensd@chromium.org>
> >
> > For devices which set min_align_mask, swiotlb preserves the offset of
> > the original physical address within that mask. Since __iommu_dma_map
> > accounts for non-aligned addresses, passing a non-aligned swiotlb
> > address with the swiotlb aligned size results in the offset being
> > accounted for twice in the size passed to iommu_map_atomic. The extra
> > page exposed to DMA is also not cleaned up by __iommu_dma_unmap, since
> > that function unmaps with the correct size. This causes mapping failures
> > if the iova gets reused, due to collisions in the iommu page tables.
> >
> > To fix this, pass the original size to __iommu_dma_map, since that
> > function already handles alignment.
> >
> > Additionally, when swiotlb returns non-aligned addresses, there is
> > padding at the start of the bounce buffer that needs to be cleared.
> >
> > Fixes: 1f221a0d0dbf ("swiotlb: respect min_align_mask")
> > Signed-off-by: David Stevens <stevensd@chromium.org>
> > ---
> >   drivers/iommu/dma-iommu.c | 24 +++++++++++++-----------
> >   1 file changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 6738420fc081..f2fb360c2907 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -788,7 +788,6 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> >       struct iommu_domain *domain = iommu_get_dma_domain(dev);
> >       struct iommu_dma_cookie *cookie = domain->iova_cookie;
> >       struct iova_domain *iovad = &cookie->iovad;
> > -     size_t aligned_size = size;
> >       dma_addr_t iova, dma_mask = dma_get_mask(dev);
> >
> >       /*
> > @@ -796,8 +795,8 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> >        * page aligned, we don't need to use a bounce page.
> >        */
> >       if (dev_use_swiotlb(dev) && iova_offset(iovad, phys | size)) {
> > -             void *padding_start;
> > -             size_t padding_size;
> > +             void *tlb_start;
> > +             size_t aligned_size, iova_off, mapping_end_off;
> >
> >               aligned_size = iova_align(iovad, size);
> >               phys = swiotlb_tbl_map_single(dev, phys, size, aligned_size,
> > @@ -806,23 +805,26 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
> >               if (phys == DMA_MAPPING_ERROR)
> >                       return DMA_MAPPING_ERROR;
> >
> > -             /* Cleanup the padding area. */
> > -             padding_start = phys_to_virt(phys);
> > -             padding_size = aligned_size;
> > +             iova_off = iova_offset(iovad, phys);
> > +             tlb_start = phys_to_virt(phys - iova_off);
> >
> >               if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
> >                   (dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL)) {
> > -                     padding_start += size;
> > -                     padding_size -= size;
> > +                     /* Cleanup the padding area. */
> > +                     mapping_end_off = iova_off + size;
> > +                     memset(tlb_start, 0, iova_off);
> > +                     memset(tlb_start + mapping_end_off, 0,
> > +                            aligned_size - mapping_end_off);
> > +             } else {
> > +                     /* Nothing was sync'ed, so clear the whole buffer. */
> > +                     memset(tlb_start, 0, aligned_size);
> >               }
> > -
> > -             memset(padding_start, 0, padding_size);
> >       }
> >
> >       if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> >               arch_sync_dma_for_device(phys, size, dir);
> >
> > -     iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
> > +     iova = __iommu_dma_map(dev, phys, size, prot, dma_mask);
>
> I still don't see how this preserves min_align_mask if it is larger than
> the IOVA granule

That's a slightly different issue, and not addressed in this series. I
guess the commit message should be 'dma-iommu: account for
min_align_mask w/swiotlb'. At least from my understanding of
min_align_mask, getting min_align_mask larger than the IOVA granule to
work would require changes to IOVA allocation, not anything to do
directly with swiotlb bounce buffers. Also, probably changes to
scatterlist coalescing. That being said, it looks like the only driver
that sets min_align_mask is the nvme driver, which sets it to 4096.

> (either way this change here does nothing since the
> first thing __iommu_dma_map() does is iova_align() the size right back
> anyway).
>

__iommu_dma_map() doesn't just align the size, it aligns
size+iova_off. Let's say you're doing a read of size 512 bytes at
offset 2048 within a page. In this case, aligned_size will be 4096.
Without min_align_mask, phys will be page aligned, so that's fine. But
with min_align_mask=4096, phys will also be at offset 2048. This
causes __iommu_dma_map to align 4096 + 2048, which becomes 8192. That
results in an extra page being mapped, which then doesn't get cleaned
up by __iommu_dma_unmap. That causes collisions in the IOMMU driver
the next time the iova is reused.

Passing size to __iommu_dma_map is sufficient. iommu_dma_map_page
needs to map [phys, phys+size), regardless of whether or not bounce
buffers are being used. __iommu_dma_map already takes care of cleaning
up the alignment, there's no need to do any extra alignment specific
to the bounce buffer case.

-David

> Robin.
>
> >       if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(phys))
> >               swiotlb_tbl_unmap_single(dev, phys, size, dir, attrs);
> >       return iova;
> >