netdev - RE: [for-next 4/6] net/mlx5: FPGA, Add basic support for Innova

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 25 May 2017 05:20:04 +0000
From:   Ilan Tayari <ilant@...lanox.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Saeed Mahameed <saeedm@...lanox.com>
CC:     "David S. Miller" <davem@...emloft.net>,
        Doug Ledford <dledford@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "jsorensen@...com" <jsorensen@...com>
Subject: RE: [for-next 4/6] net/mlx5: FPGA, Add basic support for Innova

> -----Original Message-----
> From: Alexei Starovoitov [mailto:alexei.starovoitov@...il.com]
> 
> On Tue, May 23, 2017 at 02:44:02PM +0300, Saeed Mahameed wrote:
> > From: Ilan Tayari <ilant@...lanox.com>
> >
> > Mellanox Innova is a NIC with ConnectX and an FPGA on the same
> > board. The FPGA is a bump-on-the-wire and thus affects operation of
> > the mlx5_core driver on the ConnectX ASIC.
> >
> > Add basic support for Innova in mlx5_core.
> >
> > This allows using the Innova card as a regular NIC, by detecting
> > the FPGA capability bit, and verifying its load state before
> > initializing ConnectX interfaces.
> 
> That doesn't sound like cx4/cx5 hw that mlx5 driver suppose to support.

Hi Alexei,
Thanks for your feedback.
Let me address your concerns.

I didn't state it, but the current iterations of Innova cards include the
ConnectX-4LX chip, which is exactly what mlx5 driver supports.

The PCI interface is *only* through the ConnectX chip with some new
Firmware commands. This patch doesn't register any new PCI device, 
so it does not make sense to create a separate driver.

> 
> > Also detect FPGA fatal runtime failures and enter error state if
> > they ever happen.
> >
> > All new FPGA-related logic is placed in its own subdirectory 'fpga',
> > which may be built by selecting CONFIG_MLX5_FPGA.
> > This prepares for further support of various Innova features in later
> > patchsets.
> > Additional details about hardware architecture will be provided as
> > more features get submitted.
> >
> > Signed-off-by: Ilan Tayari <ilant@...lanox.com>
> > Reviewed-by: Boris Pismenny <borisp@...lanox.com>
> > Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
> > ---
> >  MAINTAINERS                                        |  10 +
> >  drivers/net/ethernet/mellanox/mlx5/core/Kconfig    |  10 +
> >  drivers/net/ethernet/mellanox/mlx5/core/Makefile   |   3 +
> >  drivers/net/ethernet/mellanox/mlx5/core/eq.c       |  11 ++
> >  drivers/net/ethernet/mellanox/mlx5/core/fpga/cmd.c |  64 +++++++
> >  drivers/net/ethernet/mellanox/mlx5/core/fpga/cmd.h |  59 ++++++
> >  .../net/ethernet/mellanox/mlx5/core/fpga/core.c    | 202
> +++++++++++++++++++++
> >  .../net/ethernet/mellanox/mlx5/core/fpga/core.h    |  99 ++++++++++
> >  drivers/net/ethernet/mellanox/mlx5/core/main.c     |  19 +-
> >  include/linux/mlx5/device.h                        |   6 +
> >  include/linux/mlx5/driver.h                        |   5 +
> >  include/linux/mlx5/mlx5_ifc.h                      |  11 +-
> >  include/linux/mlx5/mlx5_ifc_fpga.h                 | 144
> +++++++++++++++
> >  13 files changed, 640 insertions(+), 3 deletions(-)
> 
> Can you put it into different driver? Dumping everything into by far
> the biggest nic driver already is already huge headache in terms on
> maintainability, debugging and back ports.
> Look at how intel splits their drivers.
> ixgb, ixgbe, ixgbevf are different drivers thought they have a lot in
> common. On one side it's a bit of copy paste, but on the other side
> it makes drivers much easier to develop and maintain independently.
> ConnectX-6 code and any future hw support doesn't belong to
> mlx5 driver at all.

If you build your driver without explicitly enabling CONFIG_MLX5_FPGA=y
(default is N) then you get none of this, and your driver is practically
the same as before.

The FPGA and CX operation is very tightly integrated.
If you don't want any of this, simply don't opt-in to CONFIG_MLX5_FPGA.

If you do want this, then splitting some of the logic to a
separate kernel object will not gain anything useful (logic would stay
the same), and just pollute the exported symbol table and open up the door
for issues of inter-module compatibility and so on.

Furthermore, the next patchset will introduce IPSec offload capabilities
Which are declared in netdev->hw_features. Those cannot be modified
after the netdevice is created, so all the extra logic has to be 
integrated into the mlx5 ethernet driver. This is another reason why
a separate driver is a bad idea.

We will include details about the board and how things work in the
cover letter of the IPSec offload patchset.

Ilan.