diff mbox

[dpdk-dev,v2,04/24] virtio: Add support for Link State interrupt

Message ID 1422326164-13697-5-git-send-email-changchun.ouyang@intel.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Ouyang Changchun Jan. 27, 2015, 2:35 a.m. UTC
Virtio has link state interrupt which can be used.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
 lib/librte_pmd_virtio/virtio_ethdev.c | 78 +++++++++++++++++++++++++++--------
 lib/librte_pmd_virtio/virtio_pci.c    | 22 ++++++++++
 lib/librte_pmd_virtio/virtio_pci.h    |  4 ++
 3 files changed, 86 insertions(+), 18 deletions(-)

Comments

Huawei Xie Jan. 27, 2015, 9:04 a.m. UTC | #1
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang Changchun
> Sent: Tuesday, January 27, 2015 10:36 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 04/24] virtio: Add support for Link State interrupt
> 
> Virtio has link state interrupt which can be used.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
>  lib/librte_pmd_virtio/virtio_ethdev.c | 78 +++++++++++++++++++++++++++------
> --
>  lib/librte_pmd_virtio/virtio_pci.c    | 22 ++++++++++
>  lib/librte_pmd_virtio/virtio_pci.h    |  4 ++
>  3 files changed, 86 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> b/lib/librte_pmd_virtio/virtio_ethdev.c
> index 5df3b54..ef87ff8 100644
> --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> @@ -845,6 +845,34 @@ static int virtio_resource_init(struct rte_pci_device
> *pci_dev __rte_unused)
>  #endif
> 
>  /*
> + * Process Virtio Config changed interrupt and call the callback
> + * if link state changed.
> + */
> +static void
> +virtio_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
> +			 void *param)
> +{
> +	struct rte_eth_dev *dev = param;
> +	struct virtio_hw *hw =
> +		VIRTIO_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	uint8_t isr;
> +
> +	/* Read interrupt status which clears interrupt */
> +	isr = vtpci_isr(hw);
> +	PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> +
> +	if (rte_intr_enable(&dev->pci_dev->intr_handle) < 0)
> +		PMD_DRV_LOG(ERR, "interrupt enable failed");
> +

Is it better to put rte_intr_enable after we have handled the interrupt.
Is there the possibility of interrupt reentrant in uio intr framework?

> +	if (isr & VIRTIO_PCI_ISR_CONFIG) {
> +		if (virtio_dev_link_update(dev, 0) == 0)
> +			_rte_eth_dev_callback_process(dev,
> +
> RTE_ETH_EVENT_INTR_LSC);
> +	}
> +
> +}
> +
>
Stephen Hemminger Jan. 27, 2015, 10 a.m. UTC | #2
On Tue, 27 Jan 2015 09:04:07 +0000
"Xie, Huawei" <huawei.xie@intel.com> wrote:

> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang Changchun
> > Sent: Tuesday, January 27, 2015 10:36 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 04/24] virtio: Add support for Link State interrupt
> > 
> > Virtio has link state interrupt which can be used.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > ---
> >  lib/librte_pmd_virtio/virtio_ethdev.c | 78 +++++++++++++++++++++++++++------
> > --
> >  lib/librte_pmd_virtio/virtio_pci.c    | 22 ++++++++++
> >  lib/librte_pmd_virtio/virtio_pci.h    |  4 ++
> >  3 files changed, 86 insertions(+), 18 deletions(-)
> > 
> > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> > b/lib/librte_pmd_virtio/virtio_ethdev.c
> > index 5df3b54..ef87ff8 100644
> > --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> > @@ -845,6 +845,34 @@ static int virtio_resource_init(struct rte_pci_device
> > *pci_dev __rte_unused)
> >  #endif
> > 
> >  /*
> > + * Process Virtio Config changed interrupt and call the callback
> > + * if link state changed.
> > + */
> > +static void
> > +virtio_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
> > +			 void *param)
> > +{
> > +	struct rte_eth_dev *dev = param;
> > +	struct virtio_hw *hw =
> > +		VIRTIO_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +	uint8_t isr;
> > +
> > +	/* Read interrupt status which clears interrupt */
> > +	isr = vtpci_isr(hw);
> > +	PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> > +
> > +	if (rte_intr_enable(&dev->pci_dev->intr_handle) < 0)
> > +		PMD_DRV_LOG(ERR, "interrupt enable failed");
> > +  
> 
> Is it better to put rte_intr_enable after we have handled the interrupt.
> Is there the possibility of interrupt reentrant in uio intr framework?

The UIO framework handles IRQ's via posix thread that is reading
fd, then calling this code. Therefore it is always single threaded.
Ouyang Changchun Jan. 28, 2015, 3:03 a.m. UTC | #3
Hi Stephen,

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, January 27, 2015 6:00 PM
> To: Xie, Huawei
> Cc: Ouyang, Changchun; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 04/24] virtio: Add support for Link State
> interrupt
> 
> On Tue, 27 Jan 2015 09:04:07 +0000
> "Xie, Huawei" <huawei.xie@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang
> > > Changchun
> > > Sent: Tuesday, January 27, 2015 10:36 AM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v2 04/24] virtio: Add support for Link
> > > State interrupt
> > >
> > > Virtio has link state interrupt which can be used.
> > >
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > ---
> > >  lib/librte_pmd_virtio/virtio_ethdev.c | 78
> > > +++++++++++++++++++++++++++------
> > > --
> > >  lib/librte_pmd_virtio/virtio_pci.c    | 22 ++++++++++
> > >  lib/librte_pmd_virtio/virtio_pci.h    |  4 ++
> > >  3 files changed, 86 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> > > b/lib/librte_pmd_virtio/virtio_ethdev.c
> > > index 5df3b54..ef87ff8 100644
> > > --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> > > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> > > @@ -845,6 +845,34 @@ static int virtio_resource_init(struct
> > > rte_pci_device *pci_dev __rte_unused)  #endif
> > >
> > >  /*
> > > + * Process Virtio Config changed interrupt and call the callback
> > > + * if link state changed.
> > > + */
> > > +static void
> > > +virtio_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
> > > +			 void *param)
> > > +{
> > > +	struct rte_eth_dev *dev = param;
> > > +	struct virtio_hw *hw =
> > > +		VIRTIO_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > > +	uint8_t isr;
> > > +
> > > +	/* Read interrupt status which clears interrupt */
> > > +	isr = vtpci_isr(hw);
> > > +	PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> > > +
> > > +	if (rte_intr_enable(&dev->pci_dev->intr_handle) < 0)
> > > +		PMD_DRV_LOG(ERR, "interrupt enable failed");
> > > +
> >
> > Is it better to put rte_intr_enable after we have handled the interrupt.
> > Is there the possibility of interrupt reentrant in uio intr framework?
> 
> The UIO framework handles IRQ's via posix thread that is reading fd, then
> calling this code. Therefore it is always single threaded.

Even if it is under UIO framework, and always single threaded, 
How about move rte_intr_enable after the virtio_dev_link_update() and _rte_eth_dev_callback_process is called.
This make it more like interrupt handler in linux kernel.
What do you think of it?
Thanks
Changchun
Stephen Hemminger Jan. 28, 2015, 3:11 p.m. UTC | #4
On Wed, 28 Jan 2015 03:03:32 +0000
"Ouyang, Changchun" <changchun.ouyang@intel.com> wrote:

> Hi Stephen,
> 
> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, January 27, 2015 6:00 PM
> > To: Xie, Huawei
> > Cc: Ouyang, Changchun; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 04/24] virtio: Add support for Link State
> > interrupt
> > 
> > On Tue, 27 Jan 2015 09:04:07 +0000
> > "Xie, Huawei" <huawei.xie@intel.com> wrote:
> > 
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang
> > > > Changchun
> > > > Sent: Tuesday, January 27, 2015 10:36 AM
> > > > To: dev@dpdk.org
> > > > Subject: [dpdk-dev] [PATCH v2 04/24] virtio: Add support for Link
> > > > State interrupt
> > > >
> > > > Virtio has link state interrupt which can be used.
> > > >
> > > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > > ---
> > > >  lib/librte_pmd_virtio/virtio_ethdev.c | 78
> > > > +++++++++++++++++++++++++++------
> > > > --
> > > >  lib/librte_pmd_virtio/virtio_pci.c    | 22 ++++++++++
> > > >  lib/librte_pmd_virtio/virtio_pci.h    |  4 ++
> > > >  3 files changed, 86 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> > > > b/lib/librte_pmd_virtio/virtio_ethdev.c
> > > > index 5df3b54..ef87ff8 100644
> > > > --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> > > > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> > > > @@ -845,6 +845,34 @@ static int virtio_resource_init(struct
> > > > rte_pci_device *pci_dev __rte_unused)  #endif
> > > >
> > > >  /*
> > > > + * Process Virtio Config changed interrupt and call the callback
> > > > + * if link state changed.
> > > > + */
> > > > +static void
> > > > +virtio_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
> > > > +			 void *param)
> > > > +{
> > > > +	struct rte_eth_dev *dev = param;
> > > > +	struct virtio_hw *hw =
> > > > +		VIRTIO_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > > > +	uint8_t isr;
> > > > +
> > > > +	/* Read interrupt status which clears interrupt */
> > > > +	isr = vtpci_isr(hw);
> > > > +	PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
> > > > +
> > > > +	if (rte_intr_enable(&dev->pci_dev->intr_handle) < 0)
> > > > +		PMD_DRV_LOG(ERR, "interrupt enable failed");
> > > > +
> > >
> > > Is it better to put rte_intr_enable after we have handled the interrupt.
> > > Is there the possibility of interrupt reentrant in uio intr framework?
> > 
> > The UIO framework handles IRQ's via posix thread that is reading fd, then
> > calling this code. Therefore it is always single threaded.
> 
> Even if it is under UIO framework, and always single threaded, 
> How about move rte_intr_enable after the virtio_dev_link_update() and _rte_eth_dev_callback_process is called.
> This make it more like interrupt handler in linux kernel.
> What do you think of it?

I ordered the interrupt handling to match what happens in e1000/igb
handler. My concern is that interrupt was level (not edge triggered)
and another link transisition could occur and be missed.
diff mbox

Patch

diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index 5df3b54..ef87ff8 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -845,6 +845,34 @@  static int virtio_resource_init(struct rte_pci_device *pci_dev __rte_unused)
 #endif
 
 /*
+ * Process Virtio Config changed interrupt and call the callback
+ * if link state changed.
+ */
+static void
+virtio_interrupt_handler(__rte_unused struct rte_intr_handle *handle,
+			 void *param)
+{
+	struct rte_eth_dev *dev = param;
+	struct virtio_hw *hw =
+		VIRTIO_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	uint8_t isr;
+
+	/* Read interrupt status which clears interrupt */
+	isr = vtpci_isr(hw);
+	PMD_DRV_LOG(INFO, "interrupt status = %#x", isr);
+
+	if (rte_intr_enable(&dev->pci_dev->intr_handle) < 0)
+		PMD_DRV_LOG(ERR, "interrupt enable failed");
+
+	if (isr & VIRTIO_PCI_ISR_CONFIG) {
+		if (virtio_dev_link_update(dev, 0) == 0)
+			_rte_eth_dev_callback_process(dev,
+						      RTE_ETH_EVENT_INTR_LSC);
+	}
+
+}
+
+/*
  * This function is based on probe() function in virtio_pci.c
  * It returns 0 on success.
  */
@@ -968,6 +996,10 @@  eth_virtio_dev_init(__rte_unused struct eth_driver *eth_drv,
 	PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x",
 			eth_dev->data->port_id, pci_dev->id.vendor_id,
 			pci_dev->id.device_id);
+
+	/* Setup interrupt callback  */
+	rte_intr_callback_register(&pci_dev->intr_handle,
+				   virtio_interrupt_handler, eth_dev);
 	return 0;
 }
 
