diff mbox

[dpdk-dev] vhost-user: enable virtio 1.0

Message ID 1444907319-26348-1-git-send-email-marcel@redhat.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Marcel Apfelbaum Oct. 15, 2015, 11:08 a.m. UTC 
  Make vhost-user virtio 1.0 compatible by adding it to the
supported features and keeping the header length
the same as for mergeable RX buffers.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---

To be applied on top of:
   [dpdk-dev] [PATCH v6 00/13] vhost-user multiple queues enabling

Thanks,
Marcel
    
 lib/librte_vhost/virtio-net.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Michael S. Tsirkin Oct. 15, 2015, 1:18 p.m. UTC | #1 
On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> Make vhost-user virtio 1.0 compatible by adding it to the
> supported features and keeping the header length
> the same as for mergeable RX buffers.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Looks good to me

Acked-by: Michael S. Tsirkin <mst@redhat.com>

Just one question: dpdk is only supported on little-endian
platforms at the moment, right?
virtio 1 spec requires little endian.

> ---
> 
> To be applied on top of:
>    [dpdk-dev] [PATCH v6 00/13] vhost-user multiple queues enabling
> 
> Thanks,
> Marcel
>     
>  lib/librte_vhost/virtio-net.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index a51327d..ee4650e 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -75,6 +75,7 @@ static struct virtio_net_config_ll *ll_root;
>  				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
>  				(1ULL << VIRTIO_NET_F_CTRL_RX) | \
>  				(1ULL << VIRTIO_NET_F_MQ)      | \
> +				(1ULL << VIRTIO_F_VERSION_1)   | \
>  				(1ULL << VHOST_F_LOG_ALL)      | \
>  				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
>  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> @@ -477,17 +478,17 @@ set_features(struct vhost_device_ctx ctx, uint64_t *pu)
>  		return -1;
>  
>  	dev->features = *pu;
> -	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> -		LOG_DEBUG(VHOST_CONFIG,
> -			"(%"PRIu64") Mergeable RX buffers enabled\n",
> -			dev->device_fh);
> +	if (dev->features &
> +	    ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
>  		vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  	} else {
> -		LOG_DEBUG(VHOST_CONFIG,
> -			"(%"PRIu64") Mergeable RX buffers disabled\n",
> -			dev->device_fh);
>  		vhost_hlen = sizeof(struct virtio_net_hdr);
>  	}
> +	LOG_DEBUG(VHOST_CONFIG,
> +              "(%"PRIu64") Mergeable RX buffers %s, virtio 1 %s\n",
> +	          dev->device_fh,
> +              (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
> +              (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
>  
>  	for (i = 0; i < dev->virt_qp_nb; i++) {
>  		uint16_t base_idx = i * VIRTIO_QNUM;
> -- 
> 2.1.0
Yuanhan Liu Oct. 16, 2015, 2:24 a.m. UTC | #2 
On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> > Make vhost-user virtio 1.0 compatible by adding it to the
> > supported features and keeping the header length
> > the same as for mergeable RX buffers.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Marcel, that's actually one of my TODOs in this quarter. So, thank
you! :)

> 
> Looks good to me
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Just one question: dpdk is only supported on little-endian
> platforms at the moment, right?

AFAIK, yes. But you might also see that there are some patch to add
ARM arch support showed up in the mailing list few weeks ago.

> virtio 1 spec requires little endian.

I made a quick list of the difference between virtio v0.95 and v1.0
months ago just by reading your kernel commits of adding v1.0 support:

    +-------------------+-----------------+------------------------------+
    |                   |     v0.95       |  v1.0                        |
    +-------------------+-----------------+------------------------------+
1)  | features bits     |     32          |  64                          |
    +-------------------+-----------------+------------------------------+
2)  | Endianness        |     nature      |  Little Endian               |
    +-------------------+-----------------+------------------------------+
3)  | vring space       |     contiguous  |  avail and used buffer could |
    |                   |     memory      |  be on a separate memory     |
    +-------------------+-----------------+------------------------------+
4)  | FEATURE_OK status |     No          |   Yes                        |
    +-------------------+-----------------+------------------------------+



For 1), I guess we have been using 64 bit for storing features bits
for vhost since long time ago. So, there should be no extra effort.

For 2), as stated, there might be no issue as far as DPDK is little
endian only. But we'd better add a wrapper for that, as it seems
adding big endian support would come in near future.

For 3), are we actually doing that? I just saw that there is a kernel
patch to introduce two functions for getting the avail and used buffer
address, respectively.  But I didn't see that the two buffer are
allocated in non-contiguous memory.

For 4), it's a work we should do at virtio PMD driver. And it seems
that there are far more work need to be done at virtio PDM driver than
at vhost lib, say, adding the virtio morden PCI support.

Besides those 4 differs, did I miss anyting?


BTW, since we already have same TODOs, I guess it'd be better to
share what we have in our TODO list. Here are what I got till the
time writing this email (in order of priority):

- a vhost performance issue (it might last long; it might not).

- vhost-user live migration support

- virtio 1.0 support, including PMD and vhost lib (and you guys have
  already done that :)

Thanks.

	--yliu


> > ---
> > 
> > To be applied on top of:
> >    [dpdk-dev] [PATCH v6 00/13] vhost-user multiple queues enabling
> > 
> > Thanks,
> > Marcel
> >     
> >  lib/librte_vhost/virtio-net.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> > index a51327d..ee4650e 100644
> > --- a/lib/librte_vhost/virtio-net.c
> > +++ b/lib/librte_vhost/virtio-net.c
> > @@ -75,6 +75,7 @@ static struct virtio_net_config_ll *ll_root;
> >  				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
> >  				(1ULL << VIRTIO_NET_F_CTRL_RX) | \
> >  				(1ULL << VIRTIO_NET_F_MQ)      | \
> > +				(1ULL << VIRTIO_F_VERSION_1)   | \
> >  				(1ULL << VHOST_F_LOG_ALL)      | \
> >  				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
> >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> > @@ -477,17 +478,17 @@ set_features(struct vhost_device_ctx ctx, uint64_t *pu)
> >  		return -1;
> >  
> >  	dev->features = *pu;
> > -	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> > -		LOG_DEBUG(VHOST_CONFIG,
> > -			"(%"PRIu64") Mergeable RX buffers enabled\n",
> > -			dev->device_fh);
> > +	if (dev->features &
> > +	    ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
> >  		vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> >  	} else {
> > -		LOG_DEBUG(VHOST_CONFIG,
> > -			"(%"PRIu64") Mergeable RX buffers disabled\n",
> > -			dev->device_fh);
> >  		vhost_hlen = sizeof(struct virtio_net_hdr);
> >  	}
> > +	LOG_DEBUG(VHOST_CONFIG,
> > +              "(%"PRIu64") Mergeable RX buffers %s, virtio 1 %s\n",
> > +	          dev->device_fh,
> > +              (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
> > +              (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
> >  
> >  	for (i = 0; i < dev->virt_qp_nb; i++) {
> >  		uint16_t base_idx = i * VIRTIO_QNUM;
> > -- 
> > 2.1.0
Michael S. Tsirkin Oct. 16, 2015, 6:20 a.m. UTC | #3 
On Fri, Oct 16, 2015 at 10:24:38AM +0800, Yuanhan Liu wrote:
> On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> > > Make vhost-user virtio 1.0 compatible by adding it to the
> > > supported features and keeping the header length
> > > the same as for mergeable RX buffers.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> 
> Marcel, that's actually one of my TODOs in this quarter. So, thank
> you! :)
> 
> > 
> > Looks good to me
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > Just one question: dpdk is only supported on little-endian
> > platforms at the moment, right?
> 
> AFAIK, yes. But you might also see that there are some patch to add
> ARM arch support showed up in the mailing list few weeks ago.

Luckily, that's also little-endian.

> > virtio 1 spec requires little endian.
> 
> I made a quick list of the difference between virtio v0.95 and v1.0
> months ago just by reading your kernel commits of adding v1.0 support:
> 
>     +-------------------+-----------------+------------------------------+
>     |                   |     v0.95       |  v1.0                        |
>     +-------------------+-----------------+------------------------------+
> 1)  | features bits     |     32          |  64                          |
>     +-------------------+-----------------+------------------------------+
> 2)  | Endianness        |     nature      |  Little Endian               |
>     +-------------------+-----------------+------------------------------+
> 3)  | vring space       |     contiguous  |  avail and used buffer could |
>     |                   |     memory      |  be on a separate memory     |

