Hi Andrew,

-----Original Message-----
From: Andrew Rybchenko <redacted>
Sent: Friday, August 13, 2021 4:39 PM
To: Xia, Chenbo <redacted>; Vijay Srivastava
[off-list ref]; dev@dpdk.org
Cc: maxime.coquelin@redhat.com; Vijay Kumar Srivastava <redacted>
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:

quoted

Hi Vijay,

quoted

-----Original Message-----
From: Vijay Srivastava <redacted>
Sent: Wednesday, July 7, 2021 12:44 AM
To: dev@dpdk.org
Cc: maxime.coquelin@redhat.com; Xia, Chenbo <redacted>;
andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava [off-list ref]
Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver

From: Vijay Kumar Srivastava <redacted>

Add new vDPA PMD to support vDPA operation by Xilinx devices.

vDPA operations of Xilinx devices

quoted

This patch implements probe and remove functions.

Signed-off-by: Vijay Kumar Srivastava <redacted>

[snip]

quoted

quoted

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;

quoted

quoted

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

quoted

quoted

+		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]

quoted

quoted

diff --git a/drivers/vdpa/sfc/sfc_vdpa_log.h

b/drivers/vdpa/sfc/sfc_vdpa_log.h

quoted

quoted

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

quoted

quoted

*/
+#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.