[PATCH 0/5] virtio driver fixes and cleanup

[PATCH 0/5] virtio driver fixes and cleanup

[Stephen Hemminger]

These are cleanups and enhancements to virtio driver.
The important fixes are those that allow virtio to work in non-KVM
environments. The driver was not negotiating properly with the host.

Stephen Hemminger (5):
  virtio: remove useless new lines
  virtio: don't enable/disable rx modes unless supported
  virtio: don't set mac table unless negotiated
  virtio: fix ring size negotiation
  virtio: clarify feature bit handling

 lib/librte_pmd_virtio/virtio_ethdev.c | 70 ++++++++++++++-----------
 lib/librte_pmd_virtio/virtio_ethdev.h | 27 ++++------
 lib/librte_pmd_virtio/virtio_pci.h    | 96 ++++++++++++++++++-----------------
 lib/librte_pmd_virtio/virtqueue.h     |  8 +--
 4 files changed, 100 insertions(+), 101 deletions(-)

-- 
2.1.4

[PATCH 1/5] virtio: remove useless new lines

[Stephen Hemminger]

There are several places in virtio with extra newlines between
calling a function and checking the result.
Remove them to improve readability.

Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
---
 lib/librte_pmd_virtio/virtio_ethdev.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index ffa26a0..a38ceed 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -228,7 +228,6 @@ virtio_set_multiple_queues(struct rte_eth_dev *dev, uint16_t nb_queues)
 	dlen[0] = sizeof(uint16_t);
 
 	ret = virtio_send_command(hw->cvq, &ctrl, dlen, 1);
-
 	if (ret) {
 		PMD_INIT_LOG(ERR, "Multiqueue configured but send command "
 			  "failed, this is too late now...");
@@ -394,7 +393,6 @@ virtio_dev_cq_queue_setup(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx,
 	PMD_INIT_FUNC_TRACE();
 	ret = virtio_dev_queue_setup(dev, VTNET_CQ, VTNET_SQ_CQ_QUEUE_IDX,
 			vtpci_queue_idx, nb_desc, socket_id, &vq);
-
 	if (ret < 0) {
 		PMD_INIT_LOG(ERR, "control vq initialization failed");
 		return ret;
@@ -434,7 +432,6 @@ virtio_dev_promiscuous_enable(struct rte_eth_dev *dev)
 	dlen[0] = 1;
 
 	ret = virtio_send_command(hw->cvq, &ctrl, dlen, 1);
-
 	if (ret)
 		PMD_INIT_LOG(ERR, "Failed to enable promisc");
 }
@@ -453,7 +450,6 @@ virtio_dev_promiscuous_disable(struct rte_eth_dev *dev)
 	dlen[0] = 1;
 
 	ret = virtio_send_command(hw->cvq, &ctrl, dlen, 1);
-
 	if (ret)
 		PMD_INIT_LOG(ERR, "Failed to disable promisc");
 }
@@ -472,7 +468,6 @@ virtio_dev_allmulticast_enable(struct rte_eth_dev *dev)
 	dlen[0] = 1;
 
 	ret = virtio_send_command(hw->cvq, &ctrl, dlen, 1);
-
 	if (ret)
 		PMD_INIT_LOG(ERR, "Failed to enable allmulticast");
 }
@@ -491,7 +486,6 @@ virtio_dev_allmulticast_disable(struct rte_eth_dev *dev)
 	dlen[0] = 1;
 
 	ret = virtio_send_command(hw->cvq, &ctrl, dlen, 1);
-
 	if (ret)
 		PMD_INIT_LOG(ERR, "Failed to disable allmulticast");
 }
-- 
2.1.4

Re: [PATCH 1/5] virtio: remove useless new lines

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Stephen
> Hemminger
> Sent: Wednesday, April 15, 2015 11:20 PM
> To: dev-VfR2kkLFssw@public.gmane.org
> Subject: [dpdk-dev] [PATCH 1/5] virtio: remove useless new lines
> 
> There are several places in virtio with extra newlines between calling a
> function and checking the result.
> Remove them to improve readability.
> 
> Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>

Acked-by: Changchun Ouyang <changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

[PATCH 2/5] virtio: don't enable/disable rx modes unless supported

[Stephen Hemminger]

Don't try to set features related to receiving unless the
appropriate feature bit has ben negotiated with the host.

This solves some of the issues when using virtio on non-KVM/QEMU
hypervisors.

Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
---
 lib/librte_pmd_virtio/virtio_ethdev.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index a38ceed..f0859d8 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -426,6 +426,11 @@ virtio_dev_promiscuous_enable(struct rte_eth_dev *dev)
 	int dlen[1];
 	int ret;
 
+	if (!vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_RX)) {
+		PMD_INIT_LOG(INFO, "host does not support rx control\n");
+		return;
+	}
+
 	ctrl.hdr.class = VIRTIO_NET_CTRL_RX;
 	ctrl.hdr.cmd = VIRTIO_NET_CTRL_RX_PROMISC;
 	ctrl.data[0] = 1;
@@ -444,6 +449,11 @@ virtio_dev_promiscuous_disable(struct rte_eth_dev *dev)
 	int dlen[1];
 	int ret;
 
+	if (!vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_RX)) {
+		PMD_INIT_LOG(INFO, "host does not support rx control\n");
+		return;
+	}
+
 	ctrl.hdr.class = VIRTIO_NET_CTRL_RX;
 	ctrl.hdr.cmd = VIRTIO_NET_CTRL_RX_PROMISC;
 	ctrl.data[0] = 0;
@@ -462,6 +472,11 @@ virtio_dev_allmulticast_enable(struct rte_eth_dev *dev)
 	int dlen[1];
 	int ret;
 
+	if (!vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_RX)) {
+		PMD_INIT_LOG(INFO, "host does not support rx control\n");
+		return;
+	}
+
 	ctrl.hdr.class = VIRTIO_NET_CTRL_RX;
 	ctrl.hdr.cmd = VIRTIO_NET_CTRL_RX_ALLMULTI;
 	ctrl.data[0] = 1;
@@ -480,6 +495,11 @@ virtio_dev_allmulticast_disable(struct rte_eth_dev *dev)
 	int dlen[1];
 	int ret;
 
+	if (!vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_RX)) {
+		PMD_INIT_LOG(INFO, "host does not support rx control\n");
+		return;
+	}
+
 	ctrl.hdr.class = VIRTIO_NET_CTRL_RX;
 	ctrl.hdr.cmd = VIRTIO_NET_CTRL_RX_ALLMULTI;
 	ctrl.data[0] = 0;
-- 
2.1.4

