Hi Andrew, > -----Original Message----- > From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> > Sent: Friday, August 13, 2021 4:39 PM > To: Xia, Chenbo <chenbo....@intel.com>; Vijay Srivastava > <vijay.srivast...@xilinx.com>; dev@dpdk.org > Cc: maxime.coque...@redhat.com; Vijay Kumar Srivastava <vsriv...@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.srivast...@xilinx.com> > >> Sent: Wednesday, July 7, 2021 12:44 AM > >> To: dev@dpdk.org > >> Cc: maxime.coque...@redhat.com; Xia, Chenbo <chenbo....@intel.com>; > >> andrew.rybche...@oktetlabs.ru; Vijay Kumar Srivastava <vsriv...@xilinx.com> > >> Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver > >> > >> From: Vijay Kumar Srivastava <vsriv...@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 <vsriv...@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.