[dpdk-dev] [PATCH V2 3/5] Add Intel FPGA BUS Lib Code

[Gaëtan Rivet]

On Wed, Mar 21, 2018 at 03:41:56PM +0000, Bruce Richardson wrote:
> On Wed, Mar 21, 2018 at 03:31:31PM +0100, Gaëtan Rivet wrote:
> > On Wed, Mar 21, 2018 at 03:14:05PM +0100, Gaëtan Rivet wrote:
> > > Hi Bruce,
> > > 
> > > On Wed, Mar 21, 2018 at 01:35:09PM +0000, Bruce Richardson wrote:
> > > > On Wed, Mar 21, 2018 at 11:20:25AM +0100, Gaëtan Rivet wrote:
> > > > > Hi,
> > > > > 
> > > > > I have had issues compiling a few things here, have you checked
> > > > > build status before submitting?
> > > > > 
> > > > > On Wed, Mar 21, 2018 at 03:51:32PM +0800, Rosen Xu wrote:
> > > > > > Signed-off-by: Rosen Xu <rosen.xu at intel.com>
> > > > > > ---
> > > > <snip>
> > > > > +
> > > > > > +/*
> > > > > > + * Scan the content of the FPGA bus, and the devices in the devices
> > > > > > + * list
> > > > > > + */
> > > > > 
> > > > > So you seem to scan your bus by reading parameters
> > > > > given to the --ifpga EAL option.
> > > > > 
> > > > > Can you justify why you cannot use the PCI bus, have your FPGA be probed
> > > > > by a PCI driver, that would take those parameters as driver parameters,
> > > > > and spawn raw devices (one per bitstream) as needed as a result?
> > > > > 
> > > > > I see no reason this is not feasible. Unless you duly justify this
> > > > > approach, it seems unacceptable to me. You are subverting generic EAL
> > > > > code to bend things to your approach, without clear rationale.
> > > > > 
> > > > 
> > > > While I agree with the comments in other emails about avoiding
> > > > special-cases in the code that makes things not-scalable, I would take the
> > > > view that using a bus-type is the correct choice for this. While you could
> > > > have a single device that creates other devices, that is also true for all
> > > > other buses as well.  [Furthermore, I think it is incorrect assume that all
> > > > devices on the FPGA bus would be raw devices, it's entirely possible to
> > > > have cryptodevs, bbdevs or compress devs implemented in the AFUs].
> > > > 
> > > > Consider what a bus driver provides: it's a generic mechanism for scanning
> > > > for devices - which all use a common connection method - for DPDK use, and
> > > > mapping device drivers to those devices. For an FPGA device which presents
> > > > multiple AFUs, this seems to be exactly what is required - a device driver
> > > > to scan for devices and present them to DPDK. The FPGA bus driver will have
> > > > to check each AFU and match it against the set of registered AFU device
> > > > drivers to ensure that the crypto AFU gets the cryptodev driver, etc.
> > > > 
> > > > Logically, therefore, it is a bus - which just happens to be a sub-bus of
> > > > PCI, i.e. presented as a PCI device. Consider also that it may be possible
> > > > or even desirable, to use blacklisting and whitelisting for those AFU
> > > > devices so that some AFUs could be used by one app, while others by
> > > > another. If we just have a single PCI device, I think we'll find ourselves
> > > > duplicating a lot of bus-related functionality inside the driver in that
> > > > case.
> > > > 
> > > > Regards,
> > > > /Bruce
> > > 
> > > It makes sense indeed if you need to specialize several drivers specific
> > > to AFU mappings.
> > > 
> > > So, taking the bus approach, it seems we almost all agree that the
> > > current probe is not good enough.
> > > 
> > > I think you will run into issues if you registered your bus upon probing
> > > a PCI driver. Instead, would it be possible to:
> > > 
> > >   * not add the EAL option --ifpga.
> > >   * register the ifpga bus like any other.
> > >   * have a PCI driver that hotplug an IFPGA device, triggering a
> > >     scan and probe on the ifpga bus using the PCI device parameters.
> > >     Essentially you would find there the parameters that could be
> > >     given previously to the --ifpga option.
> > > 
> > > There should not be any EAL modification necessary.
> > > 
> > > The only downside I see would be that having both IFPGA device (for
> > > triggering bus ops) and afterward using AFU devices on this bus could be
> > > confusing. However, it seems safer and still cleaner overall.
> > 
> > Actually, just to build upon this remark, and thinking a little more
> > about it:
> > 
> > You would not need an additional "special" type of ifpga devices.
> > Upon attempting to hotplug an ifpga device, you would have a new
> > rte_devargs having the parameters within inserted, addressed to your
> > bus.
> > 
> > During scan, you would find this devargs, read the relevant parameters
> > and parse them (exactly like it is now implemented). You would then be
> > able to spawn as needed any number of rte_afu_device within your bus and
> > the scan would be complete.
> > 
> > The only trickery would be afterward, because you'd need at least one
> > device that would be detached and having the name given in the devargs.
> > So you would have to name your rte_afu_device after the devargs passed
> > as parameter to the plug, essentially having all of them using the same name.
> > 
> > Nothing prevents you from renaming your AFU devices after bus->plug, but
> > the EAL will rely on it for matching and knowing the scan went ok.
> > 
> > -- 
> Thanks, Gaëtan.
> 
> So, just to confirm I understand the suggestion correctly, you are
> proposing that we create and use the FPGA bus as normal, except that the
> scan will not report any devices. Then when an FPGA PCI device is
> discovered, we use the hotplug infrastructure to trigger a real scan of the
> FPGA for AFU devices, and do the whole driver mapping etc. process then.
> Is that the basic idea you suggest?

Yes.

-- 
Gaëtan Rivet
6WIND