Re: [PATCH 2/5] virtio: don't enable/disable rx modes unless supported

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Stephen
> Hemminger
> Sent: Wednesday, April 15, 2015 11:20 PM
> To: dev-VfR2kkLFssw@public.gmane.org
> Subject: [dpdk-dev] [PATCH 2/5] virtio: don't enable/disable rx modes unless
> supported
> 
> Don't try to set features related to receiving unless the appropriate feature
> bit has ben negotiated with the host.
> 
> This solves some of the issues when using virtio on non-KVM/QEMU
> hypervisors.
> 
> Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>

Acked-by: Changchun Ouyang <changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

[PATCH 3/5] virtio: don't set mac table unless negotiated

[Stephen Hemminger]

Don't attempt to set the MAC address table unless the host allows it.
Also, don't return a value from mac_table_set since all callers
ignore the return value.

Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
---
 lib/librte_pmd_virtio/virtio_ethdev.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index f0859d8..3cb9c6a 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -668,7 +668,7 @@ virtio_get_hwaddr(struct virtio_hw *hw)
 	}
 }
 
-static int
+static void
 virtio_mac_table_set(struct virtio_hw *hw,
 		     const struct virtio_net_ctrl_mac *uc,
 		     const struct virtio_net_ctrl_mac *mc)
@@ -676,6 +676,11 @@ virtio_mac_table_set(struct virtio_hw *hw,
 	struct virtio_pmd_ctrl ctrl;
 	int err, len[2];
 
+	if (!vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
+		PMD_DRV_LOG(INFO, "host does not support mac table\n");
+		return;
+	}
+
 	ctrl.hdr.class = VIRTIO_NET_CTRL_MAC;
 	ctrl.hdr.cmd = VIRTIO_NET_CTRL_MAC_TABLE_SET;
 
@@ -688,8 +693,6 @@ virtio_mac_table_set(struct virtio_hw *hw,
 	err = virtio_send_command(hw->cvq, &ctrl, len, 2);
 	if (err != 0)
 		PMD_DRV_LOG(NOTICE, "mac table set failed: %d", err);
-
-	return err;
 }
 
 static void
-- 
2.1.4

Re: [PATCH 3/5] virtio: don't set mac table unless negotiated

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Stephen
> Hemminger
> Sent: Wednesday, April 15, 2015 11:20 PM
> To: dev-VfR2kkLFssw@public.gmane.org
> Subject: [dpdk-dev] [PATCH 3/5] virtio: don't set mac table unless negotiated
> 
> Don't attempt to set the MAC address table unless the host allows it.
> Also, don't return a value from mac_table_set since all callers ignore the
> return value.
> 
> Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>

Acked-by: Changchun Ouyang <changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

[PATCH 4/5] virtio: fix ring size negotiation

[Stephen Hemminger]

This fixes another of the issues with running virtio on non-KVM
envirionments. For example, Google Compute Engine reports a
ring size of 16K.

If guest virtio requests more slots than available then
the queue should just be initialized to the smaller value.

Conversely, if the number of descriptors requested exceeds the virtio
host queue size, then just silently use the smaller host size.

Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
---
 lib/librte_pmd_virtio/virtio_ethdev.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index 3cb9c6a..db0232e 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -267,13 +267,21 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 	if (vq_size == 0) {
 		PMD_INIT_LOG(ERR, "%s: virtqueue does not exist", __func__);
 		return -EINVAL;
-	} else if (!rte_is_power_of_2(vq_size)) {
+	}
+
+	if (!rte_is_power_of_2(vq_size)) {
 		PMD_INIT_LOG(ERR, "%s: virtqueue size is not powerof 2", __func__);
 		return -EINVAL;
-	} else if (nb_desc != vq_size) {
-		PMD_INIT_LOG(ERR, "Warning: nb_desc(%d) is not equal to vq size (%d), fall to vq size",
-			nb_desc, vq_size);
-		nb_desc = vq_size;
+	}
+
+	if (nb_desc < vq_size) {
+		if (!rte_is_power_of_2(nb_desc)) {
+			PMD_INIT_LOG(ERR,
+				     "nb_desc(%u) size is not powerof 2",
+				     nb_desc);
+			return -EINVAL;
+		}
+		vq_size = nb_desc;
 	}
 
 	if (queue_type == VTNET_RQ) {
-- 
2.1.4

Re: [PATCH 4/5] virtio: fix ring size negotiation

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Stephen
> Hemminger
> Sent: Wednesday, April 15, 2015 11:20 PM
> To: dev-VfR2kkLFssw@public.gmane.org
> Subject: [dpdk-dev] [PATCH 4/5] virtio: fix ring size negotiation
> 
> This fixes another of the issues with running virtio on non-KVM
> envirionments. For example, Google Compute Engine reports a ring size of
> 16K.
> 
> If guest virtio requests more slots than available then the queue should just

I suspect 'more' here should be 'less'?

> be initialized to the smaller value.
> 
> Conversely, if the number of descriptors requested exceeds the virtio host
> queue size, then just silently use the smaller host size.
> 
> Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
> ---
>  lib/librte_pmd_virtio/virtio_ethdev.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> b/lib/librte_pmd_virtio/virtio_ethdev.c
> index 3cb9c6a..db0232e 100644
> --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> @@ -267,13 +267,21 @@ int virtio_dev_queue_setup(struct rte_eth_dev
> *dev,
>  	if (vq_size == 0) {
>  		PMD_INIT_LOG(ERR, "%s: virtqueue does not exist",
> __func__);
>  		return -EINVAL;
> -	} else if (!rte_is_power_of_2(vq_size)) {
> +	}
> +
> +	if (!rte_is_power_of_2(vq_size)) {
>  		PMD_INIT_LOG(ERR, "%s: virtqueue size is not powerof 2",
> __func__);
>  		return -EINVAL;
> -	} else if (nb_desc != vq_size) {
> -		PMD_INIT_LOG(ERR, "Warning: nb_desc(%d) is not equal to
> vq size (%d), fall to vq size",
> -			nb_desc, vq_size);
> -		nb_desc = vq_size;
> +	}
> +
> +	if (nb_desc < vq_size) {
> +		if (!rte_is_power_of_2(nb_desc)) {
> +			PMD_INIT_LOG(ERR,
> +				     "nb_desc(%u) size is not powerof 2",
> +				     nb_desc);
> +			return -EINVAL;
> +		}
> +		vq_size = nb_desc;

Don't we need a warning when nb_desc > vq_size?

>  	}
> 
>  	if (queue_type == VTNET_RQ) {
> --
> 2.1.4

Re: [PATCH 4/5] virtio: fix ring size negotiation

[Stephen Hemminger]

No warning is needed, it just works.

On Wed, Apr 15, 2015 at 8:39 PM, Ouyang, Changchun <
changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Stephen
> > Hemminger
> > Sent: Wednesday, April 15, 2015 11:20 PM
> > To: dev-VfR2kkLFssw@public.gmane.org
> > Subject: [dpdk-dev] [PATCH 4/5] virtio: fix ring size negotiation
> >
> > This fixes another of the issues with running virtio on non-KVM
> > envirionments. For example, Google Compute Engine reports a ring size of
> > 16K.
> >
> > If guest virtio requests more slots than available then the queue should
> just
>
> I suspect 'more' here should be 'less'?
>
> > be initialized to the smaller value.
> >
> > Conversely, if the number of descriptors requested exceeds the virtio
> host
> > queue size, then just silently use the smaller host size.
> >
> > Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
> > ---
> >  lib/librte_pmd_virtio/virtio_ethdev.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> > b/lib/librte_pmd_virtio/virtio_ethdev.c
> > index 3cb9c6a..db0232e 100644
> > --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> > @@ -267,13 +267,21 @@ int virtio_dev_queue_setup(struct rte_eth_dev
> > *dev,
> >       if (vq_size == 0) {
> >               PMD_INIT_LOG(ERR, "%s: virtqueue does not exist",
> > __func__);
> >               return -EINVAL;
> > -     } else if (!rte_is_power_of_2(vq_size)) {
> > +     }
> > +
> > +     if (!rte_is_power_of_2(vq_size)) {
> >               PMD_INIT_LOG(ERR, "%s: virtqueue size is not powerof 2",
> > __func__);
> >               return -EINVAL;
> > -     } else if (nb_desc != vq_size) {
> > -             PMD_INIT_LOG(ERR, "Warning: nb_desc(%d) is not equal to
> > vq size (%d), fall to vq size",
> > -                     nb_desc, vq_size);
> > -             nb_desc = vq_size;
> > +     }
> > +
> > +     if (nb_desc < vq_size) {
> > +             if (!rte_is_power_of_2(nb_desc)) {
> > +                     PMD_INIT_LOG(ERR,
> > +                                  "nb_desc(%u) size is not powerof 2",
> > +                                  nb_desc);
> > +                     return -EINVAL;
> > +             }
> > +             vq_size = nb_desc;
>
> Don't we need a warning when nb_desc > vq_size?
>
> >       }
> >
> >       if (queue_type == VTNET_RQ) {
> > --
> > 2.1.4
>
>

Re: [PATCH 4/5] virtio: fix ring size negotiation

[Ouyang, Changchun]

From: Stephen Hemminger [mailto:stephen@networkplumber.org]
Sent: Thursday, April 16, 2015 1:48 PM
To: Ouyang, Changchun
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 4/5] virtio: fix ring size negotiation

No warning is needed, it just works.

I know it works, but the upper user don’t know the descriptor number is reduced.
I concern it is not so user-friendly here.


On Wed, Apr 15, 2015 at 8:39 PM, Ouyang, Changchun <changchun.ouyang@intel.com<mailto:changchun.ouyang@intel.com>> wrote:


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org<mailto:dev-bounces@dpdk.org>] On Behalf Of Stephen
> Hemminger
> Sent: Wednesday, April 15, 2015 11:20 PM
> To: dev@dpdk.org<mailto:dev@dpdk.org>
> Subject: [dpdk-dev] [PATCH 4/5] virtio: fix ring size negotiation
>
> This fixes another of the issues with running virtio on non-KVM
> envirionments. For example, Google Compute Engine reports a ring size of
> 16K.
>
> If guest virtio requests more slots than available then the queue should just

I suspect 'more' here should be 'less'?

> be initialized to the smaller value.
>
> Conversely, if the number of descriptors requested exceeds the virtio host
> queue size, then just silently use the smaller host size.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org<mailto:stephen@networkplumber.org>>
> ---
>  lib/librte_pmd_virtio/virtio_ethdev.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> b/lib/librte_pmd_virtio/virtio_ethdev.c
> index 3cb9c6a..db0232e 100644
> --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> @@ -267,13 +267,21 @@ int virtio_dev_queue_setup(struct rte_eth_dev
> *dev,
>       if (vq_size == 0) {
>               PMD_INIT_LOG(ERR, "%s: virtqueue does not exist",
> __func__);
>               return -EINVAL;
> -     } else if (!rte_is_power_of_2(vq_size)) {
> +     }
> +
> +     if (!rte_is_power_of_2(vq_size)) {
>               PMD_INIT_LOG(ERR, "%s: virtqueue size is not powerof 2",
> __func__);
>               return -EINVAL;
> -     } else if (nb_desc != vq_size) {
> -             PMD_INIT_LOG(ERR, "Warning: nb_desc(%d) is not equal to
> vq size (%d), fall to vq size",
> -                     nb_desc, vq_size);
> -             nb_desc = vq_size;
> +     }
> +
> +     if (nb_desc < vq_size) {
> +             if (!rte_is_power_of_2(nb_desc)) {
> +                     PMD_INIT_LOG(ERR,
> +                                  "nb_desc(%u) size is not powerof 2",
> +                                  nb_desc);
> +                     return -EINVAL;
> +             }
> +             vq_size = nb_desc;
Don't we need a warning when nb_desc > vq_size?

>       }
>
>       if (queue_type == VTNET_RQ) {
> --
> 2.1.4

Re: [PATCH 4/5] virtio: fix ring size negotiation

[Thomas Monjalon]

Guys, this is an example of what should not be done with emails formatting.

2015-04-16 06:26, Ouyang, Changchun:
> 
> From: Stephen Hemminger [mailto:stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org]
> Sent: Thursday, April 16, 2015 1:48 PM
> To: Ouyang, Changchun
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH 4/5] virtio: fix ring size negotiation

This header is partly useless.

> 
> No warning is needed, it just works.

Stephen, please do not top post. It makes harder to find what you are commenting.

> 
> I know it works, but the upper user don’t know the descriptor number is reduced.
> I concern it is not so user-friendly here.

Changchun, please be sure the above text is quoted.
How are we supposed to understand who is speaking here?

All, please, take care of readers.
Thanks

> On Wed, Apr 15, 2015 at 8:39 PM, Ouyang, Changchun <changchun.ouyang@intel.com<mailto:changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org<mailto:dev-bounces-VfR2kkLFssw@public.gmane.org>] On Behalf Of Stephen
> > Hemminger
> > Sent: Wednesday, April 15, 2015 11:20 PM
> > To: dev-VfR2kkLFssw@public.gmane.org<mailto:dev-VfR2kkLFssw@public.gmane.org>
> > Subject: [dpdk-dev] [PATCH 4/5] virtio: fix ring size negotiation
> >
> > This fixes another of the issues with running virtio on non-KVM
> > envirionments. For example, Google Compute Engine reports a ring size of
> > 16K.
> >
> > If guest virtio requests more slots than available then the queue should just
> 
> I suspect 'more' here should be 'less'?
> 
> > be initialized to the smaller value.
> >
> > Conversely, if the number of descriptors requested exceeds the virtio host
> > queue size, then just silently use the smaller host size.
> >
> > Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org<mailto:stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>>
> > ---
> >  lib/librte_pmd_virtio/virtio_ethdev.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> > b/lib/librte_pmd_virtio/virtio_ethdev.c
> > index 3cb9c6a..db0232e 100644
> > --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> > @@ -267,13 +267,21 @@ int virtio_dev_queue_setup(struct rte_eth_dev
> > *dev,
> >       if (vq_size == 0) {
> >               PMD_INIT_LOG(ERR, "%s: virtqueue does not exist",
> > __func__);
> >               return -EINVAL;
> > -     } else if (!rte_is_power_of_2(vq_size)) {
> > +     }
> > +
> > +     if (!rte_is_power_of_2(vq_size)) {
> >               PMD_INIT_LOG(ERR, "%s: virtqueue size is not powerof 2",
> > __func__);
> >               return -EINVAL;
> > -     } else if (nb_desc != vq_size) {
> > -             PMD_INIT_LOG(ERR, "Warning: nb_desc(%d) is not equal to
> > vq size (%d), fall to vq size",
> > -                     nb_desc, vq_size);
> > -             nb_desc = vq_size;
> > +     }
> > +
> > +     if (nb_desc < vq_size) {
> > +             if (!rte_is_power_of_2(nb_desc)) {
> > +                     PMD_INIT_LOG(ERR,
> > +                                  "nb_desc(%u) size is not powerof 2",
> > +                                  nb_desc);
> > +                     return -EINVAL;
> > +             }
> > +             vq_size = nb_desc;
> Don't we need a warning when nb_desc > vq_size?
> 
> >       }
> >
> >       if (queue_type == VTNET_RQ) {
> > --
> > 2.1.4
> 
> 

Re: [PATCH 4/5] virtio: fix ring size negotiation

[Stephen Hemminger]

On Thu, 16 Apr 2015 09:38:46 +0200
Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org> wrote:

> Guys, this is an example of what should not be done with emails formatting.

Sorry, Google mail client for Android seems to encourage
bad formatting.

Re: [PATCH 4/5] virtio: fix ring size negotiation

[Stephen Hemminger]

On Thu, 16 Apr 2015 06:26:02 +0000
"Ouyang, Changchun" <changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> 
> 
> From: Stephen Hemminger [mailto:stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org]
> Sent: Thursday, April 16, 2015 1:48 PM
> To: Ouyang, Changchun
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] [PATCH 4/5] virtio: fix ring size negotiation
> 
> No warning is needed, it just works.
> 
> I know it works, but the upper user don’t know the descriptor number is reduced.
> I concern it is not so user-friendly here.
> 
> 
> On Wed, Apr 15, 2015 at 8:39 PM, Ouyang, Changchun <changchun.ouyang@intel.com<mailto:changchun.ouyang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>> wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org<mailto:dev-bounces-VfR2kkLFssw@public.gmane.org>] On Behalf Of Stephen
> > Hemminger
> > Sent: Wednesday, April 15, 2015 11:20 PM
> > To: dev-VfR2kkLFssw@public.gmane.org<mailto:dev-VfR2kkLFssw@public.gmane.org>
> > Subject: [dpdk-dev] [PATCH 4/5] virtio: fix ring size negotiation
> >
> > This fixes another of the issues with running virtio on non-KVM
> > envirionments. For example, Google Compute Engine reports a ring size of
> > 16K.
> >
> > If guest virtio requests more slots than available then the queue should just
> 
> I suspect 'more' here should be 'less'?
> 
> > be initialized to the smaller value.
> >
> > Conversely, if the number of descriptors requested exceeds the virtio host
> > queue size, then just silently use the smaller host size.
> >
> > Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org<mailto:stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>>
> > ---
> >  lib/librte_pmd_virtio/virtio_ethdev.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c
> > b/lib/librte_pmd_virtio/virtio_ethdev.c
> > index 3cb9c6a..db0232e 100644
> > --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> > +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> > @@ -267,13 +267,21 @@ int virtio_dev_queue_setup(struct rte_eth_dev
> > *dev,
> >       if (vq_size == 0) {
> >               PMD_INIT_LOG(ERR, "%s: virtqueue does not exist",
> > __func__);
> >               return -EINVAL;
> > -     } else if (!rte_is_power_of_2(vq_size)) {
> > +     }
> > +
> > +     if (!rte_is_power_of_2(vq_size)) {
> >               PMD_INIT_LOG(ERR, "%s: virtqueue size is not powerof 2",
> > __func__);
> >               return -EINVAL;
> > -     } else if (nb_desc != vq_size) {
> > -             PMD_INIT_LOG(ERR, "Warning: nb_desc(%d) is not equal to
> > vq size (%d), fall to vq size",
> > -                     nb_desc, vq_size);
> > -             nb_desc = vq_size;
> > +     }
> > +
> > +     if (nb_desc < vq_size) {
> > +             if (!rte_is_power_of_2(nb_desc)) {
> > +                     PMD_INIT_LOG(ERR,
> > +                                  "nb_desc(%u) size is not powerof 2",
> > +                                  nb_desc);
> > +                     return -EINVAL;
> > +             }
> > +             vq_size = nb_desc;
> Don't we need a warning when nb_desc > vq_size?


No warning is needed. This will actually be a common case
for many applications.  

IMHO application should not have to worry about
what type of network device it is running on and therefore would likely
pass a reasonably large number of receive descriptors (say 512) and since
the default KVM/QEMU ring size is 256, the receive queue would be limited
by host not application.

The whole idea of application passing number of receive descriptors to
DPDK is bogus because each device driver has different timing, and may
use different number of receive descriptors per packet.

[PATCH 5/5] virtio: clarify feature bit handling

[Stephen Hemminger]

Change the features from bit mask to bit number. This allows the
DPDK driver to use the definitions from Linux[1]. This makes doing
enhancements to DPDK driver easier when new feature bits are added.

More importantly, get rid of the confusing feature bit initialization
code. Remove the double negative code in the feature masking.
Instead just have a new define with the list of feature bits implemented.

Signed-off-by: Stephen Hemminger <stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ@public.gmane.org>
---
 lib/librte_pmd_virtio/virtio_ethdev.c | 17 +------
 lib/librte_pmd_virtio/virtio_ethdev.h | 27 ++++------
 lib/librte_pmd_virtio/virtio_pci.h    | 96 ++++++++++++++++++-----------------
 lib/librte_pmd_virtio/virtqueue.h     |  8 +--
 4 files changed, 61 insertions(+), 87 deletions(-)

diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
index db0232e..349b73b 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -807,23 +807,10 @@ virtio_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on)
 static void
 virtio_negotiate_features(struct virtio_hw *hw)
 {
-	uint32_t host_features, mask;
-
-	/* checksum offload not implemented */
-	mask = VIRTIO_NET_F_CSUM | VIRTIO_NET_F_GUEST_CSUM;
-
-	/* TSO and LRO are only available when their corresponding
-	 * checksum offload feature is also negotiated.
-	 */
-	mask |= VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 | VIRTIO_NET_F_HOST_ECN;
-	mask |= VIRTIO_NET_F_GUEST_TSO4 | VIRTIO_NET_F_GUEST_TSO6 | VIRTIO_NET_F_GUEST_ECN;
-	mask |= VTNET_LRO_FEATURES;
-
-	/* not negotiating INDIRECT descriptor table support */
-	mask |= VIRTIO_RING_F_INDIRECT_DESC;
+	uint32_t host_features;
 
 	/* Prepare guest_features: feature that driver wants to support */
-	hw->guest_features = VTNET_FEATURES & ~mask;
+	hw->guest_features = VIRTIO_PMD_GUEST_FEATURES;
 	PMD_INIT_LOG(DEBUG, "guest_features before negotiate = %x",
 		hw->guest_features);
 
diff --git a/lib/librte_pmd_virtio/virtio_ethdev.h b/lib/librte_pmd_virtio/virtio_ethdev.h
index e6d4533..df2cb7d 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.h
+++ b/lib/librte_pmd_virtio/virtio_ethdev.h
@@ -56,24 +56,15 @@
 #define VIRTIO_MAX_RX_PKTLEN  9728
 
 /* Features desired/implemented by this driver. */
-#define VTNET_FEATURES \
-	(VIRTIO_NET_F_MAC       | \
-	VIRTIO_NET_F_STATUS     | \
-	VIRTIO_NET_F_MQ         | \
-	VIRTIO_NET_F_CTRL_MAC_ADDR | \
-	VIRTIO_NET_F_CTRL_VQ    | \
-	VIRTIO_NET_F_CTRL_RX    | \
-	VIRTIO_NET_F_CTRL_VLAN  | \
-	VIRTIO_NET_F_CSUM       | \
-	VIRTIO_NET_F_HOST_TSO4  | \
-	VIRTIO_NET_F_HOST_TSO6  | \
-	VIRTIO_NET_F_HOST_ECN   | \
-	VIRTIO_NET_F_GUEST_CSUM | \
-	VIRTIO_NET_F_GUEST_TSO4 | \
-	VIRTIO_NET_F_GUEST_TSO6 | \
-	VIRTIO_NET_F_GUEST_ECN  | \
-	VIRTIO_NET_F_MRG_RXBUF  | \
-	VIRTIO_RING_F_INDIRECT_DESC)
+#define VIRTIO_PMD_GUEST_FEATURES		\
+	(1u << VIRTIO_NET_F_MAC		  |	\
+	 1u << VIRTIO_NET_F_STATUS	  |	\
+	 1u << VIRTIO_NET_F_MQ		  |	\
+	 1u << VIRTIO_NET_F_CTRL_MAC_ADDR |	\
+	 1u << VIRTIO_NET_F_CTRL_VQ	  |	\
+	 1u << VIRTIO_NET_F_CTRL_RX	  |	\
+	 1u << VIRTIO_NET_F_CTRL_VLAN	  |	\
+	 1u << VIRTIO_NET_F_MRG_RXBUF)
 
 /*
  * CQ function prototype
diff --git a/lib/librte_pmd_virtio/virtio_pci.h b/lib/librte_pmd_virtio/virtio_pci.h
index 64d9c34..47f722a 100644
--- a/lib/librte_pmd_virtio/virtio_pci.h
+++ b/lib/librte_pmd_virtio/virtio_pci.h
@@ -96,26 +96,6 @@ struct virtqueue;
 #define VIRTIO_CONFIG_STATUS_FAILED    0x80
 
 /*
- * Generate interrupt when the virtqueue ring is
- * completely used, even if we've suppressed them.
- */
-#define VIRTIO_F_NOTIFY_ON_EMPTY (1 << 24)
-
-/*
- * The guest should never negotiate this feature; it
- * is used to detect faulty drivers.
- */
-#define VIRTIO_F_BAD_FEATURE (1 << 30)
-
-/*
- * Some VirtIO feature bits (currently bits 28 through 31) are
- * reserved for the transport being used (eg. virtio_ring), the
- * rest are per-device feature bits.
- */
-#define VIRTIO_TRANSPORT_F_START 28
-#define VIRTIO_TRANSPORT_F_END   32
-
-/*
  * Each virtqueue indirect descriptor list must be physically contiguous.
  * To allow us to malloc(9) each list individually, limit the number
  * supported to what will fit in one page. With 4KB pages, this is a limit
@@ -128,33 +108,55 @@ struct virtqueue;
 #define VIRTIO_MAX_INDIRECT ((int) (PAGE_SIZE / 16))
 
 /* The feature bitmap for virtio net */
-#define VIRTIO_NET_F_CSUM       0x00001 /* Host handles pkts w/ partial csum */
-#define VIRTIO_NET_F_GUEST_CSUM 0x00002 /* Guest handles pkts w/ partial csum*/
-#define VIRTIO_NET_F_MAC        0x00020 /* Host has given MAC address. */
-#define VIRTIO_NET_F_GSO        0x00040 /* Host handles pkts w/ any GSO type */
-#define VIRTIO_NET_F_GUEST_TSO4 0x00080 /* Guest can handle TSOv4 in. */
-#define VIRTIO_NET_F_GUEST_TSO6 0x00100 /* Guest can handle TSOv6 in. */
-#define VIRTIO_NET_F_GUEST_ECN  0x00200 /* Guest can handle TSO[6] w/ ECN in.*/
-#define VIRTIO_NET_F_GUEST_UFO  0x00400 /* Guest can handle UFO in. */
-#define VIRTIO_NET_F_HOST_TSO4  0x00800 /* Host can handle TSOv4 in. */
-#define VIRTIO_NET_F_HOST_TSO6  0x01000 /* Host can handle TSOv6 in. */
-#define VIRTIO_NET_F_HOST_ECN   0x02000 /* Host can handle TSO[6] w/ ECN in. */
-#define VIRTIO_NET_F_HOST_UFO   0x04000 /* Host can handle UFO in. */
-#define VIRTIO_NET_F_MRG_RXBUF  0x08000 /* Host can merge receive buffers. */
-#define VIRTIO_NET_F_STATUS     0x10000 /* virtio_net_config.status available*/
-#define VIRTIO_NET_F_CTRL_VQ    0x20000 /* Control channel available */
-#define VIRTIO_NET_F_CTRL_RX    0x40000 /* Control channel RX mode support */
-#define VIRTIO_NET_F_CTRL_VLAN  0x80000 /* Control channel VLAN filtering */
-#define VIRTIO_NET_F_CTRL_RX_EXTRA  0x100000 /* Extra RX mode control support */
-#define VIRTIO_RING_F_INDIRECT_DESC 0x10000000 /* Support for indirect buffer descriptors. */
-/* The guest publishes the used index for which it expects an interrupt
- * at the end of the avail ring. Host should ignore the avail->flags field.
- * The host publishes the avail index for which it expects a kick
- * at the end of the used ring. Guest should ignore the used->flags field.
+#define VIRTIO_NET_F_CSUM	0	/* Host handles pkts w/ partial csum */
+#define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
+#define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
+#define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
+#define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
+#define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
+#define VIRTIO_NET_F_GUEST_UFO	10	/* Guest can handle UFO in. */
+#define VIRTIO_NET_F_HOST_TSO4	11	/* Host can handle TSOv4 in. */
+#define VIRTIO_NET_F_HOST_TSO6	12	/* Host can handle TSOv6 in. */
+#define VIRTIO_NET_F_HOST_ECN	13	/* Host can handle TSO[6] w/ ECN in. */
+#define VIRTIO_NET_F_HOST_UFO	14	/* Host can handle UFO in. */
+#define VIRTIO_NET_F_MRG_RXBUF	15	/* Host can merge receive buffers. */
+#define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
+#define VIRTIO_NET_F_CTRL_VQ	17	/* Control channel available */
+#define VIRTIO_NET_F_CTRL_RX	18	/* Control channel RX mode support */
+#define VIRTIO_NET_F_CTRL_VLAN	19	/* Control channel VLAN filtering */
+#define VIRTIO_NET_F_CTRL_RX_EXTRA 20	/* Extra RX mode control support */
+#define VIRTIO_NET_F_GUEST_ANNOUNCE 21	/* Guest can announce device on the
+					 * network */
+#define VIRTIO_NET_F_MQ		22	/* Device supports Receive Flow
+					 * Steering */
+#define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
+
+/* Do we get callbacks when the ring is completely used, even if we've
+ * suppressed them? */
+#define VIRTIO_F_NOTIFY_ON_EMPTY	24
+
+/* Can the device handle any descriptor layout? */
+#define VIRTIO_F_ANY_LAYOUT		27
+
+/* We support indirect buffer descriptors */
+#define VIRTIO_RING_F_INDIRECT_DESC	28
+
+/*
+ * Some VirtIO feature bits (currently bits 28 through 31) are
+ * reserved for the transport being used (eg. virtio_ring), the
+ * rest are per-device feature bits.
  */
-#define VIRTIO_RING_F_EVENT_IDX 0x20000000
+#define VIRTIO_TRANSPORT_F_START 28
+#define VIRTIO_TRANSPORT_F_END   32
+
+/* The Guest publishes the used index for which it expects an interrupt
+ * at the end of the avail ring. Host should ignore the avail->flags field. */
+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_EVENT_IDX		29
 
-#define VIRTIO_NET_S_LINK_UP 1 /* Link is up */
+#define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
+#define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
 
 /*
  * Maximum number of virtqueues per device.
@@ -243,9 +245,9 @@ outl_p(unsigned int data, unsigned int port)
 	outl_p((unsigned int)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg))))
 
 static inline int
-vtpci_with_feature(struct virtio_hw *hw, uint32_t feature)
+vtpci_with_feature(struct virtio_hw *hw, uint32_t bit)
 {
-	return (hw->guest_features & feature) != 0;
+	return (hw->guest_features & (1u << bit)) != 0;
 }
 
 /*
diff --git a/lib/librte_pmd_virtio/virtqueue.h b/lib/librte_pmd_virtio/virtqueue.h
index 41dda50..5509173 100644
--- a/lib/librte_pmd_virtio/virtqueue.h
+++ b/lib/librte_pmd_virtio/virtqueue.h
@@ -201,18 +201,12 @@ struct virtqueue {
 };
 
 /* If multiqueue is provided by host, then we suppport it. */
-#ifndef VIRTIO_NET_F_MQ
-/* Device supports Receive Flow Steering */
-#define VIRTIO_NET_F_MQ 0x400000
 #define VIRTIO_NET_CTRL_MQ   4
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET        0
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN        1
 #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX        0x8000
-#endif
-#ifndef VIRTIO_NET_F_CTRL_MAC_ADDR
-#define VIRTIO_NET_F_CTRL_MAC_ADDR 0x800000
+
 #define VIRTIO_NET_CTRL_MAC_ADDR_SET         1
-#endif
 
 /**
  * This is the first element of the scatter-gather list.  If you don't
-- 
2.1.4

Re: [PATCH 5/5] virtio: clarify feature bit handling

[Stephen Hemminger]

On Wed, 15 Apr 2015 08:20:19 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> Change the features from bit mask to bit number. This allows the
> DPDK driver to use the definitions from Linux[1]. This makes doing
> enhancements to DPDK driver easier when new feature bits are added.
> 
> More importantly, get rid of the confusing feature bit initialization
> code. Remove the double negative code in the feature masking.
> Instead just have a new define with the list of feature bits implemented.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_pmd_virtio/virtio_ethdev.c | 17 +------
>  lib/librte_pmd_virtio/virtio_ethdev.h | 27 ++++------
>  lib/librte_pmd_virtio/virtio_pci.h    | 96 ++++++++++++++++++-----------------
>  lib/librte_pmd_virtio/virtqueue.h     |  8 +--
>  4 files changed, 61 insertions(+), 87 deletions(-)
> 
> diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c b/lib/librte_pmd_virtio/virtio_ethdev.c
> index db0232e..349b73b 100644
> --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> @@ -807,23 +807,10 @@ virtio_vlan_filter_set(struct rte_eth_dev *dev, uint16_t vlan_id, int on)
>  static void
>  virtio_negotiate_features(struct virtio_hw *hw)
>  {
> -	uint32_t host_features, mask;
> -
> -	/* checksum offload not implemented */
> -	mask = VIRTIO_NET_F_CSUM | VIRTIO_NET_F_GUEST_CSUM;
> -
> -	/* TSO and LRO are only available when their corresponding
> -	 * checksum offload feature is also negotiated.
> -	 */
> -	mask |= VIRTIO_NET_F_HOST_TSO4 | VIRTIO_NET_F_HOST_TSO6 | VIRTIO_NET_F_HOST_ECN;
> -	mask |= VIRTIO_NET_F_GUEST_TSO4 | VIRTIO_NET_F_GUEST_TSO6 | VIRTIO_NET_F_GUEST_ECN;
> -	mask |= VTNET_LRO_FEATURES;
> -
> -	/* not negotiating INDIRECT descriptor table support */
> -	mask |= VIRTIO_RING_F_INDIRECT_DESC;
> +	uint32_t host_features;
>  
>  	/* Prepare guest_features: feature that driver wants to support */
> -	hw->guest_features = VTNET_FEATURES & ~mask;
> +	hw->guest_features = VIRTIO_PMD_GUEST_FEATURES;
>  	PMD_INIT_LOG(DEBUG, "guest_features before negotiate = %x",
>  		hw->guest_features);
>  
> diff --git a/lib/librte_pmd_virtio/virtio_ethdev.h b/lib/librte_pmd_virtio/virtio_ethdev.h
> index e6d4533..df2cb7d 100644
> --- a/lib/librte_pmd_virtio/virtio_ethdev.h
> +++ b/lib/librte_pmd_virtio/virtio_ethdev.h
> @@ -56,24 +56,15 @@
>  #define VIRTIO_MAX_RX_PKTLEN  9728
>  
>  /* Features desired/implemented by this driver. */
> -#define VTNET_FEATURES \
> -	(VIRTIO_NET_F_MAC       | \
> -	VIRTIO_NET_F_STATUS     | \
> -	VIRTIO_NET_F_MQ         | \
> -	VIRTIO_NET_F_CTRL_MAC_ADDR | \
> -	VIRTIO_NET_F_CTRL_VQ    | \
> -	VIRTIO_NET_F_CTRL_RX    | \
> -	VIRTIO_NET_F_CTRL_VLAN  | \
> -	VIRTIO_NET_F_CSUM       | \
> -	VIRTIO_NET_F_HOST_TSO4  | \
> -	VIRTIO_NET_F_HOST_TSO6  | \
> -	VIRTIO_NET_F_HOST_ECN   | \
> -	VIRTIO_NET_F_GUEST_CSUM | \
> -	VIRTIO_NET_F_GUEST_TSO4 | \
> -	VIRTIO_NET_F_GUEST_TSO6 | \
> -	VIRTIO_NET_F_GUEST_ECN  | \
> -	VIRTIO_NET_F_MRG_RXBUF  | \
> -	VIRTIO_RING_F_INDIRECT_DESC)
> +#define VIRTIO_PMD_GUEST_FEATURES		\
> +	(1u << VIRTIO_NET_F_MAC		  |	\
> +	 1u << VIRTIO_NET_F_STATUS	  |	\
> +	 1u << VIRTIO_NET_F_MQ		  |	\
> +	 1u << VIRTIO_NET_F_CTRL_MAC_ADDR |	\
> +	 1u << VIRTIO_NET_F_CTRL_VQ	  |	\
> +	 1u << VIRTIO_NET_F_CTRL_RX	  |	\
> +	 1u << VIRTIO_NET_F_CTRL_VLAN	  |	\
> +	 1u << VIRTIO_NET_F_MRG_RXBUF)
>  
>  /*
>   * CQ function prototype
> diff --git a/lib/librte_pmd_virtio/virtio_pci.h b/lib/librte_pmd_virtio/virtio_pci.h
> index 64d9c34..47f722a 100644
> --- a/lib/librte_pmd_virtio/virtio_pci.h
> +++ b/lib/librte_pmd_virtio/virtio_pci.h
> @@ -96,26 +96,6 @@ struct virtqueue;
>  #define VIRTIO_CONFIG_STATUS_FAILED    0x80
>  
>  /*
> - * Generate interrupt when the virtqueue ring is
> - * completely used, even if we've suppressed them.
> - */
> -#define VIRTIO_F_NOTIFY_ON_EMPTY (1 << 24)
> -
> -/*
> - * The guest should never negotiate this feature; it
> - * is used to detect faulty drivers.
> - */
> -#define VIRTIO_F_BAD_FEATURE (1 << 30)
> -
> -/*
> - * Some VirtIO feature bits (currently bits 28 through 31) are
> - * reserved for the transport being used (eg. virtio_ring), the
> - * rest are per-device feature bits.
> - */
> -#define VIRTIO_TRANSPORT_F_START 28
> -#define VIRTIO_TRANSPORT_F_END   32
> -
> -/*
>   * Each virtqueue indirect descriptor list must be physically contiguous.
>   * To allow us to malloc(9) each list individually, limit the number
>   * supported to what will fit in one page. With 4KB pages, this is a limit
> @@ -128,33 +108,55 @@ struct virtqueue;
>  #define VIRTIO_MAX_INDIRECT ((int) (PAGE_SIZE / 16))
>  
>  /* The feature bitmap for virtio net */
> -#define VIRTIO_NET_F_CSUM       0x00001 /* Host handles pkts w/ partial csum */
> -#define VIRTIO_NET_F_GUEST_CSUM 0x00002 /* Guest handles pkts w/ partial csum*/
> -#define VIRTIO_NET_F_MAC        0x00020 /* Host has given MAC address. */
> -#define VIRTIO_NET_F_GSO        0x00040 /* Host handles pkts w/ any GSO type */
> -#define VIRTIO_NET_F_GUEST_TSO4 0x00080 /* Guest can handle TSOv4 in. */
> -#define VIRTIO_NET_F_GUEST_TSO6 0x00100 /* Guest can handle TSOv6 in. */
> -#define VIRTIO_NET_F_GUEST_ECN  0x00200 /* Guest can handle TSO[6] w/ ECN in.*/
> -#define VIRTIO_NET_F_GUEST_UFO  0x00400 /* Guest can handle UFO in. */
> -#define VIRTIO_NET_F_HOST_TSO4  0x00800 /* Host can handle TSOv4 in. */
> -#define VIRTIO_NET_F_HOST_TSO6  0x01000 /* Host can handle TSOv6 in. */
> -#define VIRTIO_NET_F_HOST_ECN   0x02000 /* Host can handle TSO[6] w/ ECN in. */
> -#define VIRTIO_NET_F_HOST_UFO   0x04000 /* Host can handle UFO in. */
> -#define VIRTIO_NET_F_MRG_RXBUF  0x08000 /* Host can merge receive buffers. */
> -#define VIRTIO_NET_F_STATUS     0x10000 /* virtio_net_config.status available*/
> -#define VIRTIO_NET_F_CTRL_VQ    0x20000 /* Control channel available */
> -#define VIRTIO_NET_F_CTRL_RX    0x40000 /* Control channel RX mode support */
> -#define VIRTIO_NET_F_CTRL_VLAN  0x80000 /* Control channel VLAN filtering */
> -#define VIRTIO_NET_F_CTRL_RX_EXTRA  0x100000 /* Extra RX mode control support */
> -#define VIRTIO_RING_F_INDIRECT_DESC 0x10000000 /* Support for indirect buffer descriptors. */
> -/* The guest publishes the used index for which it expects an interrupt
> - * at the end of the avail ring. Host should ignore the avail->flags field.
> - * The host publishes the avail index for which it expects a kick
> - * at the end of the used ring. Guest should ignore the used->flags field.
> +#define VIRTIO_NET_F_CSUM	0	/* Host handles pkts w/ partial csum */
> +#define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
> +#define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
> +#define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
> +#define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> +#define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
> +#define VIRTIO_NET_F_GUEST_UFO	10	/* Guest can handle UFO in. */
> +#define VIRTIO_NET_F_HOST_TSO4	11	/* Host can handle TSOv4 in. */
> +#define VIRTIO_NET_F_HOST_TSO6	12	/* Host can handle TSOv6 in. */
> +#define VIRTIO_NET_F_HOST_ECN	13	/* Host can handle TSO[6] w/ ECN in. */
> +#define VIRTIO_NET_F_HOST_UFO	14	/* Host can handle UFO in. */
> +#define VIRTIO_NET_F_MRG_RXBUF	15	/* Host can merge receive buffers. */
> +#define VIRTIO_NET_F_STATUS	16	/* virtio_net_config.status available */
> +#define VIRTIO_NET_F_CTRL_VQ	17	/* Control channel available */
> +#define VIRTIO_NET_F_CTRL_RX	18	/* Control channel RX mode support */
> +#define VIRTIO_NET_F_CTRL_VLAN	19	/* Control channel VLAN filtering */
> +#define VIRTIO_NET_F_CTRL_RX_EXTRA 20	/* Extra RX mode control support */
> +#define VIRTIO_NET_F_GUEST_ANNOUNCE 21	/* Guest can announce device on the
> +					 * network */
> +#define VIRTIO_NET_F_MQ		22	/* Device supports Receive Flow
> +					 * Steering */
> +#define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
> +
> +/* Do we get callbacks when the ring is completely used, even if we've
> + * suppressed them? */
> +#define VIRTIO_F_NOTIFY_ON_EMPTY	24
> +
> +/* Can the device handle any descriptor layout? */
> +#define VIRTIO_F_ANY_LAYOUT		27
> +
> +/* We support indirect buffer descriptors */
> +#define VIRTIO_RING_F_INDIRECT_DESC	28
> +
> +/*
> + * Some VirtIO feature bits (currently bits 28 through 31) are
> + * reserved for the transport being used (eg. virtio_ring), the
> + * rest are per-device feature bits.
>   */
> -#define VIRTIO_RING_F_EVENT_IDX 0x20000000
> +#define VIRTIO_TRANSPORT_F_START 28
> +#define VIRTIO_TRANSPORT_F_END   32
> +
> +/* The Guest publishes the used index for which it expects an interrupt
> + * at the end of the avail ring. Host should ignore the avail->flags field. */
> +/* The Host publishes the avail index for which it expects a kick
> + * at the end of the used ring. Guest should ignore the used->flags field. */
> +#define VIRTIO_RING_F_EVENT_IDX		29
>  
> -#define VIRTIO_NET_S_LINK_UP 1 /* Link is up */
> +#define VIRTIO_NET_S_LINK_UP	1	/* Link is up */
> +#define VIRTIO_NET_S_ANNOUNCE	2	/* Announcement is needed */
>  
>  /*
>   * Maximum number of virtqueues per device.
> @@ -243,9 +245,9 @@ outl_p(unsigned int data, unsigned int port)
>  	outl_p((unsigned int)(value), (VIRTIO_PCI_REG_ADDR((hw), (reg))))
>  
>  static inline int
> -vtpci_with_feature(struct virtio_hw *hw, uint32_t feature)
> +vtpci_with_feature(struct virtio_hw *hw, uint32_t bit)
>  {
> -	return (hw->guest_features & feature) != 0;
> +	return (hw->guest_features & (1u << bit)) != 0;
>  }
>  
>  /*
> diff --git a/lib/librte_pmd_virtio/virtqueue.h b/lib/librte_pmd_virtio/virtqueue.h
> index 41dda50..5509173 100644
> --- a/lib/librte_pmd_virtio/virtqueue.h
> +++ b/lib/librte_pmd_virtio/virtqueue.h
> @@ -201,18 +201,12 @@ struct virtqueue {
>  };
>  
>  /* If multiqueue is provided by host, then we suppport it. */
> -#ifndef VIRTIO_NET_F_MQ
> -/* Device supports Receive Flow Steering */
> -#define VIRTIO_NET_F_MQ 0x400000
>  #define VIRTIO_NET_CTRL_MQ   4
>  #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET        0
>  #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN        1
>  #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MAX        0x8000
> -#endif
> -#ifndef VIRTIO_NET_F_CTRL_MAC_ADDR
> -#define VIRTIO_NET_F_CTRL_MAC_ADDR 0x800000
> +
>  #define VIRTIO_NET_CTRL_MAC_ADDR_SET         1
> -#endif
>  
>  /**
>   * This is the first element of the scatter-gather list.  If you don't

Why are all these virtio patches stuck in the DPDK dead zone.
It makes me frustrated that no update to virtio has been done for 2.1

Re: [PATCH 5/5] virtio: clarify feature bit handling

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Wednesday, June 10, 2015 8:49 AM
> To: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 5/5] virtio: clarify feature bit handling
> 
> On Wed, 15 Apr 2015 08:20:19 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > Change the features from bit mask to bit number. This allows the DPDK
> > driver to use the definitions from Linux[1]. This makes doing
> > enhancements to DPDK driver easier when new feature bits are added.
> >
> > More importantly, get rid of the confusing feature bit initialization
> > code. Remove the double negative code in the feature masking.
> > Instead just have a new define with the list of feature bits implemented.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/librte_pmd_virtio/virtio_ethdev.c | 17 +------
> > lib/librte_pmd_virtio/virtio_ethdev.h | 27 ++++------
> >  lib/librte_pmd_virtio/virtio_pci.h    | 96 ++++++++++++++++++--------------

As the path for virtio codes is moved to drivers/net/virtio, so need a v2 version to rebase it.

Thanks
Changchun