Re: [PATCH] vhost-user: enable virtio 1.0

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

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