And desc buffer, too.

>     +-------------------+-----------------+------------------------------+
> 4)  | FEATURE_OK status |     No          |   Yes                        |
>     +-------------------+-----------------+------------------------------+
> 
> 
> 
> For 1), I guess we have been using 64 bit for storing features bits
> for vhost since long time ago. So, there should be no extra effort.
> 
> For 2), as stated, there might be no issue as far as DPDK is little
> endian only. But we'd better add a wrapper for that, as it seems
> adding big endian support would come in near future.

OK, but it probably doesn't

> For 3), are we actually doing that? I just saw that there is a kernel
> patch to introduce two functions for getting the avail and used buffer
> address, respectively.  But I didn't see that the two buffer are
> allocated in non-contiguous memory.

That will soon happen, anyone claiming support for virtio 1

But vhost user already sends each ring part separately.
Does dpdk assume they are contigious?

> For 4), it's a work we should do at virtio PMD driver. And it seems
> that there are far more work need to be done at virtio PDM driver than
> at vhost lib, say, adding the virtio morden PCI support.
> 
> Besides those 4 differs, did I miss anyting?


From virtio PMD point of view? There are more
differences. The trick is to find "legacy interface"
sections and go over them, that compares 0.9 to 1.0.

> 
> BTW, since we already have same TODOs, I guess it'd be better to
> share what we have in our TODO list. Here are what I got till the
> time writing this email (in order of priority):
> 
> - a vhost performance issue (it might last long; it might not).
> 
> - vhost-user live migration support
> 
> - virtio 1.0 support, including PMD and vhost lib (and you guys have
>   already done that :)
> 
> Thanks.

This patch only touches the vhost lib, though.

> 	--yliu
> 
> 
> > > ---
> > > 
> > > To be applied on top of:
> > >    [dpdk-dev] [PATCH v6 00/13] vhost-user multiple queues enabling
> > > 
> > > Thanks,
> > > Marcel
> > >     
> > >  lib/librte_vhost/virtio-net.c | 15 ++++++++-------
> > >  1 file changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> > > index a51327d..ee4650e 100644
> > > --- a/lib/librte_vhost/virtio-net.c
> > > +++ b/lib/librte_vhost/virtio-net.c
> > > @@ -75,6 +75,7 @@ static struct virtio_net_config_ll *ll_root;
> > >  				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
> > >  				(1ULL << VIRTIO_NET_F_CTRL_RX) | \
> > >  				(1ULL << VIRTIO_NET_F_MQ)      | \
> > > +				(1ULL << VIRTIO_F_VERSION_1)   | \
> > >  				(1ULL << VHOST_F_LOG_ALL)      | \
> > >  				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
> > >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> > > @@ -477,17 +478,17 @@ set_features(struct vhost_device_ctx ctx, uint64_t *pu)
> > >  		return -1;
> > >  
> > >  	dev->features = *pu;
> > > -	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> > > -		LOG_DEBUG(VHOST_CONFIG,
> > > -			"(%"PRIu64") Mergeable RX buffers enabled\n",
> > > -			dev->device_fh);
> > > +	if (dev->features &
> > > +	    ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
> > >  		vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> > >  	} else {
> > > -		LOG_DEBUG(VHOST_CONFIG,
> > > -			"(%"PRIu64") Mergeable RX buffers disabled\n",
> > > -			dev->device_fh);
> > >  		vhost_hlen = sizeof(struct virtio_net_hdr);
> > >  	}
> > > +	LOG_DEBUG(VHOST_CONFIG,
> > > +              "(%"PRIu64") Mergeable RX buffers %s, virtio 1 %s\n",
> > > +	          dev->device_fh,
> > > +              (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
> > > +              (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
> > >  
> > >  	for (i = 0; i < dev->virt_qp_nb; i++) {
> > >  		uint16_t base_idx = i * VIRTIO_QNUM;
> > > -- 
> > > 2.1.0
Yuanhan Liu Oct. 16, 2015, 6:38 a.m. UTC | #4 
On Fri, Oct 16, 2015 at 09:20:18AM +0300, Michael S. Tsirkin wrote:
> On Fri, Oct 16, 2015 at 10:24:38AM +0800, Yuanhan Liu wrote:
> > On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> > > > Make vhost-user virtio 1.0 compatible by adding it to the
> > > > supported features and keeping the header length
> > > > the same as for mergeable RX buffers.
> > > > 
> > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > 
> > Marcel, that's actually one of my TODOs in this quarter. So, thank
> > you! :)
> > 
> > > 
> > > Looks good to me
> > > 
> > > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > 
> > > Just one question: dpdk is only supported on little-endian
> > > platforms at the moment, right?
> > 
> > AFAIK, yes. But you might also see that there are some patch to add
> > ARM arch support showed up in the mailing list few weeks ago.
> 
> Luckily, that's also little-endian.

Oops, I never had hands on experience on ARM and I was always thinking
it's big endian. Sigh ... :-/

Anyway, good to know.

> 
> > > virtio 1 spec requires little endian.
> > 
> > I made a quick list of the difference between virtio v0.95 and v1.0
> > months ago just by reading your kernel commits of adding v1.0 support:
> > 
> >     +-------------------+-----------------+------------------------------+
> >     |                   |     v0.95       |  v1.0                        |
> >     +-------------------+-----------------+------------------------------+
> > 1)  | features bits     |     32          |  64                          |
> >     +-------------------+-----------------+------------------------------+
> > 2)  | Endianness        |     nature      |  Little Endian               |
> >     +-------------------+-----------------+------------------------------+
> > 3)  | vring space       |     contiguous  |  avail and used buffer could |
> >     |                   |     memory      |  be on a separate memory     |
> 
> And desc buffer, too.

