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 C203DA0A02; Thu, 14 Jan 2021 17:04:26 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8E71414138E; Thu, 14 Jan 2021 17:04:26 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mails.dpdk.org (Postfix) with ESMTP id 0681E14138D for ; Thu, 14 Jan 2021 17:04:24 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610640264; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fSpzm68EsCVEnQvIn68gj2iYp/feZ9oB8IguhIyCpDM=; b=KoQpk9gSYdJGWUFK+jwpydXEQFDCQs0u7zNZ6BJ04hdrshqdzkclqz7u1JPv0H8Mok1C3d gGujRERjMtbtOq6WMgVIsn+5O6/ravezsoOnr7LlPTaEvadiF6TMg56SyF+EPleIBYJXXK +KvKnuWChg0pUQY75QImTkn7oUSTlKM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-232-esm8gL4MMtSkY_DUGI2-8g-1; Thu, 14 Jan 2021 11:04:21 -0500 X-MC-Unique: esm8gL4MMtSkY_DUGI2-8g-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BFB3E107ACF7; Thu, 14 Jan 2021 16:04:20 +0000 (UTC) Received: from [10.36.110.24] (unknown [10.36.110.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9D3C15D6AD; Thu, 14 Jan 2021 16:04:16 +0000 (UTC) To: David Marchand Cc: dev , "Xia, Chenbo" , Olivier Matz , Adrian Moreno Zapata References: <20201220211405.313012-1-maxime.coquelin@redhat.com> <20201220211405.313012-6-maxime.coquelin@redhat.com> From: Maxime Coquelin Message-ID: <774eb1f0-585e-f114-fe91-546673e7375e@redhat.com> Date: Thu, 14 Jan 2021 17:04:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=maxime.coquelin@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 05/40] net/virtio: move PCI device init in dedicated file 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" On 1/5/21 10:19 PM, David Marchand wrote: > On Sun, Dec 20, 2020 at 10:14 PM Maxime Coquelin > wrote: >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >> index 99a5a1bb88..ca21a019e5 100644 >> --- a/drivers/net/virtio/virtio_ethdev.c >> +++ b/drivers/net/virtio/virtio_ethdev.c > > [...] > >> @@ -2135,52 +2078,7 @@ virtio_dev_devargs_parse(struct rte_devargs *devargs, int *vdpa, >> return ret; >> } >> >> -static int eth_virtio_pci_probe(struct rte_pci_driver *pci_drv __rte_unused, >> - struct rte_pci_device *pci_dev) >> -{ >> - int vdpa = 0; >> - int ret = 0; >> - >> - ret = virtio_dev_devargs_parse(pci_dev->device.devargs, &vdpa, NULL, >> - NULL); >> - if (ret < 0) { >> - PMD_INIT_LOG(ERR, "devargs parsing is failed"); >> - return ret; >> - } >> - /* virtio pmd skips probe if device needs to work in vdpa mode */ >> - if (vdpa == 1) >> - return 1; >> - >> - return rte_eth_dev_pci_generic_probe(pci_dev, sizeof(struct virtio_pci_dev), >> - eth_virtio_dev_init); >> -} >> - >> -static int eth_virtio_pci_remove(struct rte_pci_device *pci_dev) >> -{ >> - int ret; >> - >> - ret = rte_eth_dev_pci_generic_remove(pci_dev, eth_virtio_dev_uninit); >> - /* Port has already been released by close. */ >> - if (ret == -ENODEV) >> - ret = 0; >> - return ret; >> -} >> - >> -static struct rte_pci_driver rte_virtio_pmd = { >> - .driver = { >> - .name = "net_virtio", >> - }, >> - .id_table = pci_id_virtio_map, >> - .drv_flags = 0, >> - .probe = eth_virtio_pci_probe, >> - .remove = eth_virtio_pci_remove, >> -}; >> >> -RTE_INIT(rte_virtio_pmd_init) >> -{ >> - rte_eal_iopl_init(); >> - rte_pci_register(&rte_virtio_pmd); >> -} >> > > Leaving three empty lines if I count correctly. > > >> static bool >> rx_offload_enabled(struct virtio_hw *hw) >> @@ -2521,7 +2419,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev) >> /* >> * Stop device: disable interrupt and mark link down >> */ >> -static int >> +int >> virtio_dev_stop(struct rte_eth_dev *dev) >> { >> struct virtio_hw *hw = dev->data->dev_private; >> @@ -2673,7 +2571,5 @@ __rte_unused uint8_t is_rx) >> } >> >> RTE_PMD_EXPORT_NAME(net_virtio, __COUNTER__); > > This belongs with the rest of the pci driver declarations. > > $ ./usertools/dpdk-pmdinfo.py > $HOME/builds/build-gcc-shared/drivers/librte_net_virtio.so > PMD NAME: net_virtio_user > PMD PARAMETERS: path= mac= cq= queue_size= > queues= iface= server=<0|1> mrg_rxbuf=<0|1> > in_order=<0|1> packed_vq=<0|1> speed= vectorized=<0|1> > > PMD NAME: net_virtio > > ^^^ > Here, I would expect the pci table and the kmod dependencies. > > > This makes me realise that the net_virtio_user driver is not exported > (but it seems to work without it.. interesting). As discussed offlist, I have moved RTE_PMD_EXPORT_NAME to virtio_pci_ethdev.c, and I now get expected output: $ ./usertools/dpdk-pmdinfo.py build/drivers/librte_net_virtio.so PMD NAME: net_virtio_user PMD PARAMETERS: path= mac= cq= queue_size= queues= iface= server=<0|1> mrg_rxbuf=<0|1> in_order=<0|1> packed_vq=<0|1> speed= vectorized=<0|1> PMD NAME: net_virtio PMD KMOD DEPENDENCIES: * igb_uio | uio_pci_generic | vfio-pci PMD HW SUPPORT: Red Hat, Inc. (1af4) : Virtio network device (1000) (All Subdevices) Red Hat, Inc. (1af4) : Virtio network device (1041) (All Subdevices) > >> -RTE_PMD_REGISTER_PCI_TABLE(net_virtio, pci_id_virtio_map); >> -RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci"); >> RTE_LOG_REGISTER(virtio_logtype_init, pmd.net.virtio.init, NOTICE); >> RTE_LOG_REGISTER(virtio_logtype_driver, pmd.net.virtio.driver, NOTICE); >> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h >> index b7d52d497f..13395937c8 100644 >> --- a/drivers/net/virtio/virtio_ethdev.h >> +++ b/drivers/net/virtio/virtio_ethdev.h >> @@ -117,6 +117,8 @@ void virtio_interrupt_handler(void *param); >> >> int virtio_dev_pause(struct rte_eth_dev *dev); >> void virtio_dev_resume(struct rte_eth_dev *dev); >> +int virtio_dev_stop(struct rte_eth_dev *dev); >> +int virtio_dev_close(struct rte_eth_dev *dev); >> int virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **tx_pkts, >> int nb_pkts); >> >> diff --git a/drivers/net/virtio/virtio_pci_ethdev.c b/drivers/net/virtio/virtio_pci_ethdev.c >> new file mode 100644 >> index 0000000000..9c0935068e >> --- /dev/null >> +++ b/drivers/net/virtio/virtio_pci_ethdev.c >> @@ -0,0 +1,148 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright(c) 2010-2016 Intel Corporation >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> + >> +#include "virtio_ethdev.h" >> +#include "virtio_pci.h" >> +#include "virtio_logs.h" >> + >> +/* >> + * The set of PCI devices this driver supports >> + */ >> +static const struct rte_pci_id pci_id_virtio_map[] = { >> + { RTE_PCI_DEVICE(VIRTIO_PCI_VENDORID, VIRTIO_PCI_LEGACY_DEVICEID_NET) }, >> + { RTE_PCI_DEVICE(VIRTIO_PCI_VENDORID, VIRTIO_PCI_MODERN_DEVICEID_NET) }, >> + { .vendor_id = 0, /* sentinel */ }, >> +}; >> + >> +static int >> +eth_virtio_pci_init(struct rte_eth_dev *eth_dev) >> +{ >> + return eth_virtio_dev_init(eth_dev); >> +} >> + >> +static int >> +eth_virtio_pci_uninit(struct rte_eth_dev *eth_dev) >> +{ >> + int ret; >> + PMD_INIT_FUNC_TRACE(); >> + >> + if (rte_eal_process_type() == RTE_PROC_SECONDARY) >> + return 0; >> + >> + ret = virtio_dev_stop(eth_dev); >> + virtio_dev_close(eth_dev); >> + >> + PMD_INIT_LOG(DEBUG, "dev_uninit completed"); >> + >> + return ret; >> +} >> + >> +static int vdpa_check_handler(__rte_unused const char *key, >> + const char *value, void *ret_val) >> +{ >> + if (strcmp(value, "1") == 0) >> + *(int *)ret_val = 1; >> + else >> + *(int *)ret_val = 0; >> + >> + return 0; >> +} >> + >> +#define VIRTIO_ARG_VDPA "vdpa" >> + >> +static int >> +virtio_pci_devargs_parse(struct rte_devargs *devargs, int *vdpa) >> +{ >> + struct rte_kvargs *kvlist; >> + int ret = 0; >> + >> + if (devargs == NULL) >> + return 0; >> + >> + kvlist = rte_kvargs_parse(devargs->args, NULL); >> + if (kvlist == NULL) { >> + PMD_INIT_LOG(ERR, "error when parsing param"); >> + return 0; >> + } >> + >> + if (vdpa && rte_kvargs_count(kvlist, VIRTIO_ARG_VDPA) == 1) { > > I would remove this check on vdpa pointer being NULL, as it gets > passed on the stack by a single caller in this file. I agree this is not necessary. > >> + /* vdpa mode selected when there's a key-value pair: >> + * vdpa=1 >> + */ >> + ret = rte_kvargs_process(kvlist, VIRTIO_ARG_VDPA, >> + vdpa_check_handler, vdpa); >> + if (ret < 0) >> + PMD_INIT_LOG(ERR, "Failed to parse %s", VIRTIO_ARG_VDPA); >> + } >> + >> + rte_kvargs_free(kvlist); >> + >> + return ret; >> +} >> + >> +static int eth_virtio_pci_probe(struct rte_pci_driver *pci_drv __rte_unused, >> + struct rte_pci_device *pci_dev) >> +{ >> + int vdpa = 0; >> + int ret = 0; >> + >> + ret = virtio_pci_devargs_parse(pci_dev->device.devargs, &vdpa); >> + if (ret < 0) { >> + PMD_INIT_LOG(ERR, "devargs parsing is failed"); >> + return ret; >> + } >> + /* virtio pmd skips probe if device needs to work in vdpa mode */ >> + if (vdpa == 1) >> + return 1; >> + >> + return rte_eth_dev_pci_generic_probe(pci_dev, sizeof(struct virtio_pci_dev), >> + eth_virtio_pci_init); >> +} >> + >> +static int eth_virtio_pci_remove(struct rte_pci_device *pci_dev) >> +{ >> + int ret; >> + >> + ret = rte_eth_dev_pci_generic_remove(pci_dev, eth_virtio_pci_uninit); >> + /* Port has already been released by close. */ >> + if (ret == -ENODEV) >> + ret = 0; >> + return ret; >> +} >> + >> +static struct rte_pci_driver rte_virtio_pmd = { > > This seems too generic. > > virtio_pci_pmd ? virtio_net_pci_pmd is even better. > >> + .driver = { >> + .name = "net_virtio", I keept it as is here not to break tools relying on driver name. Makes sense? >> + }, >> + .id_table = pci_id_virtio_map, >> + .drv_flags = 0, >> + .probe = eth_virtio_pci_probe, >> + .remove = eth_virtio_pci_remove, >> +}; >> + >> +RTE_INIT(rte_virtio_pmd_init) > > virtio_pci_pmd_init ? > rte_virtio_net_pci_pmd_init > >> +{ >> + rte_eal_iopl_init(); >> + rte_pci_register(&rte_virtio_pmd); >> +} >> + >> +RTE_PMD_REGISTER_PCI_TABLE(net_virtio, pci_id_virtio_map); >> +RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci"); > > > -- > David Marchand >