From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 4B9F51B32F for ; Wed, 8 Nov 2017 14:52:58 +0100 (CET) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP; 08 Nov 2017 05:52:58 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,364,1505804400"; d="scan'208";a="147453900" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.255.25.150]) ([10.255.25.150]) by orsmga004.jf.intel.com with ESMTP; 08 Nov 2017 05:52:56 -0800 To: Zhiyong Yang , dev@dpdk.org References: <20171031094456.24912-1-zhiyong.yang@intel.com> <20171108110348.38548-1-zhiyong.yang@intel.com> Cc: yliu@fridaylinux.org, maxime.coquelin@redhat.com From: "Tan, Jianfeng" Message-ID: Date: Wed, 8 Nov 2017 21:52:55 +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: <20171108110348.38548-1-zhiyong.yang@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2] 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: Wed, 08 Nov 2017 13:53:00 -0000 Hi Zhiyong, On 11/8/2017 7:03 PM, 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 > stage. 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 > support and enable msix can assign "1" to use_msix. Should be "2". Better to use macro here. > > Fixes: cb482cb3a305 ("net/virtio: fix MAC address read") > Signed-off-by: Zhiyong Yang > --- > drivers/net/virtio/virtio_ethdev.c | 64 +++++++++++++++++++++++++++++++++----- > drivers/net/virtio/virtio_pci.c | 42 +++++++++++++++++++++++++ > drivers/net/virtio/virtio_pci.h | 5 +++ > 3 files changed, 104 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > index d2576d5e0..525cfa06c 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,44 @@ 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; > + int msix_detect; As I don't think we need to return a value except 0, 1, 2 as my comment at the end, so I suppose we just: hw->use_msix = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev)); > + > + if (rte_intr_enable(dev->intr_handle) < 0) > + return -1; > + > + msix_detect = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev)); This breaks virtio_user. Need to add a check before this line. > + if (msix_detect < 0) > + return -1; > + else if (msix_detect == SUPPORT_MSIX_STATUS_ENABLED) > + hw->use_msix = 1; > + else > + hw->use_msix = 0; > + return 0; > +} > + > +static int > +virtio_intr_disable(struct rte_eth_dev *dev) > +{ > + struct virtio_hw *hw = dev->data->dev_private; > + int msix_detect; > + > + if (rte_intr_disable(dev->intr_handle) < 0) > + return -1; > + > + msix_detect = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(dev)); Ditto. > + if (msix_detect < 0) > + return -1; > + else if (msix_detect == SUPPORT_MSIX_STATUS_ENABLED) > + hw->use_msix = 1; > + else > + hw->use_msix = 0; > + return 0; > +} > + > +static int > virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features) > { > uint64_t host_features; > @@ -1228,7 +1269,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 +1389,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; > } > @@ -1370,7 +1411,15 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features) > struct virtio_net_config local_config; > struct rte_pci_device *pci_dev = NULL; > int ret; > + int msix_detect; > > + msix_detect = vtpci_msix_detect(RTE_ETH_DEV_TO_PCI(eth_dev)); > + if (msix_detect < 0) > + return -1; > + else if (msix_detect == SUPPORT_MSIX_STATUS_ENABLED) > + hw->use_msix = 1; > + else > + hw->use_msix = 0; Ditto, we directly assign return value to hw->use_msix. > /* Reset the device although not necessary at startup */ > vtpci_reset(hw); > > @@ -1388,7 +1437,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) && > + (msix_detect >= SUPPORT_MSIX_STATUS_DISABLED)) != should be better than >= here. > eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC; > else > eth_dev->data->dev_flags &= ~RTE_ETH_DEV_INTR_LSC; > @@ -1801,9 +1851,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 +1962,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..a73b41765 100644 > --- a/drivers/net/virtio/virtio_pci.c > +++ b/drivers/net/virtio/virtio_pci.c > @@ -710,3 +710,45 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw) > > return 0; > } > + > +/* Return -1 fails to detect msix. I don't think we need to return -1. We can treat the error as msix not supported. > + * Return 0 (NO_SUPPORT_MSIX) don't support msix. > + * Return 1 (SUPPORT_MSIX_STATUS_DISABLED) support msix, status: disabled. > + * Return 2 (SUPPORT_MSIX_STATUS_ENABLED) sppport msix, status: enabled. > + */ > + > +int > +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 -1; I suppose we can return NO_SUPPORT_MSIX here. > + } > + > + 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 SUPPORT_MSIX_STATUS_ENABLED; > + else > + return SUPPORT_MSIX_STATUS_DISABLED; > + } > + > + pos = cap.cap_next; > + } > + > + return NO_SUPPORT_MSIX; > +} > diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h > index 36d452c06..401afc7f9 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 NO_SUPPORT_MSIX 0 > +#define SUPPORT_MSIX_STATUS_DISABLED 1 > +#define SUPPORT_MSIX_STATUS_ENABLED 2 Maybe we refine the name as: VIRTIO_MSIX_NONE VIRTIO_MSIX_DISABLED VIRTIO_MSIX_ENABLED > + > static inline int > vtpci_with_feature(struct virtio_hw *hw, uint64_t bit) > { > @@ -325,6 +329,7 @@ vtpci_with_feature(struct virtio_hw *hw, uint64_t bit) > */ > int vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw); > void vtpci_reset(struct virtio_hw *); > +int vtpci_msix_detect(struct rte_pci_device *dev); > > void vtpci_reinit_complete(struct virtio_hw *); >