From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3E1EEA0C4D; Fri, 13 Aug 2021 10:38:53 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F1C3D40151; Fri, 13 Aug 2021 10:38:52 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 8DB0A40140 for ; Fri, 13 Aug 2021 10:38:51 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id EC3CC7F52A; Fri, 13 Aug 2021 11:38:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru EC3CC7F52A DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1628843931; bh=LzOphWrMh2Rrdg5EuCbN1H4T1d07wMs1YFjAElOJFCc=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=fELDhVuSQbzY/mctsvH6Kj+DG7h6CQWHopJSbcj0JcjTiiXBNeNkHlpO4QpbkN0yb povBlkFO4V54rIT4rllVNjQyrQMrFQUQmF256DVIO7TNzOVJzwrrbZBdoUpGIFeBQe T5pBTs2lzjbUrx3ZYgYOxpAJgqFi2NpXZ+nMJsiw= To: "Xia, Chenbo" , Vijay Srivastava , "dev@dpdk.org" Cc: "maxime.coquelin@redhat.com" , Vijay Kumar Srivastava References: <20210706164418.32615-1-vsrivast@xilinx.com> <20210706164418.32615-2-vsrivast@xilinx.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Fri, 13 Aug 2021 11:38:50 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> Sent: Wednesday, July 7, 2021 12:44 AM >> To: dev@dpdk.org >> Cc: maxime.coquelin@redhat.com; Xia, Chenbo ; >> andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava >> Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver >> >> From: Vijay Kumar Srivastava >> >> 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 [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 >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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.