@@ -975,7 +1007,7 @@  static struct eth_driver rte_virtio_pmd = {
 	{
 		.name = "rte_virtio_pmd",
 		.id_table = pci_id_virtio_map,
-		.drv_flags = RTE_PCI_DRV_NEED_MAPPING,
+		.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
 	},
 	.eth_dev_init = eth_virtio_dev_init,
 	.dev_private_size = sizeof(struct virtio_adapter),
@@ -1021,6 +1053,9 @@  static int
 virtio_dev_configure(struct rte_eth_dev *dev)
 {
 	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+	struct virtio_hw *hw =
+		VIRTIO_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	int ret;
 
 	PMD_INIT_LOG(DEBUG, "configure");
 
@@ -1029,7 +1064,11 @@  virtio_dev_configure(struct rte_eth_dev *dev)
 		return (-EINVAL);
 	}
 
-	return 0;
+	ret = vtpci_irq_config(hw, 0);
+	if (ret != 0)
+		PMD_DRV_LOG(ERR, "failed to set config vector");
+
+	return ret;
 }
 
 
@@ -1037,7 +1076,6 @@  static int
 virtio_dev_start(struct rte_eth_dev *dev)
 {
 	uint16_t nb_queues, i;
-	uint16_t status;
 	struct virtio_hw *hw =
 		VIRTIO_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
@@ -1052,18 +1090,22 @@  virtio_dev_start(struct rte_eth_dev *dev)
 	/* Do final configuration before rx/tx engine starts */
 	virtio_dev_rxtx_start(dev);
 
-	/* Check VIRTIO_NET_F_STATUS for link status*/
-	if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
-		vtpci_read_dev_config(hw,
-				offsetof(struct virtio_net_config, status),
-				&status, sizeof(status));
-		if ((status & VIRTIO_NET_S_LINK_UP) == 0)
-			PMD_INIT_LOG(ERR, "Port: %d Link is DOWN",
-				     dev->data->port_id);
-		else
-			PMD_INIT_LOG(DEBUG, "Port: %d Link is UP",
-				     dev->data->port_id);
+	/* check if lsc interrupt feature is enabled */
+	if (dev->data->dev_conf.intr_conf.lsc) {
+		if (!vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
+			PMD_DRV_LOG(ERR, "link status not supported by host");
+			return -ENOTSUP;
+		}
+
+		if (rte_intr_enable(&dev->pci_dev->intr_handle) < 0) {
+			PMD_DRV_LOG(ERR, "interrupt enable failed");
+			return -EIO;
+		}
 	}
+
+	/* Initialize Link state */
+	virtio_dev_link_update(dev, 0);
+
 	vtpci_reinit_complete(hw);
 
 	/*Notify the backend
@@ -1145,6 +1187,7 @@  virtio_dev_stop(struct rte_eth_dev *dev)
 		VIRTIO_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	/* reset the NIC */
+	vtpci_irq_config(hw, 0);
 	vtpci_reset(hw);
 	virtio_dev_free_mbufs(dev);
 }
