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@xxxxxxx> 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
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