Thanks for the remind.
> 
> >     +-------------------+-----------------+------------------------------+
> > 4)  | FEATURE_OK status |     No          |   Yes                        |
> >     +-------------------+-----------------+------------------------------+
> > 
> > 
> > 
> > For 1), I guess we have been using 64 bit for storing features bits
> > for vhost since long time ago. So, there should be no extra effort.
> > 
> > For 2), as stated, there might be no issue as far as DPDK is little
> > endian only. But we'd better add a wrapper for that, as it seems
> > adding big endian support would come in near future.
> 
> OK, but it probably doesn't

Yeah, let's handle that later.

> 
> > For 3), are we actually doing that? I just saw that there is a kernel
> > patch to introduce two functions for getting the avail and used buffer
> > address, respectively.  But I didn't see that the two buffer are
> > allocated in non-contiguous memory.
> 
> That will soon happen, anyone claiming support for virtio 1
> 
> But vhost user already sends each ring part separately.
> Does dpdk assume they are contigious?

Oh, right, we don't assume that. See set_vring_addr() ib/librte_vhost/virtio-net.c.
We get the address of avail buffer, used buffere and desc buffer one by
one.

> 
> > For 4), it's a work we should do at virtio PMD driver. And it seems
> > that there are far more work need to be done at virtio PDM driver than
> > at vhost lib, say, adding the virtio morden PCI support.
> > 
> > Besides those 4 differs, did I miss anyting?
> 
> 
> >From virtio PMD point of view? There are more
> differences.

That's true.

> The trick is to find "legacy interface"
> sections and go over them, that compares 0.9 to 1.0.

Thanks for the tip!

> 
> > 
> > BTW, since we already have same TODOs, I guess it'd be better to
> > share what we have in our TODO list. Here are what I got till the
> > time writing this email (in order of priority):
> > 
> > - a vhost performance issue (it might last long; it might not).
> > 
> > - vhost-user live migration support
> > 
> > - virtio 1.0 support, including PMD and vhost lib (and you guys have
> >   already done that :)
> > 
> > Thanks.
> 
> This patch only touches the vhost lib, though.

Yes, and this patch also looks good to me. I will add Reviewed-by in
another email, to make it oustanding so that Thomas can see that clearly
and pick it up.


	--yliu
