RE: [PATCH v2 1/3] net/enic: support SR-IOV VF using admin channel

["Hyong Youb Kim (hyonkim)" <hyonkim@cisco.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, August 8, 2024 5:50 PM
> To: Hyong Youb Kim (hyonkim) <hyonkim@cisco.com>
> Cc: dev@dpdk.org; John Daley (johndale) <johndale@cisco.com>
> Subject: Re: [PATCH v2 1/3] net/enic: support SR-IOV VF using admin channel
> 
> On 8/8/2024 7:14 AM, Hyong Youb Kim wrote:
> > The Linux VIC PF driver now requires the use of the admin channel
> > between PF and VF drivers. Certain devcmds are disabled for VF. The VF
> > driver is supposed to send control messages through the admin channel
> > to the PF driver to perform those devcmds. This commit adds the admin
> > channel to the VF driver (net/enic).
> >
> > The VF's admin channel consists of normal Tx/Rx queues. VIC firmware
> > hardwires those queues to PF. Control messages are specially crafted
> > but otherwise normal packets.
> >
> > The Rx queue uses LSC interrupt (interrupt vector 0) to notify the
> > driver of new Rx control messages. The PF driver may send unsolicited
> > request messages (e.g. asking for VF stats) to VF. Such messages cause
> > LSC interrupts and are processed on the global interrupt thread.
> >
> > For devcmds that must be sent through the admin channel, use wrapper
> > functions. They check if the device is a VF. If VF, use the admin
> > channel. Otherwise, perform devcmd directly.
> >
> > Two complications:
> > - Soft Rx stats
> > VF on old VIC models does not have HW Rx counters. In this case, the
> > VF driver counts packets/bytes and reports them as device stats.
> >
> > - Backward compatibility mode
> > Old VIC PF drivers on some operating systems may support only
> > VF_CAPABILITY_REQUEST message or not support the admin channel at
> > all. When the VF driver detects such PF driver, it reverts to the
> > compatibility mode and does not use the admin channel. In this mode,
> > trust mode (e.g. enabling promiscuous mode) does not work.
> >
> 
> Do you think does it worth to document above restrictions in the driver
> guide?
> 

I updated doc and release notes. Thanks for the reminder.

> > Signed-off-by: Hyong Youb Kim <hyonkim@cisco.com>
> > Reviewed-by: John Daley <johndale@cisco.com>
> > ---
> >  drivers/net/enic/base/vnic_cq.c       |  27 +
> >  drivers/net/enic/base/vnic_cq.h       |   3 +
> >  drivers/net/enic/base/vnic_dev.c      |  48 ++
> >  drivers/net/enic/base/vnic_dev.h      |   3 +
> >  drivers/net/enic/base/vnic_devcmd.h   |  49 ++
> >  drivers/net/enic/base/vnic_resource.h |  32 +-
> >  drivers/net/enic/base/vnic_rq.c       |  27 +
> >  drivers/net/enic/base/vnic_rq.h       |   7 +
> >  drivers/net/enic/base/vnic_wq.c       |  37 +-
> >  drivers/net/enic/base/vnic_wq.h       |   5 +
> >  drivers/net/enic/enic.h               |  28 +-
> >  drivers/net/enic/enic_ethdev.c        |   8 +-
> >  drivers/net/enic/enic_main.c          |  77 ++-
> >  drivers/net/enic/enic_res.c           |  12 +
> >  drivers/net/enic/enic_rxtx.c          |  20 +
> >  drivers/net/enic/enic_sriov.c         | 801 ++++++++++++++++++++++++++
> >  drivers/net/enic/enic_sriov.h         | 209 +++++++
> >
> 
> This new file is missing coopyright and license information.
> (Detected by './devtools/check-spdx-tag.sh' script)
> 

Added SPDX.

> <...>
> 
> > diff --git a/drivers/net/enic/enic_ethdev.c b/drivers/net/enic/enic_ethdev.c
> > index cad8db2f6f..5c967677fb 100644
> > --- a/drivers/net/enic/enic_ethdev.c
> > +++ b/drivers/net/enic/enic_ethdev.c
> > @@ -21,6 +21,7 @@
> >  #include "vnic_rq.h"
> >  #include "vnic_enet.h"
> >  #include "enic.h"
> > +#include "enic_sriov.h"
> >
> >  /*
> >   * The set of PCI devices this driver supports
> > @@ -28,7 +29,6 @@
> >  #define CISCO_PCI_VENDOR_ID 0x1137
> >  static const struct rte_pci_id pci_id_enic_map[] = {
> >  	{RTE_PCI_DEVICE(CISCO_PCI_VENDOR_ID,
> PCI_DEVICE_ID_CISCO_VIC_ENET)},
> > -	{RTE_PCI_DEVICE(CISCO_PCI_VENDOR_ID,
> PCI_DEVICE_ID_CISCO_VIC_ENET_VF)},
> >  	{RTE_PCI_DEVICE(CISCO_PCI_VENDOR_ID,
> PCI_DEVICE_ID_CISCO_VIC_ENET_SN)},
> >
> 
> Won't there be a specific PCIe device ID for the VF?
> Can there be some old devices around that requires this support?
> 
> What is the difference between PCI_DEVICE_ID_CISCO_VIC_ENET_VF &
> PCI_DEVICE_ID_CISCO_VIC_ENET_SN?
> Current code seems detecting device as VF when the PCIe device ID is
> PCI_DEVICE_ID_CISCO_VIC_ENET_SN.

The previous SR-IOV implementation was tied to Cisco VM-FEX. ENET_VF
is the device ID for that. That has been discontinued for some time. There 
is no need to support that anymore.

The current SR-IOV implementation is not tied to VM-FEX. ENET_SN is
the new device ID for this implementation. VF/SN naming is a bit
confusing. But SN is the name used internally..

Thanks.
-Hyong