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

Andrew Rybchenko andrew.rybchenko at oktetlabs.ru
Fri Aug 13 10:38:50 CEST 2021


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 at xilinx.com>
>> Sent: Wednesday, July 7, 2021 12:44 AM
>> To: dev at dpdk.org
>> Cc: maxime.coquelin at redhat.com; Xia, Chenbo <chenbo.xia at intel.com>;
>> andrew.rybchenko at oktetlabs.ru; Vijay Kumar Srivastava <vsrivast at xilinx.com>
>> Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver
>>
>> From: Vijay Kumar Srivastava <vsrivast at 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 at 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.


More information about the dev mailing list