From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 8F4331B3BB; Thu, 9 Nov 2017 09:18:25 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B71734E4D4; Thu, 9 Nov 2017 08:18:24 +0000 (UTC) Received: from [10.36.112.47] (ovpn-112-47.ams2.redhat.com [10.36.112.47]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 946D461988; Thu, 9 Nov 2017 08:18:22 +0000 (UTC) To: Zhiyong Yang , dev@dpdk.org Cc: stable@dpdk.org, jianfeng.tan@intel.com, yliu@fridaylinux.org References: <20171109031818.31040-1-zhiyong.yang@intel.com> <20171109044607.3327-1-zhiyong.yang@intel.com> From: Maxime Coquelin Message-ID: <02aba023-85a9-3a0c-7a4a-f316a6c5ca36@redhat.com> Date: Thu, 9 Nov 2017 09:18:19 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171109044607.3327-1-zhiyong.yang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 09 Nov 2017 08:18:24 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH v4] net/virtio: fix rxq intr config fails using vfio-pci X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Nov 2017 08:18:26 -0000 On 11/09/2017 05:46 AM, Zhiyong Yang wrote: > When running l3fwd-power to test virtio rxq interrupt using vfio > pci noiommu mode, startup fails. In the function virtio_read_caps, > the code if (flags & PCI_MSIX_ENABLE) intends to double check > if vfio msix is enabled or not. However, it is not enable at that > time. So use_msix is assigned to "0", not "1", which causes the > failure of configuring rxq intr in l3fwd-power. > This patch adds the function "vtpci_msix_detect" to detect the status > of msix when interrupt changes happen. > In the meanwhile, virtio_intr_enable/disable are introduced to wrap > rte_intr_enable/disable to enhance the ability to detect msix. > use_msix can indicate three different msix status by: > VIRTIO_MSIX_NONE (0) > VIRTIO_MSIX_DISABLED (1) > VIRTIO_MSIX_ENABLED (2) > > CC: stable@dpdk.org > CC: jianfeng.tan@intel.com > CC: yliu@fridaylinux.org > CC: maxime.coquelin@redhat.com > > Fixes: cb482cb3a305 ("net/virtio: fix MAC address read") > Signed-off-by: Zhiyong Yang > --- > > Changes in v4: > Enhance the commit log description. > > Changes in v3: > Simply the code according to jianfeng's comments. > > Changes in v2: > Add the capability to detect msix if virtio intr changes. > > drivers/net/virtio/virtio_ethdev.c | 46 ++++++++++++++++++++++++++++++------ > drivers/net/virtio/virtio_pci.c | 48 ++++++++++++++++++++++++++++++++++++-- > drivers/net/virtio/virtio_pci.h | 6 +++++ > 3 files changed, 91 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > index d2576d5e0..87ac2bee6 100644 > --- a/drivers/net/virtio/virtio_ethdev.c > +++ b/drivers/net/virtio/virtio_ethdev.c > @@ -97,6 +97,9 @@ static void virtio_mac_addr_remove(struct rte_eth_dev *dev, uint32_t index); > static void virtio_mac_addr_set(struct rte_eth_dev *dev, > struct ether_addr *mac_addr); > > +static int virtio_intr_enable(struct rte_eth_dev *dev); > +static int virtio_intr_disable(struct rte_eth_dev *dev); > + > static int virtio_dev_queue_stats_mapping_set( > struct rte_eth_dev *eth_dev, > uint16_t queue_id, > @@ -618,7 +621,7 @@ virtio_dev_close(struct rte_eth_dev *dev) > virtio_queues_unbind_intr(dev); > > if (intr_conf->lsc || intr_conf->rxq) { > - rte_intr_disable(dev->intr_handle); > + virtio_intr_disable(dev); > rte_intr_efd_disable(dev->intr_handle); > rte_free(dev->intr_handle->intr_vec); > dev->intr_handle->intr_vec = NULL; > @@ -1160,6 +1163,34 @@ virtio_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on) > } > > static int > +virtio_intr_enable(struct rte_eth_dev *dev) > +{ > + struct virtio_hw *hw = dev->data->dev_private; > + > + if (rte_intr_enable(dev->intr_handle) < 0) > + return -1; > + > + if (!hw->virtio_user_dev) > + hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev)); > + > + return 0; > +} > + > +static int > +virtio_intr_disable(struct rte_eth_dev *dev) > +{ > + struct virtio_hw *hw = dev->data->dev_private; > + > + if (rte_intr_disable(dev->intr_handle) < 0) > + return -1; > + > + if (!hw->virtio_user_dev) > + hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev)); > + > + return 0; > +} > + > +static int > virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features) > { > uint64_t host_features; > @@ -1228,7 +1259,7 @@ virtio_interrupt_handler(void *param) > isr = vtpci_isr(hw); > PMD_DRV_LOG(INFO, "interrupt status = %#x", isr); > > - if (rte_intr_enable(dev->intr_handle) < 0) > + if (virtio_intr_enable(dev) < 0) > PMD_DRV_LOG(ERR, "interrupt enable failed"); > > if (isr & VIRTIO_PCI_ISR_CONFIG) { > @@ -1348,7 +1379,7 @@ virtio_configure_intr(struct rte_eth_dev *dev) > * to change the config size from 20 to 24, or VIRTIO_MSI_QUEUE_VECTOR > * (22) will be ignored. > */ > - if (rte_intr_enable(dev->intr_handle) < 0) { > + if (virtio_intr_enable(dev) < 0) { > PMD_DRV_LOG(ERR, "interrupt enable failed"); > return -1; > } > @@ -1388,7 +1419,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features) > } > > /* If host does not support both status and MSI-X then disable LSC */ > - if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS) && hw->use_msix) > + if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS) && > + hw->use_msix != VIRTIO_MSIX_NONE) > eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC; > else > eth_dev->data->dev_flags &= ~RTE_ETH_DEV_INTR_LSC; > @@ -1801,9 +1833,9 @@ virtio_dev_start(struct rte_eth_dev *dev) > */ > if (dev->data->dev_conf.intr_conf.lsc || > dev->data->dev_conf.intr_conf.rxq) { > - rte_intr_disable(dev->intr_handle); > + virtio_intr_disable(dev); > > - if (rte_intr_enable(dev->intr_handle) < 0) { > + if (virtio_intr_enable(dev) < 0) { > PMD_DRV_LOG(ERR, "interrupt enable failed"); > return -EIO; > } > @@ -1912,7 +1944,7 @@ virtio_dev_stop(struct rte_eth_dev *dev) > PMD_INIT_LOG(DEBUG, "stop"); > > if (intr_conf->lsc || intr_conf->rxq) > - rte_intr_disable(dev->intr_handle); > + virtio_intr_disable(dev); > > hw->started = 0; > memset(&link, 0, sizeof(link)); > diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c > index 55b717c03..10c45228c 100644 > --- a/drivers/net/virtio/virtio_pci.c > +++ b/drivers/net/virtio/virtio_pci.c > @@ -57,7 +57,8 @@ > * The remaining space is defined by each driver as the per-driver > * configuration space. > */ > -#define VIRTIO_PCI_CONFIG(hw) (((hw)->use_msix) ? 24 : 20) > +#define VIRTIO_PCI_CONFIG(hw) \ > + (((hw)->use_msix == VIRTIO_MSIX_ENABLED) ? 24 : 20) > > static inline int > check_vq_phys_addr_ok(struct virtqueue *vq) > @@ -617,7 +618,9 @@ virtio_read_caps(struct rte_pci_device *dev, struct virtio_hw *hw) > uint16_t flags = ((uint16_t *)&cap)[1]; > > if (flags & PCI_MSIX_ENABLE) > - hw->use_msix = 1; > + hw->use_msix = VIRTIO_MSIX_ENABLED; > + else > + hw->use_msix = VIRTIO_MSIX_DISABLED; > } > > if (cap.cap_vndr != PCI_CAP_ID_VNDR) { > @@ -710,3 +713,44 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw) > > return 0; > } > + > +/* Return 0 (VIRTIO_MSIX_NONE) > + * Return 1 (VIRTIO_MSIX_DISABLED) > + * Return 2 (VIRTIO_MSIX_ENABLED) > + */ > + > +uint8_t > +vtpci_msix_detect(struct rte_pci_device *dev) > +{ > + uint8_t pos; > + struct virtio_pci_cap cap; > + int ret; > + > + ret = rte_pci_read_config(dev, &pos, 1, PCI_CAPABILITY_LIST); > + if (ret < 0) { > + PMD_INIT_LOG(DEBUG, "failed to read pci capability list"); > + return VIRTIO_MSIX_NONE; > + } > + > + while (pos) { > + ret = rte_pci_read_config(dev, &cap, sizeof(cap), pos); > + if (ret < 0) { > + PMD_INIT_LOG(ERR, > + "failed to read pci cap at pos: %x", pos); > + break; > + } > + > + if (cap.cap_vndr == PCI_CAP_ID_MSIX) { > + uint16_t flags = ((uint16_t *)&cap)[1]; > + > + if (flags & PCI_MSIX_ENABLE) > + return VIRTIO_MSIX_ENABLED; > + else > + return VIRTIO_MSIX_DISABLED; > + } > + > + pos = cap.cap_next; > + } > + > + return VIRTIO_MSIX_NONE; > +} > diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h > index 36d452c06..600e106eb 100644 > --- a/drivers/net/virtio/virtio_pci.h > +++ b/drivers/net/virtio/virtio_pci.h > @@ -314,6 +314,10 @@ struct virtio_net_config { > /* The alignment to use between consumer and producer parts of vring. */ > #define VIRTIO_PCI_VRING_ALIGN 4096 > > +#define VIRTIO_MSIX_NONE 0 > +#define VIRTIO_MSIX_DISABLED 1 > +#define VIRTIO_MSIX_ENABLED 2 Maybe it could be cleaner to change the defines into an enum. > + > static inline int > vtpci_with_feature(struct virtio_hw *hw, uint64_t bit) > { > @@ -339,6 +343,8 @@ void vtpci_read_dev_config(struct virtio_hw *, size_t, void *, int); > > uint8_t vtpci_isr(struct virtio_hw *); > > +uint8_t vtpci_msix_detect(struct rte_pci_device *dev); So here, the function would return an enum. Otherwise, the patch looks good to me. If you do the proposed change, feel free to add my: Acked-by: Maxime Coquelin Thanks, Maxime