Introduce Microsoft Azure Network Adatper (MANA) PMD

CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.

From: Long Li <[email protected]>

MANA is a PCI device. It uses IB verbs to access hardware through the kernel RDMA layer. This patch introduces build environment and basic device probe functions.

Signed-off-by Long Li <[email protected]>

---
Change log:
v2:
Fix typos.
Make the driver build only on x86-64 and Linux.
Remove unused header files.
Change port definition to uint16_t or uint8_t (for IB).
Use getline() in place of fgets() to read and truncate a line. v3:
Add meson build check for required functions from RDMA direct verb header file v4:
Remove extra "\n" in logging code.
Use "r" in place of "rb" in fopen() to read text files.

drivers/net/mana/mana.c new file +14 -0

new file mode 100644
index 0000000000..cb59eb6882
@@ -0,0 +1,704 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2022 Microsoft Corporation
+ */
+
+#include <unistd.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <sys/mman.h>
+
+#include <ethdev_driver.h>
+#include <ethdev_pci.h>
+#include <rte_kvargs.h>
+#include <rte_eal_paging.h>
+

drivers/net/mana/mana.c new file +9 -0

+/* Spinlock for mana_shared_data */
+static rte_spinlock_t mana_shared_data_lock = RTE_SPINLOCK_INITIALIZER;
+
+/* Allocate a buffer on the stack and fill it with a printf format string. */
+#define MKSTR(name, ...) \
+       int mkstr_size_##name = snprintf(NULL, 0, "" __VA_ARGS__); \
+       char name[mkstr_size_##name + 1]; \
+       \
+       memset(name, 0, mkstr_size_##name + 1); \

drivers/net/mana/mana.c new file +23 -0

+
+uint16_t
+mana_rx_burst_removed(void *dpdk_rxq __rte_unused,
+                     struct rte_mbuf **pkts __rte_unused,
+                     uint16_t pkts_n __rte_unused)
+{
+       rte_mb();
+       return 0;
+}
+
+uint16_t
+mana_tx_burst_removed(void *dpdk_rxq __rte_unused,
+                     struct rte_mbuf **pkts __rte_unused,
+                     uint16_t pkts_n __rte_unused)
+{
+       rte_mb();
+       return 0;
+}
+
+static const char *mana_init_args[] = {
+       "mac",
+       NULL,
+};

drivers/net/mana/mana.c new file +22 -0

+{
+       struct mana_conf *conf = (struct mana_conf *)private;
+       int ret;
+
+       DRV_LOG(INFO, "key=%s value=%s index=%d", key, val, conf->index);
+
+       if (conf->index >= MAX_NUM_ADDRESS) {
+               DRV_LOG(ERR, "Exceeding max MAC address");
+               return 1;
+       }
+
+       ret = rte_ether_unformat_addr(val, &conf->mac_array[conf->index]);
+       if (ret) {
+               DRV_LOG(ERR, "Invalid MAC address %s", val);
+               return ret;
+       }
+
+       conf->index++;
+
+       return 0;
+}
+

drivers/net/mana/mana.c new file +30 -0

+static int get_port_mac(struct ibv_device *device, unsigned int port,
+                       struct rte_ether_addr *addr)
+{
+       FILE *file;
+       int ret = 0;
+       DIR *dir;
+       struct dirent *dent;
+       unsigned int dev_port;
+       char mac[20];
+
+       MKSTR(path, "%s/device/net", device->ibdev_path);
+
+       dir = opendir(path);
+       if (!dir)
+               return -ENOENT;
+
+       while ((dent = readdir(dir))) {
+               char *name = dent->d_name;
+
+               MKSTR(filepath, "%s/%s/dev_port", path, name);
+
+               /* Ignore . and .. */
+               if ((name[0] == '.') &&
+                   ((name[1] == '\0') ||
+                    ((name[1] == '.') && (name[2] == '\0'))))
+                       continue;
+
+               file = fopen(filepath, "r");
+               if (!file)
+                       continue;

drivers/net/mana/mana.c new file +22 -0

+                             struct rte_pci_device *pci_dev,
+                             struct rte_ether_addr *mac_addr)
+{
+       struct ibv_device **ibv_list;
+       int ibv_idx;
+       struct ibv_context *ctx;
+       struct ibv_device_attr_ex dev_attr;
+       int num_devices;
+       int ret = 0;
+       uint8_t port;
+       struct mana_priv *priv = NULL;
+       struct rte_eth_dev *eth_dev = NULL;
+       bool found_port;
+
+       ibv_list = ibv_get_device_list(&num_devices);
+       for (ibv_idx = 0; ibv_idx < num_devices; ibv_idx++) {
+               struct ibv_device *ibdev = ibv_list[ibv_idx];
+               struct rte_pci_addr pci_addr;
+
+               DRV_LOG(INFO, "Probe device name %s dev_name %s ibdev_path %s",
+                       ibdev->name, ibdev->dev_name, ibdev->ibdev_path);
+

drivers/net/mana/mana.c new file +5 -0

+               ctx = ibv_open_device(ibdev);
+               if (!ctx) {
+                       DRV_LOG(ERR, "Failed to open IB device %s",
+                               ibdev->name);
+                       continue;

drivers/net/mana/mana.c new file +5 -0

+RTE_PMD_EXPORT_NAME(net_mana, __COUNTER__);
+RTE_PMD_REGISTER_PCI_TABLE(net_mana, mana_pci_id_map);
+RTE_PMD_REGISTER_KMOD_DEP(net_mana, "* ib_uverbs & mana_ib");
+RTE_LOG_REGISTER_SUFFIX(mana_logtype_init, init, NOTICE);
+RTE_LOG_REGISTER_SUFFIX(mana_logtype_driver, driver, NOTICE);

drivers/net/mana/mana.h new file +17 -0

new file mode 100644
index 0000000000..e30c030b4e
@@ -0,0 +1,210 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2022 Microsoft Corporation
+ */
+
+#ifndef __MANA_H__
+#define __MANA_H__
+
+enum {
+       PCI_VENDOR_ID_MICROSOFT = 0x1414,
+};
+
+enum {
+       PCI_DEVICE_ID_MICROSOFT_MANA = 0x00ba,
+};
+
+/* Shared data between primary/secondary processes */
+struct mana_shared_data {

Quoted 28 lines

+                             struct rte_pci_device *pci_dev, +                             struct rte_ether_addr *mac_addr)

drivers/net/mana/mana.c new file +18 -0

+{
+       struct ibv_device **ibv_list;
+       int ibv_idx;
+       struct ibv_context *ctx;
+       struct ibv_device_attr_ex dev_attr;
+       int num_devices;
+       int ret = 0;
+       uint8_t port;
+       struct mana_priv *priv = NULL;
+       struct rte_eth_dev *eth_dev = NULL;
+       bool found_port;
+
+       ibv_list = ibv_get_device_list(&num_devices);
+       for (ibv_idx = 0; ibv_idx < num_devices; ibv_idx++) {
+               struct ibv_device *ibdev = ibv_list[ibv_idx];
+               struct rte_pci_addr pci_addr;
+
+               DRV_LOG(INFO, "Probe device name %s dev_name %s 

ibdev_path %s",
+                       ibdev->name, ibdev->dev_name, ibdev->ibdev_path);

drivers/net/mana/mana.c new file +6 -0

+
+               if (mana_ibv_device_to_pci_addr(ibdev, &pci_addr))
+                       continue;
+
+               /* Ignore if this IB device is not this PCI device */
+               if (pci_dev->addr.domain != pci_addr.domain ||

Quoted 5 lines

+                   pci_dev->addr.bus != pci_addr.bus || +                   pci_dev->addr.devid != pci_addr.devid || +                   pci_dev->addr.function != pci_addr.function) +                       continue;
+

As far as I understand, intention of this loop is to find 'ibdev' matching this device, code gooes through all "ibv device list" for this, I wonder if there is a easy way for doing this, like a sysfs entry to help getting this information? And how mlx4/5 does this?

Subject: Re: [Patch v4 01/17] net/mana: add basic driver, build environment and doc

Quoted 31 lines · Ferruh Yigit

Quoted 31 lines

+                             struct rte_pci_device *pci_dev, +                             struct rte_ether_addr *mac_addr) { +       struct ibv_device **ibv_list;
+       int ibv_idx;
+       struct ibv_context *ctx;
+       struct ibv_device_attr_ex dev_attr;
+       int num_devices;
+       int ret = 0;
+       uint8_t port;
+       struct mana_priv *priv = NULL;
+       struct rte_eth_dev *eth_dev = NULL;
+       bool found_port;

drivers/net/mana/mana.c new file +7 -0

+
+       ibv_list = ibv_get_device_list(&num_devices);
+       for (ibv_idx = 0; ibv_idx < num_devices; ibv_idx++) {
+               struct ibv_device *ibdev = ibv_list[ibv_idx];
+               struct rte_pci_addr pci_addr;
+
+               DRV_LOG(INFO, "Probe device name %s dev_name %s

ibdev_path %s",
+                       ibdev->name, ibdev->dev_name,

drivers/net/mana/mana.c new file +10 -0

+ibdev->ibdev_path);
+
+               if (mana_ibv_device_to_pci_addr(ibdev, &pci_addr))
+                       continue;
+
+               /* Ignore if this IB device is not this PCI device */
+               if (pci_dev->addr.domain != pci_addr.domain ||
+                   pci_dev->addr.bus != pci_addr.bus ||
+                   pci_dev->addr.devid != pci_addr.devid ||
+                   pci_dev->addr.function != pci_addr.function)

Quoted 8 lines

Quoted 2 lines

+                       continue;
+

As far as I understand, intention of this loop is to find 'ibdev' matching this device, code gooes through all "ibv device list" for this, I wonder if there is a easy way for doing this, like a sysfs entry to help getting this information?
And how mlx4/5 does this?

Since there are multiple RDMA devices now, does it make sense to have RDMA bus driver, which can hide some PCIe details under bus, and driver can get PCI and ibdev information during probe?

Quoted 44 lines

Subject: Re: [Patch v4 01/17] net/mana: add basic driver, build environment and doc

Quoted 39 lines · Ferruh Yigit

Quoted 23 lines

drivers/net/mana/mana.c new file +19 -0

+                             struct rte_pci_device *pci_dev,
+                             struct rte_ether_addr *mac_addr) {
+       struct ibv_device **ibv_list;
+       int ibv_idx;
+       struct ibv_context *ctx;
+       struct ibv_device_attr_ex dev_attr;
+       int num_devices;
+       int ret = 0;
+       uint8_t port;
+       struct mana_priv *priv = NULL;
+       struct rte_eth_dev *eth_dev = NULL;
+       bool found_port;
+
+       ibv_list = ibv_get_device_list(&num_devices);
+       for (ibv_idx = 0; ibv_idx < num_devices; ibv_idx++) {
+               struct ibv_device *ibdev = ibv_list[ibv_idx];
+               struct rte_pci_addr pci_addr;
+
+               DRV_LOG(INFO, "Probe device name %s dev_name %s

ibdev_path %s",

drivers/net/mana/mana.c new file +3 -0

+                       ibdev->name, ibdev->dev_name,
+ibdev->ibdev_path);
+

drivers/net/mana/mana.c line 383

Quoted 10 lines

drivers/net/mana/mana.c new file +10 -0

+               if (mana_ibv_device_to_pci_addr(ibdev, &pci_addr))
+                       continue;
+
+               /* Ignore if this IB device is not this PCI device */
+               if (pci_dev->addr.domain != pci_addr.domain ||
+                   pci_dev->addr.bus != pci_addr.bus ||
+                   pci_dev->addr.devid != pci_addr.devid ||
+                   pci_dev->addr.function != pci_addr.function)
+                       continue;
+

As far as I understand, intention of this loop is to find 'ibdev' matching this device, code gooes through all "ibv device list" for this, I wonder if there is a easy way for doing this, like a sysfs entry to help getting this information?
And how mlx4/5 does this?

Quoted 3 lines

Since there are multiple RDMA devices now, does it make sense to have RDMA bus driver, which can hide some PCIe details under bus, and driver can get PCI and ibdev information during probe?

Mellanox drivers use a similar way to go through the list of IB devices.

Matan, Viacheslav, what are your thoughts on implementing a bus for RDMA devices?

22/08/2022 20:27, Long Li:

Quoted 48 lines

Quoted 47 lines

Subject: Re: [Patch v4 01/17] net/mana: add basic driver, build environment and doc

Quoted 31 lines · Ferruh Yigit

Quoted 31 lines

drivers/net/mana/mana.c new file +19 -0

+                             struct rte_pci_device *pci_dev,
+                             struct rte_ether_addr *mac_addr) {
+       struct ibv_device **ibv_list;
+       int ibv_idx;
+       struct ibv_context *ctx;
+       struct ibv_device_attr_ex dev_attr;
+       int num_devices;
+       int ret = 0;
+       uint8_t port;
+       struct mana_priv *priv = NULL;
+       struct rte_eth_dev *eth_dev = NULL;
+       bool found_port;
+
+       ibv_list = ibv_get_device_list(&num_devices);
+       for (ibv_idx = 0; ibv_idx < num_devices; ibv_idx++) {
+               struct ibv_device *ibdev = ibv_list[ibv_idx];
+               struct rte_pci_addr pci_addr;
+
+               DRV_LOG(INFO, "Probe device name %s dev_name %s

ibdev_path %s",

drivers/net/mana/mana.c new file +11 -0

+                       ibdev->name, ibdev->dev_name,
+ibdev->ibdev_path);
+
+               if (mana_ibv_device_to_pci_addr(ibdev, &pci_addr))
+                       continue;
+
+               /* Ignore if this IB device is not this PCI device */
+               if (pci_dev->addr.domain != pci_addr.domain ||
+                   pci_dev->addr.bus != pci_addr.bus ||
+                   pci_dev->addr.devid != pci_addr.devid ||
+                   pci_dev->addr.function != pci_addr.function)

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+                       continue;
+

As far as I understand, intention of this loop is to find 'ibdev' matching this device, code gooes through all "ibv device list" for this, I wonder if there is a easy way for doing this, like a sysfs entry to help getting this information?
And how mlx4/5 does this?

Since there are multiple RDMA devices now, does it make sense to have RDMA bus driver, which can hide some PCIe details under bus, and driver can get PCI and ibdev information during probe?

Quoted 3 lines

Mellanox drivers use a similar way to go through the list of IB devices.

Matan, Viacheslav, what are your thoughts on implementing a bus for RDMA devices?

These are PCI devices first.
Is it a good idea to have a bus driver on top of another one? What would be the responsibility of such a bus driver?

Quoted 22 lines

22/08/2022 20:27, Long Li:

Quoted 21 lines

Quoted 20 lines · Ferruh Yigit

Quoted 7 lines

Quoted 7 lines

drivers/net/mana/mana.c new file +6 -0

+       ibv_list = ibv_get_device_list(&num_devices);
+       for (ibv_idx = 0; ibv_idx < num_devices; ibv_idx++) {
+               struct ibv_device *ibdev = ibv_list[ibv_idx];
+               struct rte_pci_addr pci_addr;
+
+               DRV_LOG(INFO, "Probe device name %s dev_name %s

ibdev_path %s",

Quoted 8 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+                       ibdev->name, ibdev->dev_name,
+ibdev->ibdev_path);

As far as I understand, intention of this loop is to find 'ibdev' matching this device, code gooes through all "ibv device list" for this, I wonder if there is a easy way for doing this, like a sysfs entry to help getting this information?
And how mlx4/5 does this?

Since there are multiple RDMA devices now, does it make sense to have RDMA bus driver, which can hide some PCIe details under bus, and driver can get PCI and ibdev information during probe?

Quoted 8 lines

Quoted 3 lines

Mellanox drivers use a similar way to go through the list of IB devices.

Matan, Viacheslav, what are your thoughts on implementing a bus for RDMA devices?

These are PCI devices first.
Is it a good idea to have a bus driver on top of another one? What would be the responsibility of such a bus driver?

As far as I can see a 'ibdev' device struct is required to be able to configure the device, so rdma bus can be on top of PCI and find matching 'ibdev' device for it, and driver probe can get pci and ibdev information directly, instead of each rdma device driver walking through all ibdev devices (ibv_get_device_list()).

Subject: Re: [Patch v4 01/17] net/mana: add basic driver, build environment and doc

Quoted 1 line · On 7/9/2022 12:49 AM, [email protected] wrote:

CAUTION: This message has originated from an External Source. Please use

proper judgment and caution when opening attachments, clicking links, or responding to this email.

Quoted 24 lines

From: Long Li <[email protected]>

MANA is a PCI device. It uses IB verbs to access hardware through the kernel RDMA layer. This patch introduces build environment and basic device probe functions.

Signed-off-by Long Li <[email protected]>

---
Change log:
v2:
Fix typos.
Make the driver build only on x86-64 and Linux.
Remove unused header files.
Change port definition to uint16_t or uint8_t (for IB). Use getline() in place of fgets() to read and truncate a line. v3:
Add meson build check for required functions from RDMA direct verb header file
v4:
Remove extra "\n" in logging code.
Use "r" in place of "rb" in fopen() to read text files.

<...>

doc/guides/nics/mana.rst line 9

Quoted 12 lines

doc/guides/nics/mana.rst new file +9 -0

@@ -0,0 +1,66 @@
+..  SPDX-License-Identifier: BSD-3-Clause
+    Copyright 2022 Microsoft Corporation
+
+MANA poll mode driver library
+=============================
+
+The MANA poll mode driver library (**librte_net_mana**) implements
+support for Microsoft Azure Network Adapter VF in SR-IOV context.
+

Can you please provide any link to an official product description? As a reference point for anybody interested more with the product details.

<..>

doc/guides/nics/mana.rst

Quoted 2 lines

doc/guides/nics/mana.rst new file +2 -0

+
+Netvsc PMD arguments > +--------------------

'Netvsc'? Do you mean 'MANA'?

j

doc/guides/nics/mana.rst line 66

Quoted 9 lines

doc/guides/nics/mana.rst new file +9 -0

+
+The user can specify below argument in devargs.
+
+#.  ``mac``:
+
+    Specify the MAC address for this device. If it is set, the driver
+    probes and loads the NIC with a matching mac address. If it is not
+    set, the driver probes on all the NICs on the PCI device. The default
+    value is not set, meaning all the NICs will be probed and loaded.

Code accepts up to 8 mac value, should this be documented?

Also why this devarg is needed?

Quoted 26 lines

drivers/net/mana/mana.c new file +19 -0

new file mode 100644
index 0000000000..cb59eb6882
@@ -0,0 +1,704 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2022 Microsoft Corporation
+ */
+
+#include <unistd.h>
+#include <dirent.h>
+#include <fcntl.h>
+#include <sys/mman.h>
+
+#include <ethdev_driver.h>
+#include <ethdev_pci.h>
+#include <rte_kvargs.h>
+#include <rte_eal_paging.h>
+
+#include <infiniband/verbs.h>
+#include <infiniband/manadv.h>
+
+#include <assert.h>
+

drivers/net/mana/mana.c line 24

Quoted 5 lines

drivers/net/mana/mana.c new file +5 -0

+#include "mana.h"
+
+/* Shared memory between primary/secondary processes, per driver */
+struct mana_shared_data *mana_shared_data;
+const struct rte_memzone *mana_shared_mz;

If these global variables are not used by other compilation units, please try to make them static as much as possible.

drivers/net/mana/mana.c line 28

Quoted 5 lines

drivers/net/mana/mana.c new file +4 -0

+static const char *MZ_MANA_SHARED_DATA = "mana_shared_data";
+
+struct mana_shared_data mana_local_data;
+

Can you put some comment to this global variables?

drivers/net/mana/mana.c line 30

Quoted 3 lines

drivers/net/mana/mana.c new file +2 -0

+/* Spinlock for mana_shared_data */
+static rte_spinlock_t mana_shared_data_lock =

RTE_SPINLOCK_INITIALIZER;

Quoted 12 lines

drivers/net/mana/mana.c new file +12 -0

+
+/* Allocate a buffer on the stack and fill it with a printf format string. */
+#define MKSTR(name, ...) \
+       int mkstr_size_##name = snprintf(NULL, 0, "" __VA_ARGS__); \
+       char name[mkstr_size_##name + 1]; \
+       \
+       memset(name, 0, mkstr_size_##name + 1); \
+       snprintf(name, sizeof(name), "" __VA_ARGS__)
+
+int mana_logtype_driver;
+int mana_logtype_init;
+

drivers/net/mana/mana.c line 47

Quoted 5 lines

drivers/net/mana/mana.c new file +5 -0

+const struct eth_dev_ops mana_dev_ops = {
+};
+
+const struct eth_dev_ops mana_dev_sec_ops = {
+};

It may be better to expand 'sec' to secondary to not confuse with security etc...

Quoted 24 lines

drivers/net/mana/mana.c new file +23 -0

+
+uint16_t
+mana_rx_burst_removed(void *dpdk_rxq __rte_unused,
+                     struct rte_mbuf **pkts __rte_unused,
+                     uint16_t pkts_n __rte_unused)
+{
+       rte_mb();
+       return 0;
+}
+
+uint16_t
+mana_tx_burst_removed(void *dpdk_rxq __rte_unused,
+                     struct rte_mbuf **pkts __rte_unused,
+                     uint16_t pkts_n __rte_unused)
+{
+       rte_mb();
+       return 0;
+}
+
+static const char *mana_init_args[] = {
+       "mac",
+       NULL,
+};

drivers/net/mana/mana.c line 80

Quoted 10 lines

drivers/net/mana/mana.c new file +10 -0

+
+/* Support of parsing up to 8 mac address from EAL command line */
+#define MAX_NUM_ADDRESS 8
+struct mana_conf {
+       struct rte_ether_addr mac_array[MAX_NUM_ADDRESS];
+       unsigned int index;
+};
+
+static int mana_arg_parse_callback(const char *key, const char *val,
+                                  void *private)

Since this is new driver, better to follow the coding convention:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.
dpdk.org%2Fguides%2Fcontributing%2Fcoding_style.html&amp;data=05%7C 01%7Clongli%40microsoft.com%7Ccd41ac9636a7472eabf408da844f861e%7C7 2f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637967774356090060%7CU nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=qTHvw4H%2 FGVn6DVyFZaSQNhDVqBxyGTSPHKDwSCLxarw%3D&amp;reserved=0

Please put return type to another line:

static int
mana_arg_parse_callback(const char *key, const char *val, void *private)

Quoted 23 lines

drivers/net/mana/mana.c new file +22 -0

+{
+       struct mana_conf *conf = (struct mana_conf *)private;
+       int ret;
+
+       DRV_LOG(INFO, "key=%s value=%s index=%d", key, val, conf->index);
+
+       if (conf->index >= MAX_NUM_ADDRESS) {
+               DRV_LOG(ERR, "Exceeding max MAC address");
+               return 1;
+       }
+
+       ret = rte_ether_unformat_addr(val, &conf->mac_array[conf->index]);
+       if (ret) {
+               DRV_LOG(ERR, "Invalid MAC address %s", val);
+               return ret;
+       }
+
+       conf->index++;
+
+       return 0;
+}
+

<...>

Quoted 37 lines

drivers/net/mana/mana.c new file +37 -0

+static int get_port_mac(struct ibv_device *device, unsigned int port,
+                       struct rte_ether_addr *addr)
+{
+       FILE *file;
+       int ret = 0;
+       DIR *dir;
+       struct dirent *dent;
+       unsigned int dev_port;
+       char mac[20];
+
+       MKSTR(path, "%s/device/net", device->ibdev_path);
+
+       dir = opendir(path);
+       if (!dir)
+               return -ENOENT;
+
+       while ((dent = readdir(dir))) {
+               char *name = dent->d_name;
+
+               MKSTR(filepath, "%s/%s/dev_port", path, name);
+
+               /* Ignore . and .. */
+               if ((name[0] == '.') &&
+                   ((name[1] == '\0') ||
+                    ((name[1] == '.') && (name[2] == '\0'))))
+                       continue;
+
+               file = fopen(filepath, "r");
+               if (!file)
+                       continue;
+
+               ret = fscanf(file, "%u", &dev_port);
+               fclose(file);
+
+               if (ret != 1)
+                       continue;
+

drivers/net/mana/mana.c line 171

Quoted 3 lines

drivers/net/mana/mana.c new file +3 -0

+               /* Ethernet ports start at 0, IB port start at 1 */
+               if (dev_port == port - 1) {
+                       MKSTR(filepath, "%s/%s/address", path, name);

'MKSTR' macro adds two variables related with first argument, 'filepath' already used above. Yes there is a new scope but better to not define new variables, can you select a new name here?

<...>

drivers/net/mana/mana.c

Quoted 2 lines

drivers/net/mana/mana.c new file +2 -0

+
+static int mana_pci_probe_mac(struct rte_pci_driver *pci_drv

__rte_unused,

This is a static function, if you don't use 'pci_drv', why not drop it from the argument list.

Quoted 11 lines

drivers/net/mana/mana.c new file +10 -0

+                             struct rte_pci_device *pci_dev,
+                             struct rte_ether_addr *mac_addr)
+{
+       struct ibv_device **ibv_list;
+       int ibv_idx;
+       struct ibv_context *ctx;
+       struct ibv_device_attr_ex dev_attr;
+       int num_devices;
+       int ret = 0;
+       uint8_t port;

drivers/net/mana/mana.c

Quoted 10 lines

drivers/net/mana/mana.c new file +10 -0

+       struct mana_priv *priv = NULL;
+       struct rte_eth_dev *eth_dev = NULL;
+       bool found_port;
+
+       ibv_list = ibv_get_device_list(&num_devices);
+       for (ibv_idx = 0; ibv_idx < num_devices; ibv_idx++) {
+               struct ibv_device *ibdev = ibv_list[ibv_idx];
+               struct rte_pci_addr pci_addr;
+
+               DRV_LOG(INFO, "Probe device name %s dev_name %s

ibdev_path %s",

drivers/net/mana/mana.c line 383

Quoted 12 lines

drivers/net/mana/mana.c new file +12 -0

+                       ibdev->name, ibdev->dev_name, ibdev->ibdev_path);
+
+               if (mana_ibv_device_to_pci_addr(ibdev, &pci_addr))
+                       continue;
+
+               /* Ignore if this IB device is not this PCI device */
+               if (pci_dev->addr.domain != pci_addr.domain ||
+                   pci_dev->addr.bus != pci_addr.bus ||
+                   pci_dev->addr.devid != pci_addr.devid ||
+                   pci_dev->addr.function != pci_addr.function)
+                       continue;
+

As far as I understand, intention of this loop is to find 'ibdev' matching this device, code gooes through all "ibv device list" for this, I wonder if there is a easy way for doing this, like a sysfs entry to help getting this information?
And how mlx4/5 does this?

Quoted 5 lines

drivers/net/mana/mana.c new file +5 -0

+               ctx = ibv_open_device(ibdev);
+               if (!ctx) {
+                       DRV_LOG(ERR, "Failed to open IB device %s",
+                               ibdev->name);
+                       continue;

drivers/net/mana/mana.c line 398

Quoted 10 lines

drivers/net/mana/mana.c new file +10 -0

+               }
+
+               ret = ibv_query_device_ex(ctx, NULL, &dev_attr);
+               DRV_LOG(INFO, "dev_attr.orig_attr.phys_port_cnt %u",
+                       dev_attr.orig_attr.phys_port_cnt);
+               found_port = false;
+
+               for (port = 1; port <= dev_attr.orig_attr.phys_port_cnt;
+                    port++) {
+                       struct ibv_parent_domain_init_attr attr = {};

"= { 0 };" for portability.

<...>

drivers/net/mana/mana.c line 596

Quoted 5 lines

drivers/net/mana/mana.c new file +5 -0

+static int mana_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
+                         struct rte_pci_device *pci_dev)
+{
+       struct rte_devargs *args = pci_dev->device.devargs;
+       struct mana_conf conf = {};

afaik, this is not part of c spec yet, why not initialize as " = {0}".

drivers/net/mana/mana.c line 600

Quoted 5 lines

drivers/net/mana/mana.c new file +4 -0

+       unsigned int i;
+       int ret;
+
+       if (args && args->args) {

You can prefer 'args->drv_str', which is newer name of the args.

<...>

drivers/net/mana/mana.c line 682

Quoted 5 lines

drivers/net/mana/mana.c new file +5 -0

+static const struct rte_pci_id mana_pci_id_map[] = {
+       {
+               RTE_PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT,
+                              PCI_DEVICE_ID_MICROSOFT_MANA)
+       },

PCI ID list should be terminated with ".vendor_id = 0", otherwise PCI bus scan loop may behave unexpectedly.

drivers/net/mana/mana.c line 687

Quoted 6 lines

drivers/net/mana/mana.c new file +5 -0

+};
+
+static struct rte_pci_driver mana_pci_driver = {
+       .driver = {
+               .name = "mana_pci",

driver names are mostly like 'net_<driver_name>', is there a reason to diverge from it?
Also if you use 'RTE_PMD_REGISTER_PCI' macro, it will be standardised anyway.

drivers/net/mana/mana.c line 699

Quoted 12 lines

drivers/net/mana/mana.c new file +12 -0

+       },
+       .id_table = mana_pci_id_map,
+       .probe = mana_pci_probe,
+       .remove = mana_pci_remove,
+       .drv_flags = RTE_PCI_DRV_INTR_RMV,
+};
+
+RTE_INIT(rte_mana_pmd_init)
+{
+       rte_pci_register(&mana_pci_driver);
+}
+

Why not using 'RTE_PMD_REGISTER_PCI()' macro instead?

Quoted 35 lines

drivers/net/mana/mana.c new file +5 -0

+RTE_PMD_EXPORT_NAME(net_mana, __COUNTER__);
+RTE_PMD_REGISTER_PCI_TABLE(net_mana, mana_pci_id_map);
+RTE_PMD_REGISTER_KMOD_DEP(net_mana, "* ib_uverbs & mana_ib");
+RTE_LOG_REGISTER_SUFFIX(mana_logtype_init, init, NOTICE);
+RTE_LOG_REGISTER_SUFFIX(mana_logtype_driver, driver, NOTICE);

drivers/net/mana/mana.h new file +23 -0

new file mode 100644
index 0000000000..e30c030b4e
@@ -0,0 +1,210 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2022 Microsoft Corporation
+ */
+
+#ifndef __MANA_H__
+#define __MANA_H__
+
+enum {
+       PCI_VENDOR_ID_MICROSOFT = 0x1414,
+};
+
+enum {
+       PCI_DEVICE_ID_MICROSOFT_MANA = 0x00ba,
+};
+
+/* Shared data between primary/secondary processes */
+struct mana_shared_data {
+       rte_spinlock_t lock;
+       int init_done;
+       unsigned int primary_cnt;
+       unsigned int secondary_cnt;
+};
+

drivers/net/mana/mana.h line 27

Quoted 4 lines

drivers/net/mana/mana.h new file +4 -0

+#define MIN_RX_BUF_SIZE        1024
+#define MAX_FRAME_SIZE RTE_ETHER_MAX_LEN
+#define BNIC_MAX_MAC_ADDR 1
+

What 'BNIC_' prefix stands for? If it is related to the PMD, what do you think to use 'MANA_' as prefix?
Same for multiple macros below.

<...>

drivers/net/mana/mana.h line 172

Quoted 5 lines

drivers/net/mana/mana.h new file +5 -0

+
+#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>")
+
+const uint32_t *mana_supported_ptypes(struct rte_eth_dev *dev);
+

This function is not defined in this patch, so can drop declarataion.

<...>

Review

Quoted 1 line

(quoted diff) +0 -0

b/drivers/net/mana/version.map

drivers/net/mana/version.map line 1

Quoted 6 lines

new file mode 100644
index 0000000000..c2e0723b4c

drivers/net/mana/version.map new file +1 -0

@@ -0,0 +1,3 @@
+DPDK_22 {

It is 'DPDK_23' now.

From: Long Li <[email protected]>

The hardware layer of MANA understands the device queue and doorbell formats. Those functions are implemented for use by packet RX/TX code.

Signed-off-by Long Li <[email protected]>

---
Change log:
v2:
Remove unused header files.
Rename a camel case.

Subject: Re: [Patch v4 11/17] net/mana: implement the hardware layer operations

Quoted 13 lines · On 7/9/2022 12:49 AM, [email protected] wrote:

From: Long Li <[email protected]>

The hardware layer of MANA understands the device queue and doorbell formats. Those functions are implemented for use by packet RX/TX code.

Signed-off-by Long Li <[email protected]>

---
Change log:
v2:
Remove unused header files.
Rename a camel case.

<...>

drivers/net/mana/mana.h struct mana_shared_data { · line 199

Quoted 11 lines

drivers/net/mana/mana.h +11 -0

+/* NDIS HASH Types */
+#define BIT(nr)                (1 << (nr))
+#define NDIS_HASH_IPV4          BIT(0)
+#define NDIS_HASH_TCP_IPV4      BIT(1)
+#define NDIS_HASH_UDP_IPV4      BIT(2)
+#define NDIS_HASH_IPV6          BIT(3)
+#define NDIS_HASH_TCP_IPV6      BIT(4)
+#define NDIS_HASH_UDP_IPV6      BIT(5)
+#define NDIS_HASH_IPV6_EX       BIT(6)
+#define NDIS_HASH_TCP_IPV6_EX   BIT(7)
+#define NDIS_HASH_UDP_IPV6_EX   BIT(8)

Can use RTE_BIT32/64 instead of defining new macro

CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.

From: Long Li <[email protected]>

With all the TX queues created, MANA can send packets over those queues.

Signed-off-by Long Li <[email protected]>

---
Change log:
v2:
Rename all camel cases.

Quoted 1 line

+ for (uint16_t seg_idx = 0; seg_idx < m_pkt->nb_segs; seg_idx++) {

This is C99 feature and we tend to not use it BUT,

Thomas, David,

In the past there were c89/c90/ansi code in DPDK, but as far as I can see no more left.

Is there anything that prevents us using C99 features, like variable in the for loop, I think it is usefull that it reduces the scope and makes code more readable, etc...

CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.

From: Long Li <[email protected]>

Report packet statistics.

Signed-off-by Long Li <[email protected]>

drivers/net/mana/mana.c +4 -0

+static int mana_dev_stats_get(struct rte_eth_dev *dev,
+                             struct rte_eth_stats *stats)
+{
+       unsigned int i;

Subject: Re: [Patch v4 17/17] net/mana: add function to report queue stats

Quoted 1 line · On 7/9/2022 12:49 AM, [email protected] wrote:

CAUTION: This message has originated from an External Source. Please use

proper judgment and caution when opening attachments, clicking links, or responding to this email.

Quoted 7 lines

From: Long Li <[email protected]>

Report packet statistics.

Signed-off-by Long Li <[email protected]>

<...>

drivers/net/mana/mana.c static int mana_dev_link_update(struct rte_eth_dev *dev, · line 517

Quoted 13 lines

drivers/net/mana/mana.c +13 -0

+static int mana_dev_stats_get(struct rte_eth_dev *dev,
+                             struct rte_eth_stats *stats) {
+       unsigned int i;
+
+       for (i = 0; i < dev->data->nb_tx_queues; i++) {
+               struct mana_txq *txq = dev->data->tx_queues[i];
+
+               if (!txq)
+                       continue;
+
+               stats->opackets = txq->stats.packets;
+               stats->obytes = txq->stats.bytes;
+               stats->oerrors = txq->stats.errors;

Shouldn't these be "+=" ? Same for below Rx block.

From: Long Li <[email protected]>

MANA is a network interface card to be used in the Azure cloud environment. MANA provides safe access to user memory through memory registration. It has IOMMU built into the hardware.

MANA uses IB verbs and RDMA layer to configure hardware resources. It requires the corresponding RDMA kernel-mode and user-mode drivers.

The MANA RDMA kernel-mode driver is being reviewed at:
https://patchwork.kernel.org/project/netdevbpf/cover/[email protected]/

The MANA RDMA user-mode driver is being reviewed at:
https://github.com/linux-rdma/rdma-core/pull/1177

Long Li (17):
net/mana: add basic driver, build environment and doc net/mana: add device configuration and stop net/mana: add function to report support ptypes net/mana: add link update
net/mana: add function for device removal interrupts net/mana: add device info
net/mana: add function to configure RSS net/mana: add function to configure RX queues net/mana: add function to configure TX queues net/mana: implement memory registration net/mana: implement the hardware layer operations net/mana: add function to start/stop TX queues net/mana: add function to start/stop RX queues net/mana: add function to receive packets net/mana: add function to send packets net/mana: add function to start/stop device net/mana: add function to report queue stats

Subject: Re: [Patch v4 00/17] Introduce Microsoft Azure Network Adatper (MANA) PMD

Quoted 4 lines · On 7/9/2022 12:49 AM, [email protected] wrote:

From: Long Li <[email protected]>

MANA is a network interface card to be used in the Azure cloud

environment.

Quoted 8 lines

MANA provides safe access to user memory through memory registration. It has IOMMU built into the hardware.

MANA uses IB verbs and RDMA layer to configure hardware resources. It requires the corresponding RDMA kernel-mode and user-mode drivers.

The MANA RDMA kernel-mode driver is being reviewed at:

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc

Quoted 1 line

hwork.kernel.org%2Fproject%2Fnetdevbpf%2Fcover%2F1655345240-

26411-1-gi

Quoted 1 line

t-send-email-

longli%40linuxonhyperv.com%2F&amp;data=05%7C01%7Clongli%4

Quoted 1 line

0microsoft.com%7C232a4a7af70f490b94f608da844ee782%7C72f988bf86f141a f91

Quoted 1 line

ab2d7cd011db47%7C1%7C0%7C637967771686870922%7CUnknown%7CTWFp bGZsb3d8ey

Quoted 1 line

JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D% 7C300

Quoted 1 line

0%7C%7C%7C&amp;sdata=bC7l7wxfU5sbahBye8CKVS%2BJvirO%2FvIMU3M oS4%2Fdpc8

Quoted 4 lines

%3D&amp;reserved=0

The MANA RDMA user-mode driver is being reviewed at:

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith

Quoted 1 line

ub.com%2Flinux-rdma%2Frdma-

core%2Fpull%2F1177&amp;data=05%7C01%7Clongl

Quoted 1 line

i%40microsoft.com%7C232a4a7af70f490b94f608da844ee782%7C72f988bf86f1 41a

Quoted 1 line

f91ab2d7cd011db47%7C1%7C0%7C637967771686870922%7CUnknown%7CT WFpbGZsb3d

Quoted 1 line

8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0% 3D%7C

Quoted 1 line

3000%7C%7C%7C&amp;sdata=DLGg1q1N3%2FNtr7ii3WI2rqShPNoi2u0MF8J8 Cn1h60s%

Quoted 2 lines

3D&amp;reserved=0

Hi Long,

How are the dependencies are progressing? Is there anything blocker for DPDK upstream?

Quoted 21 lines

Long Li (17):
net/mana: add basic driver, build environment and doc net/mana: add device configuration and stop net/mana: add function to report support ptypes net/mana: add link update
net/mana: add function for device removal interrupts net/mana: add device info
net/mana: add function to configure RSS net/mana: add function to configure RX queues net/mana: add function to configure TX queues net/mana: implement memory registration net/mana: implement the hardware layer operations net/mana: add function to start/stop TX queues net/mana: add function to start/stop RX queues net/mana: add function to receive packets net/mana: add function to send packets net/mana: add function to start/stop device net/mana: add function to report queue stats

<...>