On Thu, Nov 05, 2020 at 06:08:16PM +0100, Christoph Hellwig wrote:
> On Thu, Nov 05, 2020 at 10:34:18AM -0400, Jason Gunthorpe wrote:
> > The check is removed here, but I didn't see a matching check added to
> > the IB side? Something like:
> >
> > static int rdma_rw_map_sg(struct ib_device *dev, struct scatterlist *sg,
> > u32 sg_cnt, enum dma_data_direction dir)
> > {
> > if (is_pci_p2pdma_page(sg_page(sg))) {
> > if (ib_uses_virt_dma(dev))
> > return 0;
> > return pci_p2pdma_map_sg(dev->dma_device, sg, sg_cnt, dir);
> > }
> > return ib_dma_map_sg(dev, sg, sg_cnt, dir);
> > }
>
> We should never get a P2P page into the rdma_rw_map_sg or other ib_dma*
> routines for the software drivers, as their struct devices don't connect
> to a PCІ device underneath, and thus no valid P2P distance can be
> retourned.
But that depends on the calling driver doing this properly, and we
don't expose an API to get the PCI device of the struct ib_device
.. how does nvme even work here?
If we can't get here then why did you add the check to the unmap side?
Why did this code in p2pdma exist at all?
> That being said IFF we want to implement P2P for those we'd need
> somethign like the above check, except that we still need to cal
> ib_dma_map_sg, i.e.:
>
> static int rdma_rw_map_sg(struct ib_device *dev, struct scatterlist *sg,
> u32 sg_cnt, enum dma_data_direction dir)
> {
> if (is_pci_p2pdma_page(sg_page(sg)) && !ib_uses_virt_dma(dev))
> return pci_p2pdma_map_sg(dev->dma_device, sg, sg_cnt, dir);
> return ib_dma_map_sg(dev, sg, sg_cnt, dir);
> }
The SW drivers can't handle PCI pages at all, they are going to try to
memcpy them or something else not __iomem, so we really do need to
prevent P2P pages going into them.
Jason
