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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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?

> 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)

<...>

> 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.