Re: [PATCH rdma-next] RDMA: Explicitly pass in the dma_device to ib_register_device

From: Jason Gunthorpe <jgg@nvidia.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Leon Romanovsky <leon@kernel.org>,
	Doug Ledford <dledford@redhat.com>,
	Adit Ranadive <aditr@vmware.com>,
	Ariel Elior <aelior@marvell.com>,
	"Bernard Metzler" <bmt@zurich.ibm.com>,
	Christian Benvenuti <benve@cisco.com>,
	"Dennis Dalessandro" <dennis.dalessandro@intel.com>,
	Devesh Sharma <devesh.sharma@broadcom.com>,
	Faisal Latif <faisal.latif@intel.com>,
	"Gal Pressman" <galpress@amazon.com>,
	Lijun Ou <oulijun@huawei.com>, <linux-rdma@vger.kernel.org>,
	Michal Kalderon <mkalderon@marvell.com>,
	"Mike Marciniszyn" <mike.marciniszyn@intel.com>,
	Naresh Kumar PBS <nareshkumar.pbs@broadcom.com>,
	Nelson Escobar <neescoba@cisco.com>,
	"Parav Pandit" <parav@nvidia.com>,
	Parvi Kaustubhi <pkaustub@cisco.com>,
	"Potnuri Bharat Teja" <bharat@chelsio.com>,
	Selvin Xavier <selvin.xavier@broadcom.com>,
	Shiraz Saleem <shiraz.saleem@intel.com>,
	Somnath Kotur <somnath.kotur@broadcom.com>,
	Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>,
	VMware PV-Drivers <pv-drivers@vmware.com>,
	Weihang Li <liweihang@huawei.com>,
	"Wei Hu(Xavier)" <huwei87@hisilicon.com>,
	Yishai Hadas <yishaih@nvidia.com>,
	Zhu Yanjun <yanjunz@nvidia.com>
Subject: Re: [PATCH rdma-next] RDMA: Explicitly pass in the dma_device to ib_register_device
Date: Wed, 23 Sep 2020 15:34:09 -0300	[thread overview]
Message-ID: <20200923183409.GA9475@nvidia.com> (raw)
In-Reply-To: <20200923053840.GA4809@infradead.org>

On Wed, Sep 23, 2020 at 06:38:40AM +0100, Christoph Hellwig wrote:
> > +static void setup_dma_device(struct ib_device *device,
> > +			     struct device *dma_device)
> >  {
> > +	if (!dma_device) {
> >  		/*
> > +		 * If the caller does not provide a DMA capable device then the
> > +		 * IB device will be used. In this case the caller should fully
> > +		 * setup the ibdev for DMA. This usually means using
> > +		 * dma_virt_ops.
> >  		 */
> > +#ifdef CONFIG_DMA_OPS
> > +		if (WARN_ON(!device->dev.dma_ops))
> > +			return;
> > +#endif
> 
> dma ops are entirely optiona and NULL for the most common case
> (direct mapping without an IOMMU).

This is the case where:

 +		dma_device = &device->dev;

device == struct ib_device we just allocated

The only use of this configuration is to override the dma ops with
dma_virt_ops, so drivers that don't do that are buggy. A ib_device
itself cannot do DMA otherwise. This should probably be clarified to
just fail if !CONIFG_DMA_OPS

All other cases should point dma_device at some kind of DMA capable
struct device like a pci_device, which can have a NULL ops.

> > +	} else {
> > +		device->dev.dma_parms = dma_device->dma_parms;
> >  		/*
> > +		 * Auto setup the segment size if a DMA device was passed in.
> > +		 * The PCI core sets the maximum segment size to 64 KB. Increase
> > +		 * this parameter to 2 GB.
> >  		 */
> > +		dma_set_max_seg_size(dma_device, SZ_2G);
> 
> You can't just inherity DMA properties like this this.  Please
> fix all code that looks at the seg size to look at the DMA device.

Inherit? This is overriding the PCI default of 64K to be 2G for RDMA
devices.

The closest thing RDMA has to segment size is the length of a IB
scatter/gather WR element in verbs. This is 32 bits by spec.

Even if a SGL > 32 bits was required the ULP should switch to use RDMA
MRs instead of inline IB SG.

So really there is no segment size limitation and the intention here
is to just disable segment size at IOMMU layer.

Since this is universal, by spec, not HW specific, it doesn't make
much sense to put in the drivers.

> Btw, where does the magic 2G come from?

2G is the largest power of two that will fit in a struct
scatterlist->length or the ib_sge->length.

Jason