> > 
> > 
> > > > ---
> > > > 
> > > > To be applied on top of:
> > > >    [dpdk-dev] [PATCH v6 00/13] vhost-user multiple queues enabling
> > > > 
> > > > Thanks,
> > > > Marcel
> > > >     
> > > >  lib/librte_vhost/virtio-net.c | 15 ++++++++-------
> > > >  1 file changed, 8 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> > > > index a51327d..ee4650e 100644
> > > > --- a/lib/librte_vhost/virtio-net.c
> > > > +++ b/lib/librte_vhost/virtio-net.c
> > > > @@ -75,6 +75,7 @@ static struct virtio_net_config_ll *ll_root;
> > > >  				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
> > > >  				(1ULL << VIRTIO_NET_F_CTRL_RX) | \
> > > >  				(1ULL << VIRTIO_NET_F_MQ)      | \
> > > > +				(1ULL << VIRTIO_F_VERSION_1)   | \
> > > >  				(1ULL << VHOST_F_LOG_ALL)      | \
> > > >  				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
> > > >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> > > > @@ -477,17 +478,17 @@ set_features(struct vhost_device_ctx ctx, uint64_t *pu)
> > > >  		return -1;
> > > >  
> > > >  	dev->features = *pu;
> > > > -	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> > > > -		LOG_DEBUG(VHOST_CONFIG,
> > > > -			"(%"PRIu64") Mergeable RX buffers enabled\n",
> > > > -			dev->device_fh);
> > > > +	if (dev->features &
> > > > +	    ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
> > > >  		vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> > > >  	} else {
> > > > -		LOG_DEBUG(VHOST_CONFIG,
> > > > -			"(%"PRIu64") Mergeable RX buffers disabled\n",
> > > > -			dev->device_fh);
> > > >  		vhost_hlen = sizeof(struct virtio_net_hdr);
> > > >  	}
> > > > +	LOG_DEBUG(VHOST_CONFIG,
> > > > +              "(%"PRIu64") Mergeable RX buffers %s, virtio 1 %s\n",
> > > > +	          dev->device_fh,
> > > > +              (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
> > > > +              (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
> > > >  
> > > >  	for (i = 0; i < dev->virt_qp_nb; i++) {
> > > >  		uint16_t base_idx = i * VIRTIO_QNUM;
> > > > -- 
> > > > 2.1.0
Yuanhan Liu Oct. 16, 2015, 6:39 a.m. UTC | #5 
Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

And thanks for the work.

	--yliu

On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> Make vhost-user virtio 1.0 compatible by adding it to the
> supported features and keeping the header length
> the same as for mergeable RX buffers.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> 
> To be applied on top of:
>    [dpdk-dev] [PATCH v6 00/13] vhost-user multiple queues enabling
> 
> Thanks,
> Marcel
>     
>  lib/librte_vhost/virtio-net.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index a51327d..ee4650e 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -75,6 +75,7 @@ static struct virtio_net_config_ll *ll_root;
>  				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
>  				(1ULL << VIRTIO_NET_F_CTRL_RX) | \
>  				(1ULL << VIRTIO_NET_F_MQ)      | \
> +				(1ULL << VIRTIO_F_VERSION_1)   | \
>  				(1ULL << VHOST_F_LOG_ALL)      | \
>  				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
>  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> @@ -477,17 +478,17 @@ set_features(struct vhost_device_ctx ctx, uint64_t *pu)
>  		return -1;
>  
>  	dev->features = *pu;
> -	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> -		LOG_DEBUG(VHOST_CONFIG,
> -			"(%"PRIu64") Mergeable RX buffers enabled\n",
> -			dev->device_fh);
> +	if (dev->features &
> +	    ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
>  		vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  	} else {
> -		LOG_DEBUG(VHOST_CONFIG,
> -			"(%"PRIu64") Mergeable RX buffers disabled\n",
> -			dev->device_fh);
>  		vhost_hlen = sizeof(struct virtio_net_hdr);
>  	}
> +	LOG_DEBUG(VHOST_CONFIG,
> +              "(%"PRIu64") Mergeable RX buffers %s, virtio 1 %s\n",
> +	          dev->device_fh,
> +              (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
> +              (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
>  
>  	for (i = 0; i < dev->virt_qp_nb; i++) {
>  		uint16_t base_idx = i * VIRTIO_QNUM;
> -- 
> 2.1.0
Andriy Berestovskyy Oct. 16, 2015, 7:43 a.m. UTC | #6 
Hi guys,
Just a minor note: ARM is bi-endian in fact. For instance, there are
both endians tool chains available on Linaro.

Andriy


On Fri, Oct 16, 2015 at 8:20 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Oct 16, 2015 at 10:24:38AM +0800, Yuanhan Liu wrote:
>> On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
>> > On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
>> > > Make vhost-user virtio 1.0 compatible by adding it to the
>> > > supported features and keeping the header length
>> > > the same as for mergeable RX buffers.
>> > >
>> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>
>> Marcel, that's actually one of my TODOs in this quarter. So, thank
>> you! :)
>>
>> >
>> > Looks good to me
>> >
>> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
>> >
>> > Just one question: dpdk is only supported on little-endian
>> > platforms at the moment, right?
>>
>> AFAIK, yes. But you might also see that there are some patch to add
>> ARM arch support showed up in the mailing list few weeks ago.
>
> Luckily, that's also little-endian.
>
>> > virtio 1 spec requires little endian.
>>
>> I made a quick list of the difference between virtio v0.95 and v1.0
>> months ago just by reading your kernel commits of adding v1.0 support:
>>
>>     +-------------------+-----------------+------------------------------+
>>     |                   |     v0.95       |  v1.0                        |
>>     +-------------------+-----------------+------------------------------+
>> 1)  | features bits     |     32          |  64                          |
>>     +-------------------+-----------------+------------------------------+
>> 2)  | Endianness        |     nature      |  Little Endian               |
>>     +-------------------+-----------------+------------------------------+
>> 3)  | vring space       |     contiguous  |  avail and used buffer could |
>>     |                   |     memory      |  be on a separate memory     |
>
> And desc buffer, too.
>
>>     +-------------------+-----------------+------------------------------+
>> 4)  | FEATURE_OK status |     No          |   Yes                        |
>>     +-------------------+-----------------+------------------------------+
>>
>>
>>
>> For 1), I guess we have been using 64 bit for storing features bits
>> for vhost since long time ago. So, there should be no extra effort.
>>
>> For 2), as stated, there might be no issue as far as DPDK is little
>> endian only. But we'd better add a wrapper for that, as it seems
>> adding big endian support would come in near future.
>
> OK, but it probably doesn't
>
>> For 3), are we actually doing that? I just saw that there is a kernel
>> patch to introduce two functions for getting the avail and used buffer
>> address, respectively.  But I didn't see that the two buffer are
>> allocated in non-contiguous memory.
>
> That will soon happen, anyone claiming support for virtio 1
>
> But vhost user already sends each ring part separately.
> Does dpdk assume they are contigious?
>
>> For 4), it's a work we should do at virtio PMD driver. And it seems
>> that there are far more work need to be done at virtio PDM driver than
>> at vhost lib, say, adding the virtio morden PCI support.
>>
>> Besides those 4 differs, did I miss anyting?
>
>
> From virtio PMD point of view? There are more
> differences. The trick is to find "legacy interface"
> sections and go over them, that compares 0.9 to 1.0.
>
>>
>> BTW, since we already have same TODOs, I guess it'd be better to
>> share what we have in our TODO list. Here are what I got till the
>> time writing this email (in order of priority):
>>
>> - a vhost performance issue (it might last long; it might not).
>>
>> - vhost-user live migration support
>>
>> - virtio 1.0 support, including PMD and vhost lib (and you guys have
>>   already done that :)
>>
>> Thanks.
>
> This patch only touches the vhost lib, though.
>
>>       --yliu
>>
>>
>> > > ---
>> > >
>> > > To be applied on top of:
>> > >    [dpdk-dev] [PATCH v6 00/13] vhost-user multiple queues enabling
>> > >
>> > > Thanks,
>> > > Marcel
>> > >
>> > >  lib/librte_vhost/virtio-net.c | 15 ++++++++-------
>> > >  1 file changed, 8 insertions(+), 7 deletions(-)
>> > >
>> > > diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
>> > > index a51327d..ee4650e 100644
>> > > --- a/lib/librte_vhost/virtio-net.c
>> > > +++ b/lib/librte_vhost/virtio-net.c
>> > > @@ -75,6 +75,7 @@ static struct virtio_net_config_ll *ll_root;
>> > >                           (1ULL << VIRTIO_NET_F_CTRL_VQ) | \
>> > >                           (1ULL << VIRTIO_NET_F_CTRL_RX) | \
>> > >                           (1ULL << VIRTIO_NET_F_MQ)      | \
>> > > +                         (1ULL << VIRTIO_F_VERSION_1)   | \
>> > >                           (1ULL << VHOST_F_LOG_ALL)      | \
>> > >                           (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
>> > >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
>> > > @@ -477,17 +478,17 @@ set_features(struct vhost_device_ctx ctx, uint64_t *pu)
>> > >           return -1;
>> > >
>> > >   dev->features = *pu;
>> > > - if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
>> > > -         LOG_DEBUG(VHOST_CONFIG,
>> > > -                 "(%"PRIu64") Mergeable RX buffers enabled\n",
>> > > -                 dev->device_fh);
>> > > + if (dev->features &
>> > > +     ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
>> > >           vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>> > >   } else {
>> > > -         LOG_DEBUG(VHOST_CONFIG,
>> > > -                 "(%"PRIu64") Mergeable RX buffers disabled\n",
>> > > -                 dev->device_fh);
>> > >           vhost_hlen = sizeof(struct virtio_net_hdr);
>> > >   }
>> > > + LOG_DEBUG(VHOST_CONFIG,
>> > > +              "(%"PRIu64") Mergeable RX buffers %s, virtio 1 %s\n",
>> > > +           dev->device_fh,
>> > > +              (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
>> > > +              (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
>> > >
>> > >   for (i = 0; i < dev->virt_qp_nb; i++) {
>> > >           uint16_t base_idx = i * VIRTIO_QNUM;
>> > > --
>> > > 2.1.0
Yuanhan Liu Oct. 16, 2015, 7:50 a.m. UTC | #7 
On Fri, Oct 16, 2015 at 09:43:09AM +0200, Andriy Berestovskyy wrote:
> Hi guys,
> Just a minor note: ARM is bi-endian in fact.

Thank you for clarifying that my old memory is right :)

	--yliu

> For instance, there are
> both endians tool chains available on Linaro.
> 
> Andriy
> 
> 
> On Fri, Oct 16, 2015 at 8:20 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Oct 16, 2015 at 10:24:38AM +0800, Yuanhan Liu wrote:
> >> On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
> >> > On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> >> > > Make vhost-user virtio 1.0 compatible by adding it to the
> >> > > supported features and keeping the header length
> >> > > the same as for mergeable RX buffers.
> >> > >
> >> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>
> >> Marcel, that's actually one of my TODOs in this quarter. So, thank
> >> you! :)
> >>
> >> >
> >> > Looks good to me
> >> >
> >> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >> >
> >> > Just one question: dpdk is only supported on little-endian
> >> > platforms at the moment, right?
> >>
> >> AFAIK, yes. But you might also see that there are some patch to add
> >> ARM arch support showed up in the mailing list few weeks ago.
> >
> > Luckily, that's also little-endian.
> >
> >> > virtio 1 spec requires little endian.
> >>
> >> I made a quick list of the difference between virtio v0.95 and v1.0
> >> months ago just by reading your kernel commits of adding v1.0 support:
> >>
> >>     +-------------------+-----------------+------------------------------+
> >>     |                   |     v0.95       |  v1.0                        |
> >>     +-------------------+-----------------+------------------------------+
> >> 1)  | features bits     |     32          |  64                          |
> >>     +-------------------+-----------------+------------------------------+
> >> 2)  | Endianness        |     nature      |  Little Endian               |
> >>     +-------------------+-----------------+------------------------------+
> >> 3)  | vring space       |     contiguous  |  avail and used buffer could |
> >>     |                   |     memory      |  be on a separate memory     |
> >
> > And desc buffer, too.
> >
> >>     +-------------------+-----------------+------------------------------+
> >> 4)  | FEATURE_OK status |     No          |   Yes                        |
> >>     +-------------------+-----------------+------------------------------+
> >>
> >>
> >>
> >> For 1), I guess we have been using 64 bit for storing features bits
> >> for vhost since long time ago. So, there should be no extra effort.
> >>
> >> For 2), as stated, there might be no issue as far as DPDK is little
> >> endian only. But we'd better add a wrapper for that, as it seems
> >> adding big endian support would come in near future.
> >
> > OK, but it probably doesn't
> >
> >> For 3), are we actually doing that? I just saw that there is a kernel
> >> patch to introduce two functions for getting the avail and used buffer
> >> address, respectively.  But I didn't see that the two buffer are
> >> allocated in non-contiguous memory.
> >
> > That will soon happen, anyone claiming support for virtio 1
> >
> > But vhost user already sends each ring part separately.
> > Does dpdk assume they are contigious?
> >
> >> For 4), it's a work we should do at virtio PMD driver. And it seems
> >> that there are far more work need to be done at virtio PDM driver than
> >> at vhost lib, say, adding the virtio morden PCI support.
> >>
> >> Besides those 4 differs, did I miss anyting?
> >
> >
> > From virtio PMD point of view? There are more
> > differences. The trick is to find "legacy interface"
> > sections and go over them, that compares 0.9 to 1.0.
> >
> >>
> >> BTW, since we already have same TODOs, I guess it'd be better to
> >> share what we have in our TODO list. Here are what I got till the
> >> time writing this email (in order of priority):
> >>
> >> - a vhost performance issue (it might last long; it might not).
> >>
> >> - vhost-user live migration support
> >>
> >> - virtio 1.0 support, including PMD and vhost lib (and you guys have
> >>   already done that :)
> >>
> >> Thanks.
> >
> > This patch only touches the vhost lib, though.
> >
> >>       --yliu
> >>
> >>
> >> > > ---
> >> > >
> >> > > To be applied on top of:
> >> > >    [dpdk-dev] [PATCH v6 00/13] vhost-user multiple queues enabling
> >> > >
> >> > > Thanks,
> >> > > Marcel
> >> > >
> >> > >  lib/librte_vhost/virtio-net.c | 15 ++++++++-------
> >> > >  1 file changed, 8 insertions(+), 7 deletions(-)
> >> > >
> >> > > diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> >> > > index a51327d..ee4650e 100644
> >> > > --- a/lib/librte_vhost/virtio-net.c
> >> > > +++ b/lib/librte_vhost/virtio-net.c
> >> > > @@ -75,6 +75,7 @@ static struct virtio_net_config_ll *ll_root;
> >> > >                           (1ULL << VIRTIO_NET_F_CTRL_VQ) | \
> >> > >                           (1ULL << VIRTIO_NET_F_CTRL_RX) | \
> >> > >                           (1ULL << VIRTIO_NET_F_MQ)      | \
> >> > > +                         (1ULL << VIRTIO_F_VERSION_1)   | \
> >> > >                           (1ULL << VHOST_F_LOG_ALL)      | \
> >> > >                           (1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
> >> > >  static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
> >> > > @@ -477,17 +478,17 @@ set_features(struct vhost_device_ctx ctx, uint64_t *pu)
> >> > >           return -1;
> >> > >
> >> > >   dev->features = *pu;
> >> > > - if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
> >> > > -         LOG_DEBUG(VHOST_CONFIG,
> >> > > -                 "(%"PRIu64") Mergeable RX buffers enabled\n",
> >> > > -                 dev->device_fh);
> >> > > + if (dev->features &
> >> > > +     ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
> >> > >           vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> >> > >   } else {
> >> > > -         LOG_DEBUG(VHOST_CONFIG,
> >> > > -                 "(%"PRIu64") Mergeable RX buffers disabled\n",
> >> > > -                 dev->device_fh);
> >> > >           vhost_hlen = sizeof(struct virtio_net_hdr);
> >> > >   }
> >> > > + LOG_DEBUG(VHOST_CONFIG,
> >> > > +              "(%"PRIu64") Mergeable RX buffers %s, virtio 1 %s\n",
> >> > > +           dev->device_fh,
> >> > > +              (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
> >> > > +              (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
> >> > >
> >> > >   for (i = 0; i < dev->virt_qp_nb; i++) {
> >> > >           uint16_t base_idx = i * VIRTIO_QNUM;
> >> > > --
> >> > > 2.1.0
> 
> 
> 
> -- 
> Andriy Berestovskyy
Michael S. Tsirkin Oct. 16, 2015, 7:56 a.m. UTC | #8 
On Fri, Oct 16, 2015 at 09:43:09AM +0200, Andriy Berestovskyy wrote:
> Hi guys,
> Just a minor note: ARM is bi-endian in fact. For instance, there are
> both endians tool chains available on Linaro.
> 
> Andriy

Yea. BE support is around for legacy stuff.  So I'm not sure it's all
that important to support that mode in NFV/DPDK applications.

If it becomes important, for virtio 0.9 you won't be able to
support transparently in dpdk anyway - you'll need a new
vhost-user message to get the endian-ness of the guest,
and do byteswaps conditionally, adding overhead.

Maybe going virtio 1 only is the sane thing to do for these
applications.
Bruce Richardson Oct. 16, 2015, 1:52 p.m. UTC | #9 
On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
> On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> > Make vhost-user virtio 1.0 compatible by adding it to the
> > supported features and keeping the header length
> > the same as for mergeable RX buffers.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> 
> Looks good to me
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Just one question: dpdk is only supported on little-endian
> platforms at the moment, right?

A recent release added in support for PPC (patches supplied by IBM). For 
example, see: 
http://dpdk.org/browse/dpdk/commit/?id=704ba3770032c5a901719d3837845581d5a56b58

/Bruce
Michael S. Tsirkin Oct. 18, 2015, 7:04 a.m. UTC | #10 
On Fri, Oct 16, 2015 at 02:52:30PM +0100, Bruce Richardson wrote:
> On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> > > Make vhost-user virtio 1.0 compatible by adding it to the
> > > supported features and keeping the header length
> > > the same as for mergeable RX buffers.
> > > 
> > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > 
> > Looks good to me
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > Just one question: dpdk is only supported on little-endian
> > platforms at the moment, right?
> 
> A recent release added in support for PPC (patches supplied by IBM). For 
> example, see: 
> http://dpdk.org/browse/dpdk/commit/?id=704ba3770032c5a901719d3837845581d5a56b58
> 
> /Bruce

This will require more work then as 1.0 is a different
endian-ness from 0.9. It's up to you guys to decide
whether correct BE support is now a requirement for all
new dpdk code. Let us know.
Thomas Monjalon Oct. 30, 2015, 5:48 p.m. UTC | #11 
2015-10-18 10:04, Michael S. Tsirkin:
> On Fri, Oct 16, 2015 at 02:52:30PM +0100, Bruce Richardson wrote:
> > On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> > > > Make vhost-user virtio 1.0 compatible by adding it to the
> > > > supported features and keeping the header length
> > > > the same as for mergeable RX buffers.
> > > > 
> > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > 
> > > Looks good to me
> > > 
> > > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > 
> > > Just one question: dpdk is only supported on little-endian
> > > platforms at the moment, right?
> > 
> > A recent release added in support for PPC (patches supplied by IBM). For 
> > example, see: 
> > http://dpdk.org/browse/dpdk/commit/?id=704ba3770032c5a901719d3837845581d5a56b58
> > 
> > /Bruce
> 
> This will require more work then as 1.0 is a different
> endian-ness from 0.9. It's up to you guys to decide
> whether correct BE support is now a requirement for all
> new dpdk code. Let us know.

I'm not sure to understand.
Yes DPDK must work on big endian platforms.
Does this patch prevent from using virtio 0.9 with big endian?
Does it work with old QEMU not supporting virtio 1.0?
Marcel Apfelbaum Nov. 1, 2015, 9 a.m. UTC | #12 
On 10/30/2015 07:48 PM, Thomas Monjalon wrote:
> 2015-10-18 10:04, Michael S. Tsirkin:
>> On Fri, Oct 16, 2015 at 02:52:30PM +0100, Bruce Richardson wrote:
>>> On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
>>>> On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
>>>>> Make vhost-user virtio 1.0 compatible by adding it to the
>>>>> supported features and keeping the header length
>>>>> the same as for mergeable RX buffers.
>>>>>
>>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>>
>>>> Looks good to me
>>>>
>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>>>
>>>> Just one question: dpdk is only supported on little-endian
>>>> platforms at the moment, right?
>>>
>>> A recent release added in support for PPC (patches supplied by IBM). For
>>> example, see:
>>> http://dpdk.org/browse/dpdk/commit/?id=704ba3770032c5a901719d3837845581d5a56b58
>>>
>>> /Bruce
>>
>> This will require more work then as 1.0 is a different
>> endian-ness from 0.9. It's up to you guys to decide
>> whether correct BE support is now a requirement for all
>> new dpdk code. Let us know.
>

Hi,

> I'm not sure to understand.
> Yes DPDK must work on big endian platforms.
> Does this patch prevent from using virtio 0.9 with big endian?

No, if it worked until now, will continue to work. (And the other way around)

However, if virtio 1.0 is supported by both QEMU and vhost-user,
virtio 1.0 has different endianess requirements than prev virtio,
Michael can better elaborate more.


> Does it work with old QEMU not supporting virtio 1.0?

Yes, it does. virtio 1.0 will be enabled only if the feature
is supported by both QEMU and vhost-user backend, otherwise
it will work as before.

Thanks,
Marcel

>
Thomas Monjalon Nov. 1, 2015, 9:53 a.m. UTC | #13 
2015-11-01 11:00, Marcel Apfelbaum:
> On 10/30/2015 07:48 PM, Thomas Monjalon wrote:
> > 2015-10-18 10:04, Michael S. Tsirkin:
> >> This will require more work then as 1.0 is a different
> >> endian-ness from 0.9. It's up to you guys to decide
> >> whether correct BE support is now a requirement for all
> >> new dpdk code. Let us know.
> 
> > I'm not sure to understand.
> > Yes DPDK must work on big endian platforms.
> > Does this patch prevent from using virtio 0.9 with big endian?
> 
> No, if it worked until now, will continue to work. (And the other way around)
> 
> However, if virtio 1.0 is supported by both QEMU and vhost-user,
> virtio 1.0 has different endianess requirements than prev virtio,

OK so that's an acceptable workaround: big endian platforms must use
virtio 0.9.

In order to have a big endian support of virtio 1.0, is it sufficient to
convert data? Is it something we want regarding performance?
Marcel Apfelbaum Nov. 1, 2015, 11:22 a.m. UTC | #14 
On 11/01/2015 11:53 AM, Thomas Monjalon wrote:
> 2015-11-01 11:00, Marcel Apfelbaum:
>> On 10/30/2015 07:48 PM, Thomas Monjalon wrote:
>>> 2015-10-18 10:04, Michael S. Tsirkin:
>>>> This will require more work then as 1.0 is a different
>>>> endian-ness from 0.9. It's up to you guys to decide
>>>> whether correct BE support is now a requirement for all
>>>> new dpdk code. Let us know.
>>
>>> I'm not sure to understand.
>>> Yes DPDK must work on big endian platforms.
>>> Does this patch prevent from using virtio 0.9 with big endian?
>>
>> No, if it worked until now, will continue to work. (And the other way around)
>>
>> However, if virtio 1.0 is supported by both QEMU and vhost-user,
>> virtio 1.0 has different endianess requirements than prev virtio,
>
> OK so that's an acceptable workaround: big endian platforms must use
> virtio 0.9.
>
> In order to have a big endian support of virtio 1.0, is it sufficient to
> convert data? Is it something we want regarding performance?

I think that converting the data should be enough, however Michael can give
a more knowledgeable answer, we'll have to wait a few days for him as
he is not available this week.


Thanks,
Marcel

>
Thomas Monjalon Nov. 2, 2015, 10:13 p.m. UTC | #15 
> > Make vhost-user virtio 1.0 compatible by adding it to the
> > supported features and keeping the header length
> > the same as for mergeable RX buffers.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> 
> Looks good to me
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Applied, thanks
Xu, Qian Q Nov. 3, 2015, 3:45 a.m. UTC | #16 
DPDK GCC 64bit build on kernel 3.18 will be failed, could you help check?

== Build lib/librte_pipeline
/home/qxu10/virtio-opt-test/dpdk/lib/librte_vhost/virtio-net.c:81:106: error: 'VIRTIO_F_VERSION_1' undeclared here (not in a function)
static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
                                                                                                          ^
/home/qxu10/virtio-opt-test/dpdk/mk/internal/rte.compile-pre.mk:126: recipe for target 'virtio-net.o' failed
make[5]: *** [virtio-net.o] Error 1
/home/qxu10/virtio-opt-test/dpdk/mk/rte.subdir.mk:61: recipe for target 'librte_vhost' failed
make[4]: *** [librte_vhost] Error 2
make[4]: *** Waiting for unfinished jobs....
  SYMLINK-FILE include/rte_pipeline.h
  CC rte_pipeline.o
  AR librte_pipeline.a
  INSTALL-LIB librte_pipeline.a
/home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkbuild.mk:93: recipe for target 'lib' failed
make[3]: *** [lib] Error 2
/home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkroot.mk:124: recipe for target 'all' failed
make[2]: *** [all] Error 2
/home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkinstall.mk:58: recipe for target 'x86_64-native-linuxapp-gcc_install' failed
make[1]: *** [x86_64-native-linuxapp-gcc_install] Error 2
/home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkroot.mk:102: recipe for target 'install' failed
make: *** [install] Error 2

Thanks
Qian


-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
Sent: Tuesday, November 03, 2015 6:14 AM
To: Marcel Apfelbaum
Cc: dev@dpdk.org; Michael S. Tsirkin
Subject: Re: [dpdk-dev] [PATCH] vhost-user: enable virtio 1.0

> > Make vhost-user virtio 1.0 compatible by adding it to the supported 
> > features and keeping the header length the same as for mergeable RX 
> > buffers.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> 
> Looks good to me
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Applied, thanks
Xu, Qian Q Nov. 3, 2015, 3:49 a.m. UTC | #17 
Sorry, correct the kernel info, my kernel version is 4.1.8-100.fc21.x86_64. 

Thanks
Qian


-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xu, Qian Q
Sent: Tuesday, November 03, 2015 11:45 AM
To: Thomas Monjalon; Marcel Apfelbaum
Cc: dev@dpdk.org; Michael S. Tsirkin
Subject: Re: [dpdk-dev] [PATCH] vhost-user: enable virtio 1.0

DPDK GCC 64bit build on kernel 3.18 will be failed, could you help check?

== Build lib/librte_pipeline
/home/qxu10/virtio-opt-test/dpdk/lib/librte_vhost/virtio-net.c:81:106: error: 'VIRTIO_F_VERSION_1' undeclared here (not in a function) static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
                                                                                                          ^
/home/qxu10/virtio-opt-test/dpdk/mk/internal/rte.compile-pre.mk:126: recipe for target 'virtio-net.o' failed
make[5]: *** [virtio-net.o] Error 1
/home/qxu10/virtio-opt-test/dpdk/mk/rte.subdir.mk:61: recipe for target 'librte_vhost' failed
make[4]: *** [librte_vhost] Error 2
make[4]: *** Waiting for unfinished jobs....
  SYMLINK-FILE include/rte_pipeline.h
  CC rte_pipeline.o
  AR librte_pipeline.a
  INSTALL-LIB librte_pipeline.a
/home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkbuild.mk:93: recipe for target 'lib' failed
make[3]: *** [lib] Error 2
/home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkroot.mk:124: recipe for target 'all' failed
make[2]: *** [all] Error 2
/home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkinstall.mk:58: recipe for target 'x86_64-native-linuxapp-gcc_install' failed
make[1]: *** [x86_64-native-linuxapp-gcc_install] Error 2
/home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkroot.mk:102: recipe for target 'install' failed
make: *** [install] Error 2

Thanks
Qian


-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
Sent: Tuesday, November 03, 2015 6:14 AM
To: Marcel Apfelbaum
Cc: dev@dpdk.org; Michael S. Tsirkin
Subject: Re: [dpdk-dev] [PATCH] vhost-user: enable virtio 1.0

> > Make vhost-user virtio 1.0 compatible by adding it to the supported 
> > features and keeping the header length the same as for mergeable RX 
> > buffers.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> 
> Looks good to me
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Applied, thanks
Marcel Apfelbaum Nov. 3, 2015, 8:03 a.m. UTC | #18 
On 11/03/2015 05:49 AM, Xu, Qian Q wrote:
> Sorry, correct the kernel info, my kernel version is 4.1.8-100.fc21.x86_64.

Hi,

This is weird, VIRTIO_F_VERSION_1 is defined in 4.0 (I think), and for sure in 4.1 .
You can see commit 4ec22fae (virtio: add virtio 1.0 feature bit) from 2014-10-22 in kernel git.

Can you please check a mismatch in header files of your development machine?

Thanks,
Marcel

>
> Thanks
> Qian
>
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Xu, Qian Q
> Sent: Tuesday, November 03, 2015 11:45 AM
> To: Thomas Monjalon; Marcel Apfelbaum
> Cc: dev@dpdk.org; Michael S. Tsirkin
> Subject: Re: [dpdk-dev] [PATCH] vhost-user: enable virtio 1.0
>
> DPDK GCC 64bit build on kernel 3.18 will be failed, could you help check?
>
> == Build lib/librte_pipeline
> /home/qxu10/virtio-opt-test/dpdk/lib/librte_vhost/virtio-net.c:81:106: error: 'VIRTIO_F_VERSION_1' undeclared here (not in a function) static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
>                                                                                                            ^
> /home/qxu10/virtio-opt-test/dpdk/mk/internal/rte.compile-pre.mk:126: recipe for target 'virtio-net.o' failed
> make[5]: *** [virtio-net.o] Error 1
> /home/qxu10/virtio-opt-test/dpdk/mk/rte.subdir.mk:61: recipe for target 'librte_vhost' failed
> make[4]: *** [librte_vhost] Error 2
> make[4]: *** Waiting for unfinished jobs....
>    SYMLINK-FILE include/rte_pipeline.h
>    CC rte_pipeline.o
>    AR librte_pipeline.a
>    INSTALL-LIB librte_pipeline.a
> /home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkbuild.mk:93: recipe for target 'lib' failed
> make[3]: *** [lib] Error 2
> /home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkroot.mk:124: recipe for target 'all' failed
> make[2]: *** [all] Error 2
> /home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkinstall.mk:58: recipe for target 'x86_64-native-linuxapp-gcc_install' failed
> make[1]: *** [x86_64-native-linuxapp-gcc_install] Error 2
> /home/qxu10/virtio-opt-test/dpdk/mk/rte.sdkroot.mk:102: recipe for target 'install' failed
> make: *** [install] Error 2
>
> Thanks
> Qian
>
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, November 03, 2015 6:14 AM
> To: Marcel Apfelbaum
> Cc: dev@dpdk.org; Michael S. Tsirkin
> Subject: Re: [dpdk-dev] [PATCH] vhost-user: enable virtio 1.0
>
>>> Make vhost-user virtio 1.0 compatible by adding it to the supported
>>> features and keeping the header length the same as for mergeable RX
>>> buffers.
>>>
>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>
>> Looks good to me
>>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> Applied, thanks
>
Huawei Xie Nov. 3, 2015, 8:16 a.m. UTC | #19 
On 11/3/2015 4:03 PM, Marcel Apfelbaum wrote:
> On 11/03/2015 05:49 AM, Xu, Qian Q wrote:
>> Sorry, correct the kernel info, my kernel version is
>> 4.1.8-100.fc21.x86_64.
>
> Hi,
>
> This is weird, VIRTIO_F_VERSION_1 is defined in 4.0 (I think), and for
> sure in 4.1 .
> You can see commit 4ec22fae (virtio: add virtio 1.0 feature bit) from
> 2014-10-22 in kernel git.
>
> Can you please check a mismatch in header files of your development
> machine?
Marcel:
Would you prepare the ifdef fix? We have done the same thing in vhost
multiple queue patch.
>
> Thanks,
> Marcel
Thomas Monjalon Nov. 3, 2015, 8:26 a.m. UTC | #20 
2015-11-03 08:16, Xie, Huawei:
> On 11/3/2015 4:03 PM, Marcel Apfelbaum wrote:
> > On 11/03/2015 05:49 AM, Xu, Qian Q wrote:
> >> Sorry, correct the kernel info, my kernel version is
> >> 4.1.8-100.fc21.x86_64.
> >
> > Hi,
> >
> > This is weird, VIRTIO_F_VERSION_1 is defined in 4.0 (I think), and for
> > sure in 4.1 .
> > You can see commit 4ec22fae (virtio: add virtio 1.0 feature bit) from
> > 2014-10-22 in kernel git.
> >
> > Can you please check a mismatch in header files of your development
> > machine?
> Marcel:
> Would you prepare the ifdef fix? We have done the same thing in vhost
> multiple queue patch.

Everybody (including me) forgot the support of older kernel.
Please provide a fix ASAP.
Marcel Apfelbaum Nov. 3, 2015, 8:30 a.m. UTC | #21 
On 11/03/2015 10:26 AM, Thomas Monjalon wrote:
> 2015-11-03 08:16, Xie, Huawei:
>> On 11/3/2015 4:03 PM, Marcel Apfelbaum wrote:
>>> On 11/03/2015 05:49 AM, Xu, Qian Q wrote:
>>>> Sorry, correct the kernel info, my kernel version is
>>>> 4.1.8-100.fc21.x86_64.
>>>
>>> Hi,
>>>
>>> This is weird, VIRTIO_F_VERSION_1 is defined in 4.0 (I think), and for
>>> sure in 4.1 .
>>> You can see commit 4ec22fae (virtio: add virtio 1.0 feature bit) from
>>> 2014-10-22 in kernel git.
>>>
>>> Can you please check a mismatch in header files of your development
>>> machine?
>> Marcel:
>> Would you prepare the ifdef fix? We have done the same thing in vhost
>> multiple queue patch.
>
> Everybody (including me) forgot the support of older kernel.
> Please provide a fix ASAP.
>

Sure, I'll send a fix today. (on top of my prev patch)

Thanks,
Marcel
Marcel Apfelbaum Nov. 3, 2015, 8:31 a.m. UTC | #22 
On 11/03/2015 10:16 AM, Xie, Huawei wrote:
> On 11/3/2015 4:03 PM, Marcel Apfelbaum wrote:
>> On 11/03/2015 05:49 AM, Xu, Qian Q wrote:
>>> Sorry, correct the kernel info, my kernel version is
>>> 4.1.8-100.fc21.x86_64.
>>
>> Hi,
>>
>> This is weird, VIRTIO_F_VERSION_1 is defined in 4.0 (I think), and for
>> sure in 4.1 .
>> You can see commit 4ec22fae (virtio: add virtio 1.0 feature bit) from
>> 2014-10-22 in kernel git.
>>
>> Can you please check a mismatch in header files of your development
>> machine?
> Marcel:
> Would you prepare the ifdef fix? We have done the same thing in vhost
> multiple queue patch.

Yes, thanks for pointing me to the solution.
Marcel

>>
>> Thanks,
>> Marcel
>
Michael S. Tsirkin Nov. 9, 2015, 7:55 p.m. UTC | #23 
On Fri, Oct 30, 2015 at 06:48:09PM +0100, Thomas Monjalon wrote:
> 2015-10-18 10:04, Michael S. Tsirkin:
> > On Fri, Oct 16, 2015 at 02:52:30PM +0100, Bruce Richardson wrote:
> > > On Thu, Oct 15, 2015 at 04:18:59PM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Oct 15, 2015 at 02:08:39PM +0300, Marcel Apfelbaum wrote:
> > > > > Make vhost-user virtio 1.0 compatible by adding it to the
> > > > > supported features and keeping the header length
> > > > > the same as for mergeable RX buffers.
> > > > > 
> > > > > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> > > > 
> > > > Looks good to me
> > > > 
> > > > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > > 
> > > > Just one question: dpdk is only supported on little-endian
> > > > platforms at the moment, right?
> > > 
> > > A recent release added in support for PPC (patches supplied by IBM). For 
> > > example, see: 
> > > http://dpdk.org/browse/dpdk/commit/?id=704ba3770032c5a901719d3837845581d5a56b58
> > > 
> > > /Bruce
> > 
> > This will require more work then as 1.0 is a different
> > endian-ness from 0.9. It's up to you guys to decide
> > whether correct BE support is now a requirement for all
> > new dpdk code. Let us know.
> 
> I'm not sure to understand.
> Yes DPDK must work on big endian platforms.
> Does this patch prevent from using virtio 0.9 with big endian?

No it doesn't. virtio 0.9 with big endian most likely does not work
reliably with vhost-user at the moment since qemu does not tell vhost-user
about guest endian-ness (though it's common to have it match host,
then it will work by luck).

> Does it work with old QEMU not supporting virtio 1.0?

Yes it does.

virtio 1 requires a bunch of fields to be little endian.
If someone uses this patch with new QEMU on BE machine,
things won't work: you need
	if (virtio_1)
		cpu_to_le
On LE, cpu_to_le is a NOP so things work by luck.

In other words this patch is fine, more is needed to make virtio 1
work on BE hosts.
diff mbox

Patch

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index a51327d..ee4650e 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -75,6 +75,7 @@  static struct virtio_net_config_ll *ll_root;
 				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
 				(1ULL << VIRTIO_NET_F_CTRL_RX) | \
 				(1ULL << VIRTIO_NET_F_MQ)      | \
+				(1ULL << VIRTIO_F_VERSION_1)   | \
 				(1ULL << VHOST_F_LOG_ALL)      | \
 				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
 static uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
@@ -477,17 +478,17 @@  set_features(struct vhost_device_ctx ctx, uint64_t *pu)
 		return -1;
 
 	dev->features = *pu;
-	if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
-		LOG_DEBUG(VHOST_CONFIG,
-			"(%"PRIu64") Mergeable RX buffers enabled\n",
-			dev->device_fh);
+	if (dev->features &
+	    ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
 		vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	} else {
-		LOG_DEBUG(VHOST_CONFIG,
-			"(%"PRIu64") Mergeable RX buffers disabled\n",
-			dev->device_fh);
 		vhost_hlen = sizeof(struct virtio_net_hdr);
 	}
+	LOG_DEBUG(VHOST_CONFIG,
+              "(%"PRIu64") Mergeable RX buffers %s, virtio 1 %s\n",
+	          dev->device_fh,
+              (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
+              (dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
 
 	for (i = 0; i < dev->virt_qp_nb; i++) {
 		uint16_t base_idx = i * VIRTIO_QNUM;