[PATCH v11 07/11] device-mapping: Introduce DMA range map, supplanting dma_pfn_offset

[Nicolas Saenz Julienne]

On Mon, 2020-09-07 at 13:40 -0400, Jim Quinlan wrote:
> On Mon, Sep 7, 2020 at 11:01 AM Nicolas Saenz Julienne
> <nsaenzjulienne at suse.de> wrote:
> > > 
> > > Hi Nicolas,
> > > 
> > > Can you please help us out here?  It appears that your commit
> > 
> > It's dma_offset_from_dma_addr() that's causing trouble. It goes over all the
> > dma regions and, if no region matches the phys address range, it returns 0.
> > This isn't acceptable as is. In the past, we used to pass the offset
> > nonetheless, which would make the phys address grow out of the dma memory area
> > and fail the dma_capable() test.
> > 
> > For example, RPi4 devices attached to the old interconnect see phys [0x0
> > 0x3fffffff] at [0xc0000000 0xffffffff]. So say you're trying to convert
> > physical address 0xfa000000, you'll get 0 from dma_offset_from_phys_addr()
> > (since your dma range only covers the first GB) to then test if 0xfa000000 is
> > dma_capable(), which it is, but for the wrong reasons. Causing DMA issues
> > further down the line.
> > 
> > I don't have a proper suggestion on how to solve this but here's the solution I
> > used:
> > 
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index 4c4646761afe..40fe3c97f2be 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -217,11 +217,19 @@ static inline u64 dma_offset_from_phys_addr(struct device *dev,
> >  {
> >         const struct bus_dma_region *m = dev->dma_range_map;
> > 
> > -       if (m)
> > +       if (m) {
> >                 for (; m->size; m++)
> >                         if (paddr >= m->cpu_start &&
> >                             paddr - m->cpu_start < m->size)
> >                                 return m->offset;
> > +
> > +               /*
> > +                * The physical address doesn't fit any of the DMA regions,
> > +                * return an impossible to fulfill offset.
> > +                */
> > +               return -(1ULL << 46);
> > +       }
> > +
> >         return 0;
> >  }
> Hi Nicolas,
> 
> Thanks for looking into this.  The concern I have with your solution
> is that returning an arbitrarily large offset might overlap with an
> improbable but valid usage.  AFAIK there is nothing that disallows
> mapping a device to anywhere within the 64bit range of PCIe space,
> including up to say 0xffffffffffffffff.

Indeed, that's why I wasn't all that happy with my solution.

As an alternative, how about returning '-dev->bus_dma_limit' instead of 0? It's
always 0 for the devices without bus_dma_regions, and, I think, an always
unattainable offset for devices that do (I tested it on RPi4 with the 30bit
limitd mmc controller and it seems to work alright).

--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -222,7 +222,8 @@ static inline u64 dma_offset_from_phys_addr(struct device *dev,
                        if (paddr >= m->cpu_start &&
                            paddr - m->cpu_start < m->size)
                                return m->offset;
-       return 0;
+
+       return -dev->bus_dma_limit;
 }

> As an alternative perhaps changing dma_capable() so that if the
> dma_range_map is present then it validates that both ends of the
> prospective DMA region get "hits" on one of the offset regions in the
> map.  Christoph, if you are okay with this I can quickly post a patch.

IIUC, by the time you enter dma_capable() you already converted the physical
address to DMA address and the damage is done.

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 484 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200907/f406aac5/attachment.sig>