[01/10] vdpa/sfc: introduce Xilinx vDPA driver
Checks
Commit Message
From: Vijay Kumar Srivastava <vsrivast@xilinx.com>
Add new vDPA PMD to support vDPA operation by Xilinx devices.
This patch implements probe and remove functions.
Signed-off-by: Vijay Kumar Srivastava <vsrivast@xilinx.com>
---
MAINTAINERS | 6 +
doc/guides/rel_notes/release_21_08.rst | 5 +
doc/guides/vdpadevs/features/sfc.ini | 9 ++
doc/guides/vdpadevs/sfc.rst | 97 +++++++++++
drivers/vdpa/meson.build | 1 +
drivers/vdpa/sfc/meson.build | 33 ++++
drivers/vdpa/sfc/sfc_vdpa.c | 286 +++++++++++++++++++++++++++++++++
drivers/vdpa/sfc/sfc_vdpa.h | 40 +++++
drivers/vdpa/sfc/sfc_vdpa_log.h | 77 +++++++++
drivers/vdpa/sfc/version.map | 3 +
10 files changed, 557 insertions(+)
create mode 100644 doc/guides/vdpadevs/features/sfc.ini
create mode 100644 doc/guides/vdpadevs/sfc.rst
create mode 100644 drivers/vdpa/sfc/meson.build
create mode 100644 drivers/vdpa/sfc/sfc_vdpa.c
create mode 100644 drivers/vdpa/sfc/sfc_vdpa.h
create mode 100644 drivers/vdpa/sfc/sfc_vdpa_log.h
create mode 100644 drivers/vdpa/sfc/version.map
Comments
Hi Vijay,
> -----Original Message-----
> From: Vijay Srivastava <vijay.srivastava@xilinx.com>
> Sent: Wednesday, July 7, 2021 12:44 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava <vsrivast@xilinx.com>
> Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>
> From: Vijay Kumar Srivastava <vsrivast@xilinx.com>
>
> Add new vDPA PMD to support vDPA operation by Xilinx devices.
vDPA operations of Xilinx devices
> This patch implements probe and remove functions.
>
> Signed-off-by: Vijay Kumar Srivastava <vsrivast@xilinx.com>
> ---
> MAINTAINERS | 6 +
> doc/guides/rel_notes/release_21_08.rst | 5 +
> doc/guides/vdpadevs/features/sfc.ini | 9 ++
> doc/guides/vdpadevs/sfc.rst | 97 +++++++++++
> drivers/vdpa/meson.build | 1 +
> drivers/vdpa/sfc/meson.build | 33 ++++
> drivers/vdpa/sfc/sfc_vdpa.c | 286
> +++++++++++++++++++++++++++++++++
> drivers/vdpa/sfc/sfc_vdpa.h | 40 +++++
> drivers/vdpa/sfc/sfc_vdpa_log.h | 77 +++++++++
> drivers/vdpa/sfc/version.map | 3 +
> 10 files changed, 557 insertions(+)
> create mode 100644 doc/guides/vdpadevs/features/sfc.ini
> create mode 100644 doc/guides/vdpadevs/sfc.rst
> create mode 100644 drivers/vdpa/sfc/meson.build
> create mode 100644 drivers/vdpa/sfc/sfc_vdpa.c
> create mode 100644 drivers/vdpa/sfc/sfc_vdpa.h
> create mode 100644 drivers/vdpa/sfc/sfc_vdpa_log.h
> create mode 100644 drivers/vdpa/sfc/version.map
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5877a16..ccc0a2a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1197,6 +1197,12 @@ F: drivers/vdpa/mlx5/
> F: doc/guides/vdpadevs/mlx5.rst
> F: doc/guides/vdpadevs/features/mlx5.ini
>
> +Xilinx sfc vDPA
> +M: Vijay Kumar Srivastava <vsrivast@xilinx.com>
> +F: drivers/vdpa/sfc/
> +F: doc/guides/vdpadevs/sfc.rst
> +F: doc/guides/vdpadevs/features/sfc.ini
> +
>
> Eventdev Drivers
> ----------------
> diff --git a/doc/guides/rel_notes/release_21_08.rst
> b/doc/guides/rel_notes/release_21_08.rst
> index a6ecfdf..bb9aa83 100644
> --- a/doc/guides/rel_notes/release_21_08.rst
> +++ b/doc/guides/rel_notes/release_21_08.rst
> @@ -55,6 +55,11 @@ New Features
> Also, make sure to start the actual text at the margin.
> =======================================================
>
> +* **Add new vDPA PMD based on Xilinx devices.**
> +
> + Added a new Xilinx vDPA (``sfc_vdpa``) PMD.
> + See the :doc:`../vdpadevs/sfc` guide for more details on this driver.
> +
>
> Removed Items
> -------------
> diff --git a/doc/guides/vdpadevs/features/sfc.ini
> b/doc/guides/vdpadevs/features/sfc.ini
> new file mode 100644
> index 0000000..71b6158
> --- /dev/null
> +++ b/doc/guides/vdpadevs/features/sfc.ini
> @@ -0,0 +1,9 @@
> +;
> +; Supported features of the 'sfc' vDPA driver.
> +;
> +; Refer to default.ini for the full list of available driver features.
> +;
> +[Features]
> +Linux = Y
> +x86-64 = Y
> +Usage doc = Y
> diff --git a/doc/guides/vdpadevs/sfc.rst b/doc/guides/vdpadevs/sfc.rst
> new file mode 100644
> index 0000000..59f990b
> --- /dev/null
> +++ b/doc/guides/vdpadevs/sfc.rst
> @@ -0,0 +1,97 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> + Copyright(c) 2021 Xilinx Corporation.
> +
> +Xilinx vDPA driver
> +==================
> +
> +The Xilinx vDPA (vhost data path acceleration) driver
> (**librte_pmd_sfc_vdpa**)
> +provides support for the Xilinx SN1022 SmartNICs family of 10/25/40/50/100
> Gbps
> +adapters has support for latest Linux and FreeBSD operating systems.
Adapters that have support for XXX?
> +
> +More information can be found at Xilinx website https://www.xilinx.com.
> +
> +
> +Xilinx vDPA implementation
> +--------------------------
> +
> +ef100 device can be configured in the net device or vDPA mode.
> +Adding "class=vdpa" parameter helps to specify that this
> +device is to be used in vDPA mode. If this parameter is not specified, device
> +will be probed by net/sfc driver and will used as a net device.
> +
> +This PMD uses libefx (common/sfc_efx) code to access the device firmware.
> +
> +
> +Supported NICs
> +--------------
> +
> +- Xilinx SN1022 SmartNICs
> +
> +
> +Features
> +--------
> +
> +Features of the Xilinx vDPA driver are:
> +
> +- Compatibility with virtio 0.95 and 1.0
> +
> +
> +Non-supported Features
> +----------------------
> +
> +- Control Queue
> +- Multi queue
> +- Live Migration
> +
> +
> +Prerequisites
> +-------------
> +
> +Requires firmware version: v1.0.7.0 or higher
> +
> +Visit `Xilinx Support Downloads <https://www.xilinx.com/support.html>`_
> +to get Xilinx Utilities with the latest firmware.
> +Follow instructions from Alveo SN1000 SmartNICs User Guide to
> +update firmware and configure the adapter.
> +
> +
> +Per-Device Parameters
> +~~~~~~~~~~~~~~~~~~~~~
> +
> +The following per-device parameters can be passed via EAL PCI device
> +whitelist option like "-w 02:00.0,arg1=value1,...".
The name 'whitelist' is deprecated in DPDK now. So please use:
allowlist option like "-a 02:00.0,arg1=value1,..."
> +
> +Case-insensitive 1/y/yes/on or 0/n/no/off may be used to specify
> +boolean parameters value.
> +
> +- ``class`` [net|vdpa] (default **net**)
> +
> + Choose the mode of operation of ef100 device.
> + **net** device will work as network device and will be probed by net/sfc
> driver.
> + **vdpa** device will work as vdpa device and will be probed by vdpa/sfc
> driver.
> + If this parameter is not specified then ef100 device will operate as
> network device.
> +
> +
> +Dynamic Logging Parameters
> +~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +One may leverage EAL option "--log-level" to change default levels
> +for the log types supported by the driver. The option is used with
> +an argument typically consisting of two parts separated by a colon.
> +
> +Level value is the last part which takes a symbolic name (or integer).
> +Log type is the former part which may shell match syntax.
> +Depending on the choice of the expression, the given log level may
> +be used either for some specific log type or for a subset of types.
> +
> +SFC vDPA PMD provides the following log types available for control:
> +
> +- ``pmd.vdpa.sfc.driver`` (default level is **notice**)
> +
> + Affects driver-wide messages unrelated to any particular devices.
> +
> +- ``pmd.vdpa.sfc.main`` (default level is **notice**)
> +
> + Matches a subset of per-port log types registered during runtime.
> + A full name for a particular type may be obtained by appending a
> + dot and a PCI device identifier (``XXXX:XX:XX.X``) to the prefix.
> diff --git a/drivers/vdpa/meson.build b/drivers/vdpa/meson.build
> index f765fe3..77412c7 100644
> --- a/drivers/vdpa/meson.build
> +++ b/drivers/vdpa/meson.build
> @@ -8,6 +8,7 @@ endif
> drivers = [
> 'ifc',
> 'mlx5',
> + 'sfc',
> ]
> std_deps = ['bus_pci', 'kvargs']
> std_deps += ['vhost']
> diff --git a/drivers/vdpa/sfc/meson.build b/drivers/vdpa/sfc/meson.build
> new file mode 100644
> index 0000000..d916389
> --- /dev/null
> +++ b/drivers/vdpa/sfc/meson.build
> @@ -0,0 +1,33 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +#
> +# Copyright(c) 2020-2021 Xilinx, Inc.
> +
> +if (arch_subdir != 'x86' or not dpdk_conf.get('RTE_ARCH_64')) and
> (arch_subdir != 'arm' or not host_machine.cpu_family().startswith('aarch64'))
> + build = false
> + reason = 'only supported on x86_64 and aarch64'
> +endif
> +
> +fmt_name = 'sfc_vdpa'
> +extra_flags = []
> +
> +# Enable more warnings
> +extra_flags += [
> + '-Wdisabled-optimization'
> +]
> +
> +# Compiler and version dependent flags
> +extra_flags += [
> + '-Waggregate-return',
> + '-Wbad-function-cast'
> +]
> +
> +foreach flag: extra_flags
> + if cc.has_argument(flag)
> + cflags += flag
> + endif
> +endforeach
> +
> +deps += ['common_sfc_efx', 'bus_pci']
> +sources = files(
> + 'sfc_vdpa.c',
> +)
> diff --git a/drivers/vdpa/sfc/sfc_vdpa.c b/drivers/vdpa/sfc/sfc_vdpa.c
> new file mode 100644
> index 0000000..d8faaca
> --- /dev/null
> +++ b/drivers/vdpa/sfc/sfc_vdpa.c
> @@ -0,0 +1,286 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + *
> + * Copyright(c) 2020-2021 Xilinx, Inc.
> + */
> +
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <sys/queue.h>
> +
> +#include <rte_common.h>
> +#include <rte_errno.h>
> +#include <rte_string_fns.h>
> +#include <rte_vfio.h>
> +#include <rte_vhost.h>
> +
> +#include "efx.h"
> +#include "sfc_efx.h"
> +#include "sfc_vdpa.h"
> +
> +TAILQ_HEAD(sfc_vdpa_adapter_list_head, sfc_vdpa_adapter);
> +static struct sfc_vdpa_adapter_list_head sfc_vdpa_adapter_list =
> + TAILQ_HEAD_INITIALIZER(sfc_vdpa_adapter_list);
> +
> +static pthread_mutex_t sfc_vdpa_adapter_list_lock = PTHREAD_MUTEX_INITIALIZER;
> +
> +struct sfc_vdpa_adapter *
> +sfc_vdpa_get_adapter_by_dev(struct rte_pci_device *pdev)
> +{
> + bool found = false;
> + struct sfc_vdpa_adapter *sva;
> +
> + pthread_mutex_lock(&sfc_vdpa_adapter_list_lock);
> +
> + TAILQ_FOREACH(sva, &sfc_vdpa_adapter_list, next) {
> + if (pdev == sva->pdev) {
> + found = true;
> + break;
> + }
> + }
> +
> + pthread_mutex_unlock(&sfc_vdpa_adapter_list_lock);
> +
> + return found ? sva : NULL;
> +}
> +
> +static int
> +sfc_vdpa_vfio_setup(struct sfc_vdpa_adapter *sva)
> +{
> + struct rte_pci_device *dev = sva->pdev;
> + char dev_name[RTE_DEV_NAME_MAX_LEN] = {0};
> + int rc;
> +
> + if (dev == NULL)
> + goto fail_inval;
> +
> + rte_pci_device_name(&dev->addr, dev_name, RTE_DEV_NAME_MAX_LEN);
> +
> + sva->vfio_container_fd = rte_vfio_container_create();
> + if (sva->vfio_container_fd < 0) {
> + sfc_vdpa_err(sva, "failed to create VFIO container");
failed -> Failed
I suggest to use capital letter for first letter of every log info.
Please check other logs.
> + goto fail_container_create;
> + }
> +
> + rc = rte_vfio_get_group_num(rte_pci_get_sysfs_path(), dev_name,
> + &sva->iommu_group_num);
> + if (rc <= 0) {
> + sfc_vdpa_err(sva, "failed to get IOMMU group for %s : %s",
> + dev_name, rte_strerror(-rc));
> + goto fail_get_group_num;
> + }
> +
> + sva->vfio_group_fd =
> + rte_vfio_container_group_bind(sva->vfio_container_fd,
> + sva->iommu_group_num);
> + if (sva->vfio_group_fd < 0) {
> + sfc_vdpa_err(sva,
> + "failed to bind IOMMU group %d to container %d",
> + sva->iommu_group_num, sva->vfio_container_fd);
> + goto fail_group_bind;
> + }
> +
> + if (rte_pci_map_device(dev) != 0) {
> + sfc_vdpa_err(sva, "failed to map PCI device %s : %s",
> + dev_name, rte_strerror(rte_errno));
> + goto fail_pci_map_device;
> + }
> +
> + sva->vfio_dev_fd = dev->intr_handle.vfio_dev_fd;
> +
> + return 0;
> +
> +fail_pci_map_device:
> + if (rte_vfio_container_group_unbind(sva->vfio_container_fd,
> + sva->iommu_group_num) != 0) {
> + sfc_vdpa_err(sva,
> + "failed to unbind IOMMU group %d from container %d",
> + sva->iommu_group_num, sva->vfio_container_fd);
> + }
> +
> +fail_group_bind:
> +fail_get_group_num:
You can combined these two tags into one with tag name changed.
> + if (rte_vfio_container_destroy(sva->vfio_container_fd) != 0) {
> + sfc_vdpa_err(sva, "failed to destroy container %d",
> + sva->vfio_container_fd);
> + }
> +
> +fail_container_create:
> +fail_inval:
Same here.
> + return -1;
> +}
> +
> +static void
> +sfc_vdpa_vfio_teardown(struct sfc_vdpa_adapter *sva)
> +{
> + rte_pci_unmap_device(sva->pdev);
> +
> + if (rte_vfio_container_group_unbind(sva->vfio_container_fd,
> + sva->iommu_group_num) != 0) {
> + sfc_vdpa_err(sva,
> + "failed to unbind IOMMU group %d from container %d",
> + sva->iommu_group_num, sva->vfio_container_fd);
> + }
> +
> + if (rte_vfio_container_destroy(sva->vfio_container_fd) != 0) {
> + sfc_vdpa_err(sva,
> + "failed to destroy container %d",
> + sva->vfio_container_fd);
> + }
> +}
> +
> +static int
> +sfc_vdpa_set_log_prefix(struct sfc_vdpa_adapter *sva)
> +{
> + struct rte_pci_device *pci_dev = sva->pdev;
> + int ret;
> +
> + ret = snprintf(sva->log_prefix, sizeof(sva->log_prefix),
> + "PMD: sfc_vdpa " PCI_PRI_FMT " : ",
> + pci_dev->addr.domain, pci_dev->addr.bus,
> + pci_dev->addr.devid, pci_dev->addr.function);
> +
> + if (ret < 0 || ret >= (int)sizeof(sva->log_prefix)) {
> + SFC_VDPA_GENERIC_LOG(ERR,
> + "reserved log prefix is too short for " PCI_PRI_FMT,
> + pci_dev->addr.domain, pci_dev->addr.bus,
> + pci_dev->addr.devid, pci_dev->addr.function);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +uint32_t
> +sfc_vdpa_register_logtype(const struct rte_pci_addr *pci_addr,
> + const char *lt_prefix_str, uint32_t ll_default)
> +{
> + size_t lt_prefix_str_size = strlen(lt_prefix_str);
> + size_t lt_str_size_max;
> + char *lt_str = NULL;
> + int ret;
> +
> + if (SIZE_MAX - PCI_PRI_STR_SIZE - 1 > lt_prefix_str_size) {
> + ++lt_prefix_str_size; /* Reserve space for prefix separator */
> + lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
> + } else {
> + return RTE_LOGTYPE_PMD;
> + }
> +
> + lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
> + if (lt_str == NULL)
> + return RTE_LOGTYPE_PMD;
> +
> + strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
> + lt_str[lt_prefix_str_size - 1] = '.';
> + rte_pci_device_name(pci_addr, lt_str + lt_prefix_str_size,
> + lt_str_size_max - lt_prefix_str_size);
> + lt_str[lt_str_size_max - 1] = '\0';
> +
> + ret = rte_log_register_type_and_pick_level(lt_str, ll_default);
> + rte_free(lt_str);
> +
> + return (ret < 0) ? RTE_LOGTYPE_PMD : ret;
'()' not needed
> +}
> +
> +static struct rte_pci_id pci_id_sfc_vdpa_efx_map[] = {
> + { RTE_PCI_DEVICE(EFX_PCI_VENID_XILINX, EFX_PCI_DEVID_RIVERHEAD_VF) },
> + { .vendor_id = 0, /* sentinel */ },
> +};
> +
> +static int
> +sfc_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> + struct rte_pci_device *pci_dev)
> +{
> + struct sfc_vdpa_adapter *sva = NULL;
> + uint32_t logtype_main;
> + int ret = 0;
> +
> + if (sfc_efx_dev_class_get(pci_dev->device.devargs) !=
> + SFC_EFX_DEV_CLASS_VDPA) {
> + SFC_VDPA_GENERIC_LOG(INFO,
> + "Incompatible device class: skip probing, should be probed
> by other sfc driver.");
> + return 1;
> + }
> +
> + /*
> + * It will not be probed in the secondary process. As device class
> + * is vdpa so return 0 to avoid probe by other sfc driver
> + */
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> + return 0;
> +
> + logtype_main = sfc_vdpa_register_logtype(&pci_dev->addr,
> + SFC_VDPA_LOGTYPE_MAIN_STR,
> + RTE_LOG_NOTICE);
> +
> + sva = rte_zmalloc("sfc_vdpa", sizeof(struct sfc_vdpa_adapter), 0);
> + if (sva == NULL)
> + goto fail_zmalloc;
> +
> + sva->pdev = pci_dev;
> + sva->logtype_main = logtype_main;
> +
> + ret = sfc_vdpa_set_log_prefix(sva);
> + if (ret != 0)
> + goto fail_set_log_prefix;
> +
> + sfc_vdpa_log_init(sva, "entry");
> +
> + sfc_vdpa_log_init(sva, "vfio init");
> + if (sfc_vdpa_vfio_setup(sva) < 0) {
> + sfc_vdpa_err(sva, "failed to setup device %s", pci_dev->name);
> + goto fail_vfio_setup;
> + }
> +
> + pthread_mutex_lock(&sfc_vdpa_adapter_list_lock);
> + TAILQ_INSERT_TAIL(&sfc_vdpa_adapter_list, sva, next);
> + pthread_mutex_unlock(&sfc_vdpa_adapter_list_lock);
> +
> + sfc_vdpa_log_init(sva, "done");
> +
> + return 0;
> +
> +fail_vfio_setup:
> +fail_set_log_prefix:
Tags can be combined
> + rte_free(sva);
> +
> +fail_zmalloc:
> + return -1;
> +}
> +
> +static int
> +sfc_vdpa_pci_remove(struct rte_pci_device *pci_dev)
> +{
> + struct sfc_vdpa_adapter *sva = NULL;
> +
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> + return -1;
> +
> + sva = sfc_vdpa_get_adapter_by_dev(pci_dev);
> + if (sva == NULL) {
> + sfc_vdpa_info(sva, "invalid device: %s", pci_dev->name);
> + return -1;
> + }
> +
> + pthread_mutex_lock(&sfc_vdpa_adapter_list_lock);
> + TAILQ_REMOVE(&sfc_vdpa_adapter_list, sva, next);
> + pthread_mutex_unlock(&sfc_vdpa_adapter_list_lock);
> +
> + sfc_vdpa_vfio_teardown(sva);
> +
> + rte_free(sva);
> +
> + return 0;
> +}
> +
> +static struct rte_pci_driver rte_sfc_vdpa = {
> + .id_table = pci_id_sfc_vdpa_efx_map,
> + .drv_flags = 0,
> + .probe = sfc_vdpa_pci_probe,
> + .remove = sfc_vdpa_pci_remove,
> +};
> +
> +RTE_PMD_REGISTER_PCI(net_sfc_vdpa, rte_sfc_vdpa);
> +RTE_PMD_REGISTER_PCI_TABLE(net_sfc_vdpa, pci_id_sfc_vdpa_efx_map);
> +RTE_PMD_REGISTER_KMOD_DEP(net_sfc_vdpa, "* vfio-pci");
> +RTE_LOG_REGISTER_SUFFIX(sfc_vdpa_logtype_driver, driver, NOTICE);
> diff --git a/drivers/vdpa/sfc/sfc_vdpa.h b/drivers/vdpa/sfc/sfc_vdpa.h
> new file mode 100644
> index 0000000..3b77900
> --- /dev/null
> +++ b/drivers/vdpa/sfc/sfc_vdpa.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + *
> + * Copyright(c) 2020-2021 Xilinx, Inc.
> + */
> +
> +#ifndef _SFC_VDPA_H
> +#define _SFC_VDPA_H
> +
> +#include <stdint.h>
> +#include <sys/queue.h>
> +
> +#include <rte_bus_pci.h>
> +
> +#include "sfc_vdpa_log.h"
> +
> +/* Adapter private data */
> +struct sfc_vdpa_adapter {
> + TAILQ_ENTRY(sfc_vdpa_adapter) next;
> + struct rte_pci_device *pdev;
> + struct rte_pci_addr pci_addr;
> +
> + char log_prefix[SFC_VDPA_LOG_PREFIX_MAX];
> + uint32_t logtype_main;
> +
> + int vfio_group_fd;
> + int vfio_dev_fd;
> + int vfio_container_fd;
> + int iommu_group_num;
> +};
> +
> +uint32_t
> +sfc_vdpa_register_logtype(const struct rte_pci_addr *pci_addr,
> + const char *lt_prefix_str,
> + uint32_t ll_default);
> +
> +struct sfc_vdpa_adapter *
> +sfc_vdpa_get_adapter_by_dev(struct rte_pci_device *pdev);
> +
> +#endif /* _SFC_VDPA_H */
> +
> diff --git a/drivers/vdpa/sfc/sfc_vdpa_log.h b/drivers/vdpa/sfc/sfc_vdpa_log.h
> new file mode 100644
> index 0000000..0a3d6ad
> --- /dev/null
> +++ b/drivers/vdpa/sfc/sfc_vdpa_log.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + *
> + * Copyright(c) 2020-2021 Xilinx, Inc.
> + */
> +
> +#ifndef _SFC_VDPA_LOG_H_
> +#define _SFC_VDPA_LOG_H_
> +
> +/** Generic driver log type */
> +extern int sfc_vdpa_logtype_driver;
> +
> +/** Common log type name prefix */
> +#define SFC_VDPA_LOGTYPE_PREFIX "pmd.vdpa.sfc."
> +
> +/** Log PMD generic message, add a prefix and a line break */
> +#define SFC_VDPA_GENERIC_LOG(level, ...) \
> + rte_log(RTE_LOG_ ## level, sfc_vdpa_logtype_driver, \
> + RTE_FMT("PMD: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
> +
> +/** Name prefix for the per-device log type used to report basic information
> */
> +#define SFC_VDPA_LOGTYPE_MAIN_STR SFC_VDPA_LOGTYPE_PREFIX "main"
> +
> +#define SFC_VDPA_LOG_PREFIX_MAX 32
> +
> +/* Log PMD message, automatically add prefix and \n */
> +#define SFC_VDPA_LOG(sva, level, type, ...) \
> + rte_log(level, type, \
> + RTE_FMT("%s" RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
> + sva->log_prefix, \
> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
> +
> +#define sfc_vdpa_err(sva, ...) \
> + do { \
> + const struct sfc_vdpa_adapter *_sva = (sva); \
> + \
> + SFC_VDPA_LOG(_sva, RTE_LOG_ERR, \
> + _sva->logtype_main, __VA_ARGS__); \
> + } while (0)
> +
> +#define sfc_vdpa_warn(sva, ...) \
> + do { \
> + const struct sfc_vdpa_adapter *_sva = (sva); \
> + \
> + SFC_VDPA_LOG(_sva, RTE_LOG_WARNING, \
> + _sva->logtype_main, __VA_ARGS__); \
> + } while (0)
> +
> +#define sfc_vdpa_notice(sva, ...) \
> + do { \
> + const struct sfc_vdpa_adapter *_sva = (sva); \
> + \
> + SFC_VDPA_LOG(_sva, RTE_LOG_NOTICE, \
> + _sva->logtype_main, __VA_ARGS__); \
> + } while (0)
> +
> +#define sfc_vdpa_info(sva, ...) \
> + do { \
> + const struct sfc_vdpa_adapter *_sva = (sva); \
> + \
> + SFC_VDPA_LOG(_sva, RTE_LOG_INFO, \
> + _sva->logtype_main, __VA_ARGS__); \
> + } while (0)
> +
For above log, can't we make log level a parameter?
Then above four define can be one.
Thanks,
Chenbo
> +#define sfc_vdpa_log_init(sva, ...) \
> + do { \
> + const struct sfc_vdpa_adapter *_sva = (sva); \
> + \
> + SFC_VDPA_LOG(_sva, RTE_LOG_INFO, \
> + _sva->logtype_main, \
> + RTE_FMT("%s(): " \
> + RTE_FMT_HEAD(__VA_ARGS__ ,), \
> + __func__, \
> + RTE_FMT_TAIL(__VA_ARGS__ ,))); \
> + } while (0)
> +
> +#endif /* _SFC_VDPA_LOG_H_ */
> diff --git a/drivers/vdpa/sfc/version.map b/drivers/vdpa/sfc/version.map
> new file mode 100644
> index 0000000..4a76d1d
> --- /dev/null
> +++ b/drivers/vdpa/sfc/version.map
> @@ -0,0 +1,3 @@
> +DPDK_21 {
> + local: *;
> +};
> --
> 1.8.3.1
Hi Chenbo,
many thanks for review. See few questions/notes below.
On 8/11/21 5:26 AM, Xia, Chenbo wrote:
> Hi Vijay,
>
>> -----Original Message-----
>> From: Vijay Srivastava <vijay.srivastava@xilinx.com>
>> Sent: Wednesday, July 7, 2021 12:44 AM
>> To: dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
>> andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava <vsrivast@xilinx.com>
>> Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>>
>> From: Vijay Kumar Srivastava <vsrivast@xilinx.com>
>>
>> Add new vDPA PMD to support vDPA operation by Xilinx devices.
>
> vDPA operations of Xilinx devices
>
>> This patch implements probe and remove functions.
>>
>> Signed-off-by: Vijay Kumar Srivastava <vsrivast@xilinx.com>
[snip]
>> diff --git a/drivers/vdpa/sfc/sfc_vdpa.c b/drivers/vdpa/sfc/sfc_vdpa.c
>> new file mode 100644
>> index 0000000..d8faaca
>> --- /dev/null
>> +++ b/drivers/vdpa/sfc/sfc_vdpa.c
>> @@ -0,0 +1,286 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + *
>> + * Copyright(c) 2020-2021 Xilinx, Inc.
>> + */
>> +
>> +#include <stdbool.h>
>> +#include <stdint.h>
>> +#include <sys/queue.h>
>> +
>> +#include <rte_common.h>
>> +#include <rte_errno.h>
>> +#include <rte_string_fns.h>
>> +#include <rte_vfio.h>
>> +#include <rte_vhost.h>
>> +
>> +#include "efx.h"
>> +#include "sfc_efx.h"
>> +#include "sfc_vdpa.h"
>> +
>> +TAILQ_HEAD(sfc_vdpa_adapter_list_head, sfc_vdpa_adapter);
>> +static struct sfc_vdpa_adapter_list_head sfc_vdpa_adapter_list =
>> + TAILQ_HEAD_INITIALIZER(sfc_vdpa_adapter_list);
>> +
>> +static pthread_mutex_t sfc_vdpa_adapter_list_lock = PTHREAD_MUTEX_INITIALIZER;
>> +
>> +struct sfc_vdpa_adapter *
>> +sfc_vdpa_get_adapter_by_dev(struct rte_pci_device *pdev)
>> +{
>> + bool found = false;
>> + struct sfc_vdpa_adapter *sva;
>> +
>> + pthread_mutex_lock(&sfc_vdpa_adapter_list_lock);
>> +
>> + TAILQ_FOREACH(sva, &sfc_vdpa_adapter_list, next) {
>> + if (pdev == sva->pdev) {
>> + found = true;
>> + break;
>> + }
>> + }
>> +
>> + pthread_mutex_unlock(&sfc_vdpa_adapter_list_lock);
>> +
>> + return found ? sva : NULL;
>> +}
>> +
>> +static int
>> +sfc_vdpa_vfio_setup(struct sfc_vdpa_adapter *sva)
>> +{
>> + struct rte_pci_device *dev = sva->pdev;
>> + char dev_name[RTE_DEV_NAME_MAX_LEN] = {0};
>> + int rc;
>> +
>> + if (dev == NULL)
>> + goto fail_inval;
>> +
>> + rte_pci_device_name(&dev->addr, dev_name, RTE_DEV_NAME_MAX_LEN);
>> +
>> + sva->vfio_container_fd = rte_vfio_container_create();
>> + if (sva->vfio_container_fd < 0) {
>> + sfc_vdpa_err(sva, "failed to create VFIO container");
>
> failed -> Failed
>
> I suggest to use capital letter for first letter of every log info.
> Please check other logs.
Why? As far as I know it is not defined. It would make sense if
it is really a start of the log message, sfc_vdpa_* log
messages start from prefix (see macros definition).
>
>> + goto fail_container_create;
>> + }
>> +
>> + rc = rte_vfio_get_group_num(rte_pci_get_sysfs_path(), dev_name,
>> + &sva->iommu_group_num);
>> + if (rc <= 0) {
>> + sfc_vdpa_err(sva, "failed to get IOMMU group for %s : %s",
>> + dev_name, rte_strerror(-rc));
>> + goto fail_get_group_num;
>> + }
>> +
>> + sva->vfio_group_fd =
>> + rte_vfio_container_group_bind(sva->vfio_container_fd,
>> + sva->iommu_group_num);
>> + if (sva->vfio_group_fd < 0) {
>> + sfc_vdpa_err(sva,
>> + "failed to bind IOMMU group %d to container %d",
>> + sva->iommu_group_num, sva->vfio_container_fd);
>> + goto fail_group_bind;
>> + }
>> +
>> + if (rte_pci_map_device(dev) != 0) {
>> + sfc_vdpa_err(sva, "failed to map PCI device %s : %s",
>> + dev_name, rte_strerror(rte_errno));
>> + goto fail_pci_map_device;
>> + }
>> +
>> + sva->vfio_dev_fd = dev->intr_handle.vfio_dev_fd;
>> +
>> + return 0;
>> +
>> +fail_pci_map_device:
>> + if (rte_vfio_container_group_unbind(sva->vfio_container_fd,
>> + sva->iommu_group_num) != 0) {
>> + sfc_vdpa_err(sva,
>> + "failed to unbind IOMMU group %d from container %d",
>> + sva->iommu_group_num, sva->vfio_container_fd);
>> + }
>> +
>> +fail_group_bind:
>> +fail_get_group_num:
>
> You can combined these two tags into one with tag name changed.
I think the original code is better. There is no point to
combine. This way code is safer against future changes
between these goto's which could require cleanup.
[snip]
>> diff --git a/drivers/vdpa/sfc/sfc_vdpa_log.h b/drivers/vdpa/sfc/sfc_vdpa_log.h
>> new file mode 100644
>> index 0000000..0a3d6ad
>> --- /dev/null
>> +++ b/drivers/vdpa/sfc/sfc_vdpa_log.h
>> @@ -0,0 +1,77 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + *
>> + * Copyright(c) 2020-2021 Xilinx, Inc.
>> + */
>> +
>> +#ifndef _SFC_VDPA_LOG_H_
>> +#define _SFC_VDPA_LOG_H_
>> +
>> +/** Generic driver log type */
>> +extern int sfc_vdpa_logtype_driver;
>> +
>> +/** Common log type name prefix */
>> +#define SFC_VDPA_LOGTYPE_PREFIX "pmd.vdpa.sfc."
>> +
>> +/** Log PMD generic message, add a prefix and a line break */
>> +#define SFC_VDPA_GENERIC_LOG(level, ...) \
>> + rte_log(RTE_LOG_ ## level, sfc_vdpa_logtype_driver, \
>> + RTE_FMT("PMD: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
>> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
>> +
>> +/** Name prefix for the per-device log type used to report basic information
>> */
>> +#define SFC_VDPA_LOGTYPE_MAIN_STR SFC_VDPA_LOGTYPE_PREFIX "main"
>> +
>> +#define SFC_VDPA_LOG_PREFIX_MAX 32
>> +
>> +/* Log PMD message, automatically add prefix and \n */
>> +#define SFC_VDPA_LOG(sva, level, type, ...) \
>> + rte_log(level, type, \
>> + RTE_FMT("%s" RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
>> + sva->log_prefix, \
>> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
>> +
>> +#define sfc_vdpa_err(sva, ...) \
>> + do { \
>> + const struct sfc_vdpa_adapter *_sva = (sva); \
>> + \
>> + SFC_VDPA_LOG(_sva, RTE_LOG_ERR, \
>> + _sva->logtype_main, __VA_ARGS__); \
>> + } while (0)
>> +
>> +#define sfc_vdpa_warn(sva, ...) \
>> + do { \
>> + const struct sfc_vdpa_adapter *_sva = (sva); \
>> + \
>> + SFC_VDPA_LOG(_sva, RTE_LOG_WARNING, \
>> + _sva->logtype_main, __VA_ARGS__); \
>> + } while (0)
>> +
>> +#define sfc_vdpa_notice(sva, ...) \
>> + do { \
>> + const struct sfc_vdpa_adapter *_sva = (sva); \
>> + \
>> + SFC_VDPA_LOG(_sva, RTE_LOG_NOTICE, \
>> + _sva->logtype_main, __VA_ARGS__); \
>> + } while (0)
>> +
>> +#define sfc_vdpa_info(sva, ...) \
>> + do { \
>> + const struct sfc_vdpa_adapter *_sva = (sva); \
>> + \
>> + SFC_VDPA_LOG(_sva, RTE_LOG_INFO, \
>> + _sva->logtype_main, __VA_ARGS__); \
>> + } while (0)
>> +
>
> For above log, can't we make log level a parameter?
> Then above four define can be one.
Yes, it definitely could, but it is more convenient to have
dedicated macros for different log levels and corresponding
lines shorter this way.
Andrew.
Hi Andrew,
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, August 13, 2021 4:39 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; Vijay Srivastava
> <vijay.srivastava@xilinx.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Vijay Kumar Srivastava <vsrivast@xilinx.com>
> Subject: Re: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>
> Hi Chenbo,
>
> many thanks for review. See few questions/notes below.
>
> On 8/11/21 5:26 AM, Xia, Chenbo wrote:
> > Hi Vijay,
> >
> >> -----Original Message-----
> >> From: Vijay Srivastava <vijay.srivastava@xilinx.com>
> >> Sent: Wednesday, July 7, 2021 12:44 AM
> >> To: dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> >> andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava <vsrivast@xilinx.com>
> >> Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
> >>
> >> From: Vijay Kumar Srivastava <vsrivast@xilinx.com>
> >>
> >> Add new vDPA PMD to support vDPA operation by Xilinx devices.
> >
> > vDPA operations of Xilinx devices
> >
> >> This patch implements probe and remove functions.
> >>
> >> Signed-off-by: Vijay Kumar Srivastava <vsrivast@xilinx.com>
>
> [snip]
>
> >> diff --git a/drivers/vdpa/sfc/sfc_vdpa.c b/drivers/vdpa/sfc/sfc_vdpa.c
> >> new file mode 100644
> >> index 0000000..d8faaca
> >> --- /dev/null
> >> +++ b/drivers/vdpa/sfc/sfc_vdpa.c
> >> @@ -0,0 +1,286 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + *
> >> + * Copyright(c) 2020-2021 Xilinx, Inc.
> >> + */
> >> +
> >> +#include <stdbool.h>
> >> +#include <stdint.h>
> >> +#include <sys/queue.h>
> >> +
> >> +#include <rte_common.h>
> >> +#include <rte_errno.h>
> >> +#include <rte_string_fns.h>
> >> +#include <rte_vfio.h>
> >> +#include <rte_vhost.h>
> >> +
> >> +#include "efx.h"
> >> +#include "sfc_efx.h"
> >> +#include "sfc_vdpa.h"
> >> +
> >> +TAILQ_HEAD(sfc_vdpa_adapter_list_head, sfc_vdpa_adapter);
> >> +static struct sfc_vdpa_adapter_list_head sfc_vdpa_adapter_list =
> >> + TAILQ_HEAD_INITIALIZER(sfc_vdpa_adapter_list);
> >> +
> >> +static pthread_mutex_t sfc_vdpa_adapter_list_lock =
> PTHREAD_MUTEX_INITIALIZER;
> >> +
> >> +struct sfc_vdpa_adapter *
> >> +sfc_vdpa_get_adapter_by_dev(struct rte_pci_device *pdev)
> >> +{
> >> + bool found = false;
> >> + struct sfc_vdpa_adapter *sva;
> >> +
> >> + pthread_mutex_lock(&sfc_vdpa_adapter_list_lock);
> >> +
> >> + TAILQ_FOREACH(sva, &sfc_vdpa_adapter_list, next) {
> >> + if (pdev == sva->pdev) {
> >> + found = true;
> >> + break;
> >> + }
> >> + }
> >> +
> >> + pthread_mutex_unlock(&sfc_vdpa_adapter_list_lock);
> >> +
> >> + return found ? sva : NULL;
> >> +}
> >> +
> >> +static int
> >> +sfc_vdpa_vfio_setup(struct sfc_vdpa_adapter *sva)
> >> +{
> >> + struct rte_pci_device *dev = sva->pdev;
> >> + char dev_name[RTE_DEV_NAME_MAX_LEN] = {0};
> >> + int rc;
> >> +
> >> + if (dev == NULL)
> >> + goto fail_inval;
> >> +
> >> + rte_pci_device_name(&dev->addr, dev_name, RTE_DEV_NAME_MAX_LEN);
> >> +
> >> + sva->vfio_container_fd = rte_vfio_container_create();
> >> + if (sva->vfio_container_fd < 0) {
> >> + sfc_vdpa_err(sva, "failed to create VFIO container");
> >
> > failed -> Failed
> >
> > I suggest to use capital letter for first letter of every log info.
> > Please check other logs.
>
> Why? As far as I know it is not defined. It would make sense if
> it is really a start of the log message, sfc_vdpa_* log
> messages start from prefix (see macros definition).
Forgot the prefix here ☹. Ignore the comment.
>
> >
> >> + goto fail_container_create;
> >> + }
> >> +
> >> + rc = rte_vfio_get_group_num(rte_pci_get_sysfs_path(), dev_name,
> >> + &sva->iommu_group_num);
> >> + if (rc <= 0) {
> >> + sfc_vdpa_err(sva, "failed to get IOMMU group for %s : %s",
> >> + dev_name, rte_strerror(-rc));
> >> + goto fail_get_group_num;
> >> + }
> >> +
> >> + sva->vfio_group_fd =
> >> + rte_vfio_container_group_bind(sva->vfio_container_fd,
> >> + sva->iommu_group_num);
> >> + if (sva->vfio_group_fd < 0) {
> >> + sfc_vdpa_err(sva,
> >> + "failed to bind IOMMU group %d to container %d",
> >> + sva->iommu_group_num, sva->vfio_container_fd);
> >> + goto fail_group_bind;
> >> + }
> >> +
> >> + if (rte_pci_map_device(dev) != 0) {
> >> + sfc_vdpa_err(sva, "failed to map PCI device %s : %s",
> >> + dev_name, rte_strerror(rte_errno));
> >> + goto fail_pci_map_device;
> >> + }
> >> +
> >> + sva->vfio_dev_fd = dev->intr_handle.vfio_dev_fd;
> >> +
> >> + return 0;
> >> +
> >> +fail_pci_map_device:
> >> + if (rte_vfio_container_group_unbind(sva->vfio_container_fd,
> >> + sva->iommu_group_num) != 0) {
> >> + sfc_vdpa_err(sva,
> >> + "failed to unbind IOMMU group %d from container %d",
> >> + sva->iommu_group_num, sva->vfio_container_fd);
> >> + }
> >> +
> >> +fail_group_bind:
> >> +fail_get_group_num:
> >
> > You can combined these two tags into one with tag name changed.
>
> I think the original code is better. There is no point to
> combine. This way code is safer against future changes
> between these goto's which could require cleanup.
Personally I don't think it's a problem for developer if the tag name is
well chosen. I would prefer a single tag but have no strong opinion since
there's no policy of it.
>
> [snip]
>
> >> diff --git a/drivers/vdpa/sfc/sfc_vdpa_log.h
> b/drivers/vdpa/sfc/sfc_vdpa_log.h
> >> new file mode 100644
> >> index 0000000..0a3d6ad
> >> --- /dev/null
> >> +++ b/drivers/vdpa/sfc/sfc_vdpa_log.h
> >> @@ -0,0 +1,77 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + *
> >> + * Copyright(c) 2020-2021 Xilinx, Inc.
> >> + */
> >> +
> >> +#ifndef _SFC_VDPA_LOG_H_
> >> +#define _SFC_VDPA_LOG_H_
> >> +
> >> +/** Generic driver log type */
> >> +extern int sfc_vdpa_logtype_driver;
> >> +
> >> +/** Common log type name prefix */
> >> +#define SFC_VDPA_LOGTYPE_PREFIX "pmd.vdpa.sfc."
> >> +
> >> +/** Log PMD generic message, add a prefix and a line break */
> >> +#define SFC_VDPA_GENERIC_LOG(level, ...) \
> >> + rte_log(RTE_LOG_ ## level, sfc_vdpa_logtype_driver, \
> >> + RTE_FMT("PMD: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
> >> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
> >> +
> >> +/** Name prefix for the per-device log type used to report basic
> information
> >> */
> >> +#define SFC_VDPA_LOGTYPE_MAIN_STR SFC_VDPA_LOGTYPE_PREFIX "main"
> >> +
> >> +#define SFC_VDPA_LOG_PREFIX_MAX 32
> >> +
> >> +/* Log PMD message, automatically add prefix and \n */
> >> +#define SFC_VDPA_LOG(sva, level, type, ...) \
> >> + rte_log(level, type, \
> >> + RTE_FMT("%s" RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
> >> + sva->log_prefix, \
> >> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
> >> +
> >> +#define sfc_vdpa_err(sva, ...) \
> >> + do { \
> >> + const struct sfc_vdpa_adapter *_sva = (sva); \
> >> + \
> >> + SFC_VDPA_LOG(_sva, RTE_LOG_ERR, \
> >> + _sva->logtype_main, __VA_ARGS__); \
> >> + } while (0)
> >> +
> >> +#define sfc_vdpa_warn(sva, ...) \
> >> + do { \
> >> + const struct sfc_vdpa_adapter *_sva = (sva); \
> >> + \
> >> + SFC_VDPA_LOG(_sva, RTE_LOG_WARNING, \
> >> + _sva->logtype_main, __VA_ARGS__); \
> >> + } while (0)
> >> +
> >> +#define sfc_vdpa_notice(sva, ...) \
> >> + do { \
> >> + const struct sfc_vdpa_adapter *_sva = (sva); \
> >> + \
> >> + SFC_VDPA_LOG(_sva, RTE_LOG_NOTICE, \
> >> + _sva->logtype_main, __VA_ARGS__); \
> >> + } while (0)
> >> +
> >> +#define sfc_vdpa_info(sva, ...) \
> >> + do { \
> >> + const struct sfc_vdpa_adapter *_sva = (sva); \
> >> + \
> >> + SFC_VDPA_LOG(_sva, RTE_LOG_INFO, \
> >> + _sva->logtype_main, __VA_ARGS__); \
> >> + } while (0)
> >> +
> >
> > For above log, can't we make log level a parameter?
> > Then above four define can be one.
>
> Yes, it definitely could, but it is more convenient to have
> dedicated macros for different log levels and corresponding
> lines shorter this way.
It could save some chars in one log line but also introduce more
LOC. And you may have to change every macro if things like SFC_VDPA_LOG
or naming of sfc_vdpa_adapter change. I prefer combining but since
the duplication is acceptable, I'll let you balance the pros/cons.
Thanks,
Chenbo
>
> Andrew.
On 8/13/21 12:23 PM, Xia, Chenbo wrote:
> Hi Andrew,
>
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Friday, August 13, 2021 4:39 PM
>> To: Xia, Chenbo <chenbo.xia@intel.com>; Vijay Srivastava
>> <vijay.srivastava@xilinx.com>; dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Vijay Kumar Srivastava <vsrivast@xilinx.com>
>> Subject: Re: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>>
>> Hi Chenbo,
>>
>> many thanks for review. See few questions/notes below.
>>
>> On 8/11/21 5:26 AM, Xia, Chenbo wrote:
>>> Hi Vijay,
>>>
>>>> -----Original Message-----
>>>> From: Vijay Srivastava <vijay.srivastava@xilinx.com>
>>>> Sent: Wednesday, July 7, 2021 12:44 AM
>>>> To: dev@dpdk.org
>>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
>>>> andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava <vsrivast@xilinx.com>
>>>> Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>>>>
>>>> From: Vijay Kumar Srivastava <vsrivast@xilinx.com>
>>>>
>>>> Add new vDPA PMD to support vDPA operation by Xilinx devices.
>>>
>>> vDPA operations of Xilinx devices
>>>
>>>> This patch implements probe and remove functions.
>>>>
>>>> Signed-off-by: Vijay Kumar Srivastava <vsrivast@xilinx.com>
[snip]
>>>> diff --git a/drivers/vdpa/sfc/sfc_vdpa_log.h
>> b/drivers/vdpa/sfc/sfc_vdpa_log.h
>>>> new file mode 100644
>>>> index 0000000..0a3d6ad
>>>> --- /dev/null
>>>> +++ b/drivers/vdpa/sfc/sfc_vdpa_log.h
>>>> @@ -0,0 +1,77 @@
>>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>>> + *
>>>> + * Copyright(c) 2020-2021 Xilinx, Inc.
>>>> + */
>>>> +
>>>> +#ifndef _SFC_VDPA_LOG_H_
>>>> +#define _SFC_VDPA_LOG_H_
>>>> +
>>>> +/** Generic driver log type */
>>>> +extern int sfc_vdpa_logtype_driver;
>>>> +
>>>> +/** Common log type name prefix */
>>>> +#define SFC_VDPA_LOGTYPE_PREFIX "pmd.vdpa.sfc."
>>>> +
>>>> +/** Log PMD generic message, add a prefix and a line break */
>>>> +#define SFC_VDPA_GENERIC_LOG(level, ...) \
>>>> + rte_log(RTE_LOG_ ## level, sfc_vdpa_logtype_driver, \
>>>> + RTE_FMT("PMD: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
>>>> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
>>>> +
>>>> +/** Name prefix for the per-device log type used to report basic
>> information
>>>> */
>>>> +#define SFC_VDPA_LOGTYPE_MAIN_STR SFC_VDPA_LOGTYPE_PREFIX "main"
>>>> +
>>>> +#define SFC_VDPA_LOG_PREFIX_MAX 32
>>>> +
>>>> +/* Log PMD message, automatically add prefix and \n */
>>>> +#define SFC_VDPA_LOG(sva, level, type, ...) \
>>>> + rte_log(level, type, \
>>>> + RTE_FMT("%s" RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
>>>> + sva->log_prefix, \
>>>> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
>>>> +
>>>> +#define sfc_vdpa_err(sva, ...) \
>>>> + do { \
>>>> + const struct sfc_vdpa_adapter *_sva = (sva); \
>>>> + \
>>>> + SFC_VDPA_LOG(_sva, RTE_LOG_ERR, \
>>>> + _sva->logtype_main, __VA_ARGS__); \
>>>> + } while (0)
>>>> +
>>>> +#define sfc_vdpa_warn(sva, ...) \
>>>> + do { \
>>>> + const struct sfc_vdpa_adapter *_sva = (sva); \
>>>> + \
>>>> + SFC_VDPA_LOG(_sva, RTE_LOG_WARNING, \
>>>> + _sva->logtype_main, __VA_ARGS__); \
>>>> + } while (0)
>>>> +
>>>> +#define sfc_vdpa_notice(sva, ...) \
>>>> + do { \
>>>> + const struct sfc_vdpa_adapter *_sva = (sva); \
>>>> + \
>>>> + SFC_VDPA_LOG(_sva, RTE_LOG_NOTICE, \
>>>> + _sva->logtype_main, __VA_ARGS__); \
>>>> + } while (0)
>>>> +
>>>> +#define sfc_vdpa_info(sva, ...) \
>>>> + do { \
>>>> + const struct sfc_vdpa_adapter *_sva = (sva); \
>>>> + \
>>>> + SFC_VDPA_LOG(_sva, RTE_LOG_INFO, \
>>>> + _sva->logtype_main, __VA_ARGS__); \
>>>> + } while (0)
>>>> +
>>>
>>> For above log, can't we make log level a parameter?
>>> Then above four define can be one.
>>
>> Yes, it definitely could, but it is more convenient to have
>> dedicated macros for different log levels and corresponding
>> lines shorter this way.
>
> It could save some chars in one log line but also introduce more
> LOC. And you may have to change every macro if things like SFC_VDPA_LOG
> or naming of sfc_vdpa_adapter change. I prefer combining but since
> the duplication is acceptable, I'll let you balance the pros/cons.
I see your point. I think it should be a macro sfc_vdpa_log()
with log level and other sfc_vpda_*() macros should be just one
liner like sfc_vdpa_log(sva, RTE_LOG_INFO, __VA_ARGS__)
Do I understand correctly that it address your concerns?
Andrew.
On Tue, 6 Jul 2021 22:14:09 +0530
Vijay Srivastava <vijay.srivastava@xilinx.com> wrote:
> +# Enable more warnings
> +extra_flags += [
> + '-Wdisabled-optimization'
> +]
> +
> +# Compiler and version dependent flags
> +extra_flags += [
> + '-Waggregate-return',
> + '-Wbad-function-cast'
> +]
> +
Having each driver decide on different compiler flags seems like
a bad policy in DPDK.
On Tue, 6 Jul 2021 22:14:09 +0530
Vijay Srivastava <vijay.srivastava@xilinx.com> wrote:
> +uint32_t
> +sfc_vdpa_register_logtype(const struct rte_pci_addr *pci_addr,
> + const char *lt_prefix_str, uint32_t ll_default)
> +{
> + size_t lt_prefix_str_size = strlen(lt_prefix_str);
> + size_t lt_str_size_max;
> + char *lt_str = NULL;
> + int ret;
> +
> + if (SIZE_MAX - PCI_PRI_STR_SIZE - 1 > lt_prefix_str_size) {
> + ++lt_prefix_str_size; /* Reserve space for prefix separator */
> + lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
> + } else {
> + return RTE_LOGTYPE_PMD;
> + }
> +
> + lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
> + if (lt_str == NULL)
> + return RTE_LOGTYPE_PMD;
> +
> + strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
> + lt_str[lt_prefix_str_size - 1] = '.';
> + rte_pci_device_name(pci_addr, lt_str + lt_prefix_str_size,
> + lt_str_size_max - lt_prefix_str_size);
> + lt_str[lt_str_size_max - 1] = '\0';
> +
> + ret = rte_log_register_type_and_pick_level(lt_str, ll_default);
> + rte_free(lt_str);
> +
> + return (ret < 0) ? RTE_LOGTYPE_PMD : ret;
> +}
This seems like overkill doing per-device log level. Other drivers
aren't doing this.
Once again my advice to DPDK driver developers is "don't be a snowflake"
your driver is not special.
On Tue, 6 Jul 2021 22:14:09 +0530
Vijay Srivastava <vijay.srivastava@xilinx.com> wrote:
> +static pthread_mutex_t sfc_vdpa_adapter_list_lock = PTHREAD_MUTEX_INITIALIZER;
> +
Why do you need a pthread_mutex when simple DPDK spin lock will do?
Hi Andrew,
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Friday, August 13, 2021 5:31 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; Vijay Srivastava
> <vijay.srivastava@xilinx.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Vijay Kumar Srivastava <vsrivast@xilinx.com>
> Subject: Re: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>
> On 8/13/21 12:23 PM, Xia, Chenbo wrote:
> > Hi Andrew,
> >
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> >> Sent: Friday, August 13, 2021 4:39 PM
> >> To: Xia, Chenbo <chenbo.xia@intel.com>; Vijay Srivastava
> >> <vijay.srivastava@xilinx.com>; dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Vijay Kumar Srivastava <vsrivast@xilinx.com>
> >> Subject: Re: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
> >>
> >> Hi Chenbo,
> >>
> >> many thanks for review. See few questions/notes below.
> >>
> >> On 8/11/21 5:26 AM, Xia, Chenbo wrote:
> >>> Hi Vijay,
> >>>
> >>>> -----Original Message-----
> >>>> From: Vijay Srivastava <vijay.srivastava@xilinx.com>
> >>>> Sent: Wednesday, July 7, 2021 12:44 AM
> >>>> To: dev@dpdk.org
> >>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> >>>> andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava
> <vsrivast@xilinx.com>
> >>>> Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
> >>>>
> >>>> From: Vijay Kumar Srivastava <vsrivast@xilinx.com>
> >>>>
> >>>> Add new vDPA PMD to support vDPA operation by Xilinx devices.
> >>>
> >>> vDPA operations of Xilinx devices
> >>>
> >>>> This patch implements probe and remove functions.
> >>>>
> >>>> Signed-off-by: Vijay Kumar Srivastava <vsrivast@xilinx.com>
>
> [snip]
>
> >>>> diff --git a/drivers/vdpa/sfc/sfc_vdpa_log.h
> >> b/drivers/vdpa/sfc/sfc_vdpa_log.h
> >>>> new file mode 100644
> >>>> index 0000000..0a3d6ad
> >>>> --- /dev/null
> >>>> +++ b/drivers/vdpa/sfc/sfc_vdpa_log.h
> >>>> @@ -0,0 +1,77 @@
> >>>> +/* SPDX-License-Identifier: BSD-3-Clause
> >>>> + *
> >>>> + * Copyright(c) 2020-2021 Xilinx, Inc.
> >>>> + */
> >>>> +
> >>>> +#ifndef _SFC_VDPA_LOG_H_
> >>>> +#define _SFC_VDPA_LOG_H_
> >>>> +
> >>>> +/** Generic driver log type */
> >>>> +extern int sfc_vdpa_logtype_driver;
> >>>> +
> >>>> +/** Common log type name prefix */
> >>>> +#define SFC_VDPA_LOGTYPE_PREFIX "pmd.vdpa.sfc."
> >>>> +
> >>>> +/** Log PMD generic message, add a prefix and a line break */
> >>>> +#define SFC_VDPA_GENERIC_LOG(level, ...) \
> >>>> + rte_log(RTE_LOG_ ## level, sfc_vdpa_logtype_driver, \
> >>>> + RTE_FMT("PMD: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
> >>>> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
> >>>> +
> >>>> +/** Name prefix for the per-device log type used to report basic
> >> information
> >>>> */
> >>>> +#define SFC_VDPA_LOGTYPE_MAIN_STR SFC_VDPA_LOGTYPE_PREFIX "main"
> >>>> +
> >>>> +#define SFC_VDPA_LOG_PREFIX_MAX 32
> >>>> +
> >>>> +/* Log PMD message, automatically add prefix and \n */
> >>>> +#define SFC_VDPA_LOG(sva, level, type, ...) \
> >>>> + rte_log(level, type, \
> >>>> + RTE_FMT("%s" RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
> >>>> + sva->log_prefix, \
> >>>> + RTE_FMT_TAIL(__VA_ARGS__ ,)))
> >>>> +
> >>>> +#define sfc_vdpa_err(sva, ...) \
> >>>> + do { \
> >>>> + const struct sfc_vdpa_adapter *_sva = (sva); \
> >>>> + \
> >>>> + SFC_VDPA_LOG(_sva, RTE_LOG_ERR, \
> >>>> + _sva->logtype_main, __VA_ARGS__); \
> >>>> + } while (0)
> >>>> +
> >>>> +#define sfc_vdpa_warn(sva, ...) \
> >>>> + do { \
> >>>> + const struct sfc_vdpa_adapter *_sva = (sva); \
> >>>> + \
> >>>> + SFC_VDPA_LOG(_sva, RTE_LOG_WARNING, \
> >>>> + _sva->logtype_main, __VA_ARGS__); \
> >>>> + } while (0)
> >>>> +
> >>>> +#define sfc_vdpa_notice(sva, ...) \
> >>>> + do { \
> >>>> + const struct sfc_vdpa_adapter *_sva = (sva); \
> >>>> + \
> >>>> + SFC_VDPA_LOG(_sva, RTE_LOG_NOTICE, \
> >>>> + _sva->logtype_main, __VA_ARGS__); \
> >>>> + } while (0)
> >>>> +
> >>>> +#define sfc_vdpa_info(sva, ...) \
> >>>> + do { \
> >>>> + const struct sfc_vdpa_adapter *_sva = (sva); \
> >>>> + \
> >>>> + SFC_VDPA_LOG(_sva, RTE_LOG_INFO, \
> >>>> + _sva->logtype_main, __VA_ARGS__); \
> >>>> + } while (0)
> >>>> +
> >>>
> >>> For above log, can't we make log level a parameter?
> >>> Then above four define can be one.
> >>
> >> Yes, it definitely could, but it is more convenient to have
> >> dedicated macros for different log levels and corresponding
> >> lines shorter this way.
> >
> > It could save some chars in one log line but also introduce more
> > LOC. And you may have to change every macro if things like SFC_VDPA_LOG
> > or naming of sfc_vdpa_adapter change. I prefer combining but since
> > the duplication is acceptable, I'll let you balance the pros/cons.
>
> I see your point. I think it should be a macro sfc_vdpa_log()
> with log level and other sfc_vpda_*() macros should be just one
> liner like sfc_vdpa_log(sva, RTE_LOG_INFO, __VA_ARGS__)
>
> Do I understand correctly that it address your concerns?
Exactly! I'd like this and you can hide RTE_LOG prefix in macro definition
, so it'll be like:
sfc_vdpa_log(sva, INFO, ...)
Thanks,
Chenbo
>
> Andrew.
>
Hi Stephen,
>-----Original Message-----
>From: Stephen Hemminger <stephen@networkplumber.org>
>Sent: Friday, August 13, 2021 9:07 PM
>To: Vijay Kumar Srivastava <vsrivast@xilinx.com>
>Cc: dev@dpdk.org; maxime.coquelin@redhat.com; chenbo.xia@intel.com;
>andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava <vsrivast@xilinx.com>
>Subject: Re: [dpdk-dev] [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>
>On Tue, 6 Jul 2021 22:14:09 +0530
>Vijay Srivastava <vijay.srivastava@xilinx.com> wrote:
>
>> +static pthread_mutex_t sfc_vdpa_adapter_list_lock =
>PTHREAD_MUTEX_INITIALIZER;
>> +
>
>Why do you need a pthread_mutex when simple DPDK spin lock will do?
rte_spinlock_lock is being used in the subsequent patches.
Only for the adapter list (sfc_vdpa_adapter_list) pthread_mutex is being used.
Regards,
Vijay
Hi Stephen,
>-----Original Message-----
>From: Stephen Hemminger <stephen@networkplumber.org>
>Sent: Friday, August 13, 2021 9:06 PM
>To: Vijay Kumar Srivastava <vsrivast@xilinx.com>
>Cc: dev@dpdk.org; maxime.coquelin@redhat.com; chenbo.xia@intel.com;
>andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava <vsrivast@xilinx.com>
>Subject: Re: [dpdk-dev] [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>
>On Tue, 6 Jul 2021 22:14:09 +0530
>Vijay Srivastava <vijay.srivastava@xilinx.com> wrote:
>
>> +uint32_t
>> +sfc_vdpa_register_logtype(const struct rte_pci_addr *pci_addr,
>> + const char *lt_prefix_str, uint32_t ll_default) {
>> + size_t lt_prefix_str_size = strlen(lt_prefix_str);
>> + size_t lt_str_size_max;
>> + char *lt_str = NULL;
>> + int ret;
>> +
>> + if (SIZE_MAX - PCI_PRI_STR_SIZE - 1 > lt_prefix_str_size) {
>> + ++lt_prefix_str_size; /* Reserve space for prefix separator */
>> + lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
>> + } else {
>> + return RTE_LOGTYPE_PMD;
>> + }
>> +
>> + lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
>> + if (lt_str == NULL)
>> + return RTE_LOGTYPE_PMD;
>> +
>> + strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
>> + lt_str[lt_prefix_str_size - 1] = '.';
>> + rte_pci_device_name(pci_addr, lt_str + lt_prefix_str_size,
>> + lt_str_size_max - lt_prefix_str_size);
>> + lt_str[lt_str_size_max - 1] = '\0';
>> +
>> + ret = rte_log_register_type_and_pick_level(lt_str, ll_default);
>> + rte_free(lt_str);
>> +
>> + return (ret < 0) ? RTE_LOGTYPE_PMD : ret; }
>
>This seems like overkill doing per-device log level. Other drivers aren't doing
>this.
We use it for the debugging. This feature is useful for us so it's good to have it.
@@ -1197,6 +1197,12 @@ F: drivers/vdpa/mlx5/
F: doc/guides/vdpadevs/mlx5.rst
F: doc/guides/vdpadevs/features/mlx5.ini
+Xilinx sfc vDPA
+M: Vijay Kumar Srivastava <vsrivast@xilinx.com>
+F: drivers/vdpa/sfc/
+F: doc/guides/vdpadevs/sfc.rst
+F: doc/guides/vdpadevs/features/sfc.ini
+
Eventdev Drivers
----------------
@@ -55,6 +55,11 @@ New Features
Also, make sure to start the actual text at the margin.
=======================================================
+* **Add new vDPA PMD based on Xilinx devices.**
+
+ Added a new Xilinx vDPA (``sfc_vdpa``) PMD.
+ See the :doc:`../vdpadevs/sfc` guide for more details on this driver.
+
Removed Items
-------------
new file mode 100644
@@ -0,0 +1,9 @@
+;
+; Supported features of the 'sfc' vDPA driver.
+;
+; Refer to default.ini for the full list of available driver features.
+;
+[Features]
+Linux = Y
+x86-64 = Y
+Usage doc = Y
new file mode 100644
@@ -0,0 +1,97 @@
+.. SPDX-License-Identifier: BSD-3-Clause
+ Copyright(c) 2021 Xilinx Corporation.
+
+Xilinx vDPA driver
+==================
+
+The Xilinx vDPA (vhost data path acceleration) driver (**librte_pmd_sfc_vdpa**)
+provides support for the Xilinx SN1022 SmartNICs family of 10/25/40/50/100 Gbps
+adapters has support for latest Linux and FreeBSD operating systems.
+
+More information can be found at Xilinx website https://www.xilinx.com.
+
+
+Xilinx vDPA implementation
+--------------------------
+
+ef100 device can be configured in the net device or vDPA mode.
+Adding "class=vdpa" parameter helps to specify that this
+device is to be used in vDPA mode. If this parameter is not specified, device
+will be probed by net/sfc driver and will used as a net device.
+
+This PMD uses libefx (common/sfc_efx) code to access the device firmware.
+
+
+Supported NICs
+--------------
+
+- Xilinx SN1022 SmartNICs
+
+
+Features
+--------
+
+Features of the Xilinx vDPA driver are:
+
+- Compatibility with virtio 0.95 and 1.0
+
+
+Non-supported Features
+----------------------
+
+- Control Queue
+- Multi queue
+- Live Migration
+
+
+Prerequisites
+-------------
+
+Requires firmware version: v1.0.7.0 or higher
+
+Visit `Xilinx Support Downloads <https://www.xilinx.com/support.html>`_
+to get Xilinx Utilities with the latest firmware.
+Follow instructions from Alveo SN1000 SmartNICs User Guide to
+update firmware and configure the adapter.
+
+
+Per-Device Parameters
+~~~~~~~~~~~~~~~~~~~~~
+
+The following per-device parameters can be passed via EAL PCI device
+whitelist option like "-w 02:00.0,arg1=value1,...".
+
+Case-insensitive 1/y/yes/on or 0/n/no/off may be used to specify
+boolean parameters value.
+
+- ``class`` [net|vdpa] (default **net**)
+
+ Choose the mode of operation of ef100 device.
+ **net** device will work as network device and will be probed by net/sfc driver.
+ **vdpa** device will work as vdpa device and will be probed by vdpa/sfc driver.
+ If this parameter is not specified then ef100 device will operate as network device.
+
+
+Dynamic Logging Parameters
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+One may leverage EAL option "--log-level" to change default levels
+for the log types supported by the driver. The option is used with
+an argument typically consisting of two parts separated by a colon.
+
+Level value is the last part which takes a symbolic name (or integer).
+Log type is the former part which may shell match syntax.
+Depending on the choice of the expression, the given log level may
+be used either for some specific log type or for a subset of types.
+
+SFC vDPA PMD provides the following log types available for control:
+
+- ``pmd.vdpa.sfc.driver`` (default level is **notice**)
+
+ Affects driver-wide messages unrelated to any particular devices.
+
+- ``pmd.vdpa.sfc.main`` (default level is **notice**)
+
+ Matches a subset of per-port log types registered during runtime.
+ A full name for a particular type may be obtained by appending a
+ dot and a PCI device identifier (``XXXX:XX:XX.X``) to the prefix.
@@ -8,6 +8,7 @@ endif
drivers = [
'ifc',
'mlx5',
+ 'sfc',
]
std_deps = ['bus_pci', 'kvargs']
std_deps += ['vhost']
new file mode 100644
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: BSD-3-Clause
+#
+# Copyright(c) 2020-2021 Xilinx, Inc.
+
+if (arch_subdir != 'x86' or not dpdk_conf.get('RTE_ARCH_64')) and (arch_subdir != 'arm' or not host_machine.cpu_family().startswith('aarch64'))
+ build = false
+ reason = 'only supported on x86_64 and aarch64'
+endif
+
+fmt_name = 'sfc_vdpa'
+extra_flags = []
+
+# Enable more warnings
+extra_flags += [
+ '-Wdisabled-optimization'
+]
+
+# Compiler and version dependent flags
+extra_flags += [
+ '-Waggregate-return',
+ '-Wbad-function-cast'
+]
+
+foreach flag: extra_flags
+ if cc.has_argument(flag)
+ cflags += flag
+ endif
+endforeach
+
+deps += ['common_sfc_efx', 'bus_pci']
+sources = files(
+ 'sfc_vdpa.c',
+)
new file mode 100644
@@ -0,0 +1,286 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ *
+ * Copyright(c) 2020-2021 Xilinx, Inc.
+ */
+
+#include <stdbool.h>
+#include <stdint.h>
+#include <sys/queue.h>
+
+#include <rte_common.h>
+#include <rte_errno.h>
+#include <rte_string_fns.h>
+#include <rte_vfio.h>
+#include <rte_vhost.h>
+
+#include "efx.h"
+#include "sfc_efx.h"
+#include "sfc_vdpa.h"
+
+TAILQ_HEAD(sfc_vdpa_adapter_list_head, sfc_vdpa_adapter);
+static struct sfc_vdpa_adapter_list_head sfc_vdpa_adapter_list =
+ TAILQ_HEAD_INITIALIZER(sfc_vdpa_adapter_list);
+
+static pthread_mutex_t sfc_vdpa_adapter_list_lock = PTHREAD_MUTEX_INITIALIZER;
+
+struct sfc_vdpa_adapter *
+sfc_vdpa_get_adapter_by_dev(struct rte_pci_device *pdev)
+{
+ bool found = false;
+ struct sfc_vdpa_adapter *sva;
+
+ pthread_mutex_lock(&sfc_vdpa_adapter_list_lock);
+
+ TAILQ_FOREACH(sva, &sfc_vdpa_adapter_list, next) {
+ if (pdev == sva->pdev) {
+ found = true;
+ break;
+ }
+ }
+
+ pthread_mutex_unlock(&sfc_vdpa_adapter_list_lock);
+
+ return found ? sva : NULL;
+}
+
+static int
+sfc_vdpa_vfio_setup(struct sfc_vdpa_adapter *sva)
+{
+ struct rte_pci_device *dev = sva->pdev;
+ char dev_name[RTE_DEV_NAME_MAX_LEN] = {0};
+ int rc;
+
+ if (dev == NULL)
+ goto fail_inval;
+
+ rte_pci_device_name(&dev->addr, dev_name, RTE_DEV_NAME_MAX_LEN);
+
+ sva->vfio_container_fd = rte_vfio_container_create();
+ if (sva->vfio_container_fd < 0) {
+ sfc_vdpa_err(sva, "failed to create VFIO container");
+ goto fail_container_create;
+ }
+
+ rc = rte_vfio_get_group_num(rte_pci_get_sysfs_path(), dev_name,
+ &sva->iommu_group_num);
+ if (rc <= 0) {
+ sfc_vdpa_err(sva, "failed to get IOMMU group for %s : %s",
+ dev_name, rte_strerror(-rc));
+ goto fail_get_group_num;
+ }
+
+ sva->vfio_group_fd =
+ rte_vfio_container_group_bind(sva->vfio_container_fd,
+ sva->iommu_group_num);
+ if (sva->vfio_group_fd < 0) {
+ sfc_vdpa_err(sva,
+ "failed to bind IOMMU group %d to container %d",
+ sva->iommu_group_num, sva->vfio_container_fd);
+ goto fail_group_bind;
+ }
+
+ if (rte_pci_map_device(dev) != 0) {
+ sfc_vdpa_err(sva, "failed to map PCI device %s : %s",
+ dev_name, rte_strerror(rte_errno));
+ goto fail_pci_map_device;
+ }
+
+ sva->vfio_dev_fd = dev->intr_handle.vfio_dev_fd;
+
+ return 0;
+
+fail_pci_map_device:
+ if (rte_vfio_container_group_unbind(sva->vfio_container_fd,
+ sva->iommu_group_num) != 0) {
+ sfc_vdpa_err(sva,
+ "failed to unbind IOMMU group %d from container %d",
+ sva->iommu_group_num, sva->vfio_container_fd);
+ }
+
+fail_group_bind:
+fail_get_group_num:
+ if (rte_vfio_container_destroy(sva->vfio_container_fd) != 0) {
+ sfc_vdpa_err(sva, "failed to destroy container %d",
+ sva->vfio_container_fd);
+ }
+
+fail_container_create:
+fail_inval:
+ return -1;
+}
+
+static void
+sfc_vdpa_vfio_teardown(struct sfc_vdpa_adapter *sva)
+{
+ rte_pci_unmap_device(sva->pdev);
+
+ if (rte_vfio_container_group_unbind(sva->vfio_container_fd,
+ sva->iommu_group_num) != 0) {
+ sfc_vdpa_err(sva,
+ "failed to unbind IOMMU group %d from container %d",
+ sva->iommu_group_num, sva->vfio_container_fd);
+ }
+
+ if (rte_vfio_container_destroy(sva->vfio_container_fd) != 0) {
+ sfc_vdpa_err(sva,
+ "failed to destroy container %d",
+ sva->vfio_container_fd);
+ }
+}
+
+static int
+sfc_vdpa_set_log_prefix(struct sfc_vdpa_adapter *sva)
+{
+ struct rte_pci_device *pci_dev = sva->pdev;
+ int ret;
+
+ ret = snprintf(sva->log_prefix, sizeof(sva->log_prefix),
+ "PMD: sfc_vdpa " PCI_PRI_FMT " : ",
+ pci_dev->addr.domain, pci_dev->addr.bus,
+ pci_dev->addr.devid, pci_dev->addr.function);
+
+ if (ret < 0 || ret >= (int)sizeof(sva->log_prefix)) {
+ SFC_VDPA_GENERIC_LOG(ERR,
+ "reserved log prefix is too short for " PCI_PRI_FMT,
+ pci_dev->addr.domain, pci_dev->addr.bus,
+ pci_dev->addr.devid, pci_dev->addr.function);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+uint32_t
+sfc_vdpa_register_logtype(const struct rte_pci_addr *pci_addr,
+ const char *lt_prefix_str, uint32_t ll_default)
+{
+ size_t lt_prefix_str_size = strlen(lt_prefix_str);
+ size_t lt_str_size_max;
+ char *lt_str = NULL;
+ int ret;
+
+ if (SIZE_MAX - PCI_PRI_STR_SIZE - 1 > lt_prefix_str_size) {
+ ++lt_prefix_str_size; /* Reserve space for prefix separator */
+ lt_str_size_max = lt_prefix_str_size + PCI_PRI_STR_SIZE + 1;
+ } else {
+ return RTE_LOGTYPE_PMD;
+ }
+
+ lt_str = rte_zmalloc("logtype_str", lt_str_size_max, 0);
+ if (lt_str == NULL)
+ return RTE_LOGTYPE_PMD;
+
+ strncpy(lt_str, lt_prefix_str, lt_prefix_str_size);
+ lt_str[lt_prefix_str_size - 1] = '.';
+ rte_pci_device_name(pci_addr, lt_str + lt_prefix_str_size,
+ lt_str_size_max - lt_prefix_str_size);
+ lt_str[lt_str_size_max - 1] = '\0';
+
+ ret = rte_log_register_type_and_pick_level(lt_str, ll_default);
+ rte_free(lt_str);
+
+ return (ret < 0) ? RTE_LOGTYPE_PMD : ret;
+}
+
+static struct rte_pci_id pci_id_sfc_vdpa_efx_map[] = {
+ { RTE_PCI_DEVICE(EFX_PCI_VENID_XILINX, EFX_PCI_DEVID_RIVERHEAD_VF) },
+ { .vendor_id = 0, /* sentinel */ },
+};
+
+static int
+sfc_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
+ struct rte_pci_device *pci_dev)
+{
+ struct sfc_vdpa_adapter *sva = NULL;
+ uint32_t logtype_main;
+ int ret = 0;
+
+ if (sfc_efx_dev_class_get(pci_dev->device.devargs) !=
+ SFC_EFX_DEV_CLASS_VDPA) {
+ SFC_VDPA_GENERIC_LOG(INFO,
+ "Incompatible device class: skip probing, should be probed by other sfc driver.");
+ return 1;
+ }
+
+ /*
+ * It will not be probed in the secondary process. As device class
+ * is vdpa so return 0 to avoid probe by other sfc driver
+ */
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return 0;
+
+ logtype_main = sfc_vdpa_register_logtype(&pci_dev->addr,
+ SFC_VDPA_LOGTYPE_MAIN_STR,
+ RTE_LOG_NOTICE);
+
+ sva = rte_zmalloc("sfc_vdpa", sizeof(struct sfc_vdpa_adapter), 0);
+ if (sva == NULL)
+ goto fail_zmalloc;
+
+ sva->pdev = pci_dev;
+ sva->logtype_main = logtype_main;
+
+ ret = sfc_vdpa_set_log_prefix(sva);
+ if (ret != 0)
+ goto fail_set_log_prefix;
+
+ sfc_vdpa_log_init(sva, "entry");
+
+ sfc_vdpa_log_init(sva, "vfio init");
+ if (sfc_vdpa_vfio_setup(sva) < 0) {
+ sfc_vdpa_err(sva, "failed to setup device %s", pci_dev->name);
+ goto fail_vfio_setup;
+ }
+
+ pthread_mutex_lock(&sfc_vdpa_adapter_list_lock);
+ TAILQ_INSERT_TAIL(&sfc_vdpa_adapter_list, sva, next);
+ pthread_mutex_unlock(&sfc_vdpa_adapter_list_lock);
+
+ sfc_vdpa_log_init(sva, "done");
+
+ return 0;
+
+fail_vfio_setup:
+fail_set_log_prefix:
+ rte_free(sva);
+
+fail_zmalloc:
+ return -1;
+}
+
+static int
+sfc_vdpa_pci_remove(struct rte_pci_device *pci_dev)
+{
+ struct sfc_vdpa_adapter *sva = NULL;
+
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+ return -1;
+
+ sva = sfc_vdpa_get_adapter_by_dev(pci_dev);
+ if (sva == NULL) {
+ sfc_vdpa_info(sva, "invalid device: %s", pci_dev->name);
+ return -1;
+ }
+
+ pthread_mutex_lock(&sfc_vdpa_adapter_list_lock);
+ TAILQ_REMOVE(&sfc_vdpa_adapter_list, sva, next);
+ pthread_mutex_unlock(&sfc_vdpa_adapter_list_lock);
+
+ sfc_vdpa_vfio_teardown(sva);
+
+ rte_free(sva);
+
+ return 0;
+}
+
+static struct rte_pci_driver rte_sfc_vdpa = {
+ .id_table = pci_id_sfc_vdpa_efx_map,
+ .drv_flags = 0,
+ .probe = sfc_vdpa_pci_probe,
+ .remove = sfc_vdpa_pci_remove,
+};
+
+RTE_PMD_REGISTER_PCI(net_sfc_vdpa, rte_sfc_vdpa);
+RTE_PMD_REGISTER_PCI_TABLE(net_sfc_vdpa, pci_id_sfc_vdpa_efx_map);
+RTE_PMD_REGISTER_KMOD_DEP(net_sfc_vdpa, "* vfio-pci");
+RTE_LOG_REGISTER_SUFFIX(sfc_vdpa_logtype_driver, driver, NOTICE);
new file mode 100644
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ *
+ * Copyright(c) 2020-2021 Xilinx, Inc.
+ */
+
+#ifndef _SFC_VDPA_H
+#define _SFC_VDPA_H
+
+#include <stdint.h>
+#include <sys/queue.h>
+
+#include <rte_bus_pci.h>
+
+#include "sfc_vdpa_log.h"
+
+/* Adapter private data */
+struct sfc_vdpa_adapter {
+ TAILQ_ENTRY(sfc_vdpa_adapter) next;
+ struct rte_pci_device *pdev;
+ struct rte_pci_addr pci_addr;
+
+ char log_prefix[SFC_VDPA_LOG_PREFIX_MAX];
+ uint32_t logtype_main;
+
+ int vfio_group_fd;
+ int vfio_dev_fd;
+ int vfio_container_fd;
+ int iommu_group_num;
+};
+
+uint32_t
+sfc_vdpa_register_logtype(const struct rte_pci_addr *pci_addr,
+ const char *lt_prefix_str,
+ uint32_t ll_default);
+
+struct sfc_vdpa_adapter *
+sfc_vdpa_get_adapter_by_dev(struct rte_pci_device *pdev);
+
+#endif /* _SFC_VDPA_H */
+
new file mode 100644
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ *
+ * Copyright(c) 2020-2021 Xilinx, Inc.
+ */
+
+#ifndef _SFC_VDPA_LOG_H_
+#define _SFC_VDPA_LOG_H_
+
+/** Generic driver log type */
+extern int sfc_vdpa_logtype_driver;
+
+/** Common log type name prefix */
+#define SFC_VDPA_LOGTYPE_PREFIX "pmd.vdpa.sfc."
+
+/** Log PMD generic message, add a prefix and a line break */
+#define SFC_VDPA_GENERIC_LOG(level, ...) \
+ rte_log(RTE_LOG_ ## level, sfc_vdpa_logtype_driver, \
+ RTE_FMT("PMD: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
+ RTE_FMT_TAIL(__VA_ARGS__ ,)))
+
+/** Name prefix for the per-device log type used to report basic information */
+#define SFC_VDPA_LOGTYPE_MAIN_STR SFC_VDPA_LOGTYPE_PREFIX "main"
+
+#define SFC_VDPA_LOG_PREFIX_MAX 32
+
+/* Log PMD message, automatically add prefix and \n */
+#define SFC_VDPA_LOG(sva, level, type, ...) \
+ rte_log(level, type, \
+ RTE_FMT("%s" RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \
+ sva->log_prefix, \
+ RTE_FMT_TAIL(__VA_ARGS__ ,)))
+
+#define sfc_vdpa_err(sva, ...) \
+ do { \
+ const struct sfc_vdpa_adapter *_sva = (sva); \
+ \
+ SFC_VDPA_LOG(_sva, RTE_LOG_ERR, \
+ _sva->logtype_main, __VA_ARGS__); \
+ } while (0)
+
+#define sfc_vdpa_warn(sva, ...) \
+ do { \
+ const struct sfc_vdpa_adapter *_sva = (sva); \
+ \
+ SFC_VDPA_LOG(_sva, RTE_LOG_WARNING, \
+ _sva->logtype_main, __VA_ARGS__); \
+ } while (0)
+
+#define sfc_vdpa_notice(sva, ...) \
+ do { \
+ const struct sfc_vdpa_adapter *_sva = (sva); \
+ \
+ SFC_VDPA_LOG(_sva, RTE_LOG_NOTICE, \
+ _sva->logtype_main, __VA_ARGS__); \
+ } while (0)
+
+#define sfc_vdpa_info(sva, ...) \
+ do { \
+ const struct sfc_vdpa_adapter *_sva = (sva); \
+ \
+ SFC_VDPA_LOG(_sva, RTE_LOG_INFO, \
+ _sva->logtype_main, __VA_ARGS__); \
+ } while (0)
+
+#define sfc_vdpa_log_init(sva, ...) \
+ do { \
+ const struct sfc_vdpa_adapter *_sva = (sva); \
+ \
+ SFC_VDPA_LOG(_sva, RTE_LOG_INFO, \
+ _sva->logtype_main, \
+ RTE_FMT("%s(): " \
+ RTE_FMT_HEAD(__VA_ARGS__ ,), \
+ __func__, \
+ RTE_FMT_TAIL(__VA_ARGS__ ,))); \
+ } while (0)
+
+#endif /* _SFC_VDPA_LOG_H_ */
new file mode 100644
@@ -0,0 +1,3 @@
+DPDK_21 {
+ local: *;
+};