From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 79AAF1B25C for ; Thu, 9 Nov 2017 04:47:33 +0100 (CET) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Nov 2017 19:47:25 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,367,1505804400"; d="scan'208";a="147745571" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.239.201.116]) ([10.239.201.116]) by orsmga004.jf.intel.com with ESMTP; 08 Nov 2017 19:47:24 -0800 To: Zhiyong Yang , dev@dpdk.org References: <20171108110348.38548-1-zhiyong.yang@intel.com> <20171109031818.31040-1-zhiyong.yang@intel.com> Cc: yliu@fridaylinux.org From: "Tan, Jianfeng" Message-ID: Date: Thu, 9 Nov 2017 11:47:23 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20171109031818.31040-1-zhiyong.yang@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3] 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 03:47:34 -0000 On 11/9/2017 11:18 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. Only > supporting and enabling msix can assign "VIRTIO_MSIX_ENABLED(2)" to > use_msix. Above sentence seems not correct. As we can assign VIRTIO_MSIX_NONE, VIRTIO_MSIX_DISABLED, VIRTIO_MSIX_ENABLED to use_msix to indicate three different msix status. > > Fixes: cb482cb3a305 ("net/virtio: fix MAC address read") > Signed-off-by: Zhiyong Yang > --- > > 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)); Maybe we can check hw->use_msix as an additional check; if it does not equal VIRTIO_MSIX_ENABLE, returns -1. > + > + 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 > + > 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); > + > extern const struct virtio_pci_ops legacy_ops; > extern const struct virtio_pci_ops modern_ops; > extern const struct virtio_pci_ops virtio_user_ops;