Re: [dpdk-dev] [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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.