[dpdk-dev,v2,04/24] virtio: Add support for Link State interrupt
Commit Message
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
> -----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);
> + }
> +
> +}
> +
>
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.
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
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.
@@ -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
@@ -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;
+}
@@ -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_ */