@@ -1161,6 +1204,7 @@  virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
 	old = link;
 	link.link_duplex = FULL_DUPLEX;
 	link.link_speed  = SPEED_10G;
+
 	if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
 		PMD_INIT_LOG(DEBUG, "Get link status from hw");
 		vtpci_read_dev_config(hw,
@@ -1179,10 +1223,8 @@  virtio_dev_link_update(struct rte_eth_dev *dev, __rte_unused int wait_to_complet
 		link.link_status = 1;   /* Link up */
 	}
 	virtio_dev_atomic_write_link_status(dev, &link);
-	if (old.link_status == link.link_status)
-		return -1;
-	/*changed*/
-	return 0;
+
+	return (old.link_status == link.link_status) ? -1 : 0;
 }
 
 static void
diff --git a/lib/librte_pmd_virtio/virtio_pci.c b/lib/librte_pmd_virtio/virtio_pci.c
index ca9c748..6d51032 100644
--- a/lib/librte_pmd_virtio/virtio_pci.c
+++ b/lib/librte_pmd_virtio/virtio_pci.c
@@ -127,3 +127,25 @@  vtpci_set_status(struct virtio_hw *hw, uint8_t status)
 
 	VIRTIO_WRITE_REG_1(hw, VIRTIO_PCI_STATUS, status);
 }
+
+uint8_t
+vtpci_isr(struct virtio_hw *hw)
+{
+
+	return VIRTIO_READ_REG_1(hw, VIRTIO_PCI_ISR);
+}
+
+
+/* Enable one vector (0) for Link State Intrerrupt */
+int
+vtpci_irq_config(struct virtio_hw *hw, uint16_t vec)
+{
+	VIRTIO_WRITE_REG_2(hw, VIRTIO_MSI_CONFIG_VECTOR, vec);
+	vec = VIRTIO_READ_REG_2(hw, VIRTIO_MSI_CONFIG_VECTOR);
+	if (vec == VIRTIO_MSI_NO_VECTOR) {
+		PMD_DRV_LOG(ERR, "failed to set config vector");
+		return -EBUSY;
+	}
+
+	return 0;
+}
diff --git a/lib/librte_pmd_virtio/virtio_pci.h b/lib/librte_pmd_virtio/virtio_pci.h
index 373f9dc..6998737 100644
--- a/lib/librte_pmd_virtio/virtio_pci.h
+++ b/lib/librte_pmd_virtio/virtio_pci.h
@@ -263,4 +263,8 @@  void vtpci_write_dev_config(struct virtio_hw *, uint64_t, void *, int);
 
 void vtpci_read_dev_config(struct virtio_hw *, uint64_t, void *, int);
 
+uint8_t vtpci_isr(struct virtio_hw *);
+
+int vtpci_irq_config(struct virtio_hw *, uint16_t);
+
 #endif /* _VIRTIO_PCI_H_ */