vhost-user: enable virtio 1.0

PATCH Applied Marcel Apfelbaum 8 participants 24 messages Oct 15, 2015 – Nov 9, 2015

Share & tools

Marcel Apfelbaum

author

2015-10-15 11:08 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---

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(-)

lib/librte_vhost/virtio-net.c +8 -7

index a51327d..ee4650e 100644
@@ -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;

Signature · 1 line

2.1.0

Michael S. Tsirkin

Acked-by on v1 1/1

2015-10-15 13:19 UTC #1 lore raw

Quoted 5 lines · Marcel Apfelbaum

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 <[email protected]>

Looks good to me

Acked-by Michael S. Tsirkin <[email protected]>

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

Quoted 50 lines

---

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(-)

lib/librte_vhost/virtio-net.c +8 -7

index a51327d..ee4650e 100644
@@ -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;

Signature · 1 line

2.1.0

Yuanhan Liu

2015-10-16 02:23 UTC #2 lore raw

Quoted 6 lines · Michael S. Tsirkin

Quoted 5 lines · Marcel Apfelbaum
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 <[email protected]>

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

Quoted 7 lines

Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>
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.

Quoted 1 line

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

Quoted 50 lines

---

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(-)

lib/librte_vhost/virtio-net.c +8 -7

index a51327d..ee4650e 100644
@@ -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;

Signature · 1 line

2.1.0

Michael S. Tsirkin

2015-10-16 06:20 UTC #3 lore raw

Quoted 14 lines · Yuanhan Liu

Quoted 6 lines · Michael S. Tsirkin
Quoted 5 lines · Marcel Apfelbaum
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 <[email protected]>
Marcel, that's actually one of my TODOs in this quarter. So, thank you! :)
Quoted 3 lines
Looks good to me

Quoted 7 lines

Quoted 4 lines
Acked-by Michael S. Tsirkin <[email protected]>
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.

Quoted 5 lines

Quoted 1 line
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:

Quoted 9 lines

    +-------------------+-----------------+------------------------------+
    |                   |     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.

Quoted 6 lines

    +-------------------+-----------------+------------------------------+
4)  | FEATURE_OK status |     No          |   Yes                        |
    +-------------------+-----------------+------------------------------+

Quoted 6 lines

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

Quoted 4 lines

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?

Quoted 8 lines

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.

Quoted 5 lines

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):

Quoted 8 lines

- 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.

Quoted 53 lines

	--yliu

Quoted 50 lines

---

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(-)

lib/librte_vhost/virtio-net.c +8 -7

index a51327d..ee4650e 100644
@@ -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;

Signature · 1 line

2.1.0

Yuanhan Liu

2015-10-16 06:36 UTC #4 lore raw

Quoted 17 lines · Michael S. Tsirkin

Quoted 16 lines · Yuanhan Liu
Quoted 6 lines · Michael S. Tsirkin
Quoted 5 lines · Marcel Apfelbaum
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 <[email protected]>
Marcel, that's actually one of my TODOs in this quarter. So, thank you! :)
Quoted 5 lines
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
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.

Quoted 16 lines

Quoted 6 lines

Quoted 2 lines
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:

Quoted 9 lines

    +-------------------+-----------------+------------------------------+
    |                   |     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     |

Quoted 1 line

And desc buffer, too.

Thanks for the remind.

Quoted 7 lines

    +-------------------+-----------------+------------------------------+
4)  | FEATURE_OK status |     No          |   Yes                        |
    +-------------------+-----------------+------------------------------+

Quoted 8 lines

Quoted 6 lines
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.

Quoted 10 lines

Quoted 5 lines
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.

Quoted 10 lines

Quoted 9 lines
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.

Quoted 2 lines

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

Thanks for the tip!

Quoted 8 lines

Quoted 8 lines
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).

Quoted 8 lines

Quoted 6 lines
- 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

Quoted 52 lines

---

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(-)

lib/librte_vhost/virtio-net.c +8 -7

index a51327d..ee4650e 100644
@@ -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;

Signature · 1 line

2.1.0

Andriy Berestovskyy

2015-10-16 07:43 UTC #5 lore raw

In reply to Michael S. Tsirkin

Hi guys,
Just a minor note: ARM is bi-endian in fact. For instance, there are both endians tool chains available on Linaro.

Andriy

Quoted 147 lines · Michael S. Tsirkin

Quoted 14 lines · Yuanhan Liu
Quoted 6 lines · Michael S. Tsirkin
Quoted 5 lines · Marcel Apfelbaum
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 <[email protected]>
Marcel, that's actually one of my TODOs in this quarter. So, thank you! :)
Quoted 3 lines
Looks good to me
Quoted 7 lines
Quoted 4 lines
Acked-by Michael S. Tsirkin <[email protected]>
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.
Quoted 5 lines
Quoted 1 line
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:
Quoted 9 lines
    +-------------------+-----------------+------------------------------+
    |                   |     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.
Quoted 6 lines
    +-------------------+-----------------+------------------------------+
4)  | FEATURE_OK status |     No          |   Yes                        |
    +-------------------+-----------------+------------------------------+
Quoted 6 lines
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
Quoted 4 lines
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?
Quoted 5 lines
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.
Quoted 5 lines
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):
Quoted 8 lines
- 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.
Quoted 53 lines
      --yliu
Quoted 50 lines
Quoted 50 lines
---
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(-)
lib/librte_vhost/virtio-net.c +8 -8
index a51327d..ee4650e 100644
@@ -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

Signature · 1 line

Andriy Berestovskyy

Yuanhan Liu

2015-10-16 07:49 UTC #6 lore raw

Quoted 2 lines · Andriy Berestovskyy

Hi guys,
Just a minor note: ARM is bi-endian in fact.

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

	--yliu

Quoted 159 lines

For instance, there are
both endians tool chains available on Linaro.
Andriy
Quoted 147 lines · Michael S. Tsirkin
Quoted 14 lines · Yuanhan Liu
Quoted 6 lines · Michael S. Tsirkin
Quoted 5 lines · Marcel Apfelbaum
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 <[email protected]>
Marcel, that's actually one of my TODOs in this quarter. So, thank you! :)
Quoted 3 lines
Looks good to me
Quoted 7 lines
Quoted 4 lines
Acked-by Michael S. Tsirkin <[email protected]>
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.
Quoted 5 lines
Quoted 1 line
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:
Quoted 9 lines
    +-------------------+-----------------+------------------------------+
    |                   |     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.
Quoted 6 lines
    +-------------------+-----------------+------------------------------+
4)  | FEATURE_OK status |     No          |   Yes                        |
    +-------------------+-----------------+------------------------------+
Quoted 6 lines
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
Quoted 4 lines
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?
Quoted 5 lines
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.
Quoted 5 lines
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):
Quoted 8 lines
- 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.
Quoted 53 lines
      --yliu
Quoted 50 lines
Quoted 50 lines
---
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(-)
lib/librte_vhost/virtio-net.c +8 -8
index a51327d..ee4650e 100644
@@ -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
Signature · 1 line
Andriy Berestovskyy

Michael S. Tsirkin

2015-10-16 07:56 UTC #7 lore raw

In reply to Andriy Berestovskyy

Quoted 5 lines · Andriy Berestovskyy

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.

Signature · 1 line

MST

Bruce Richardson

2015-10-16 13:52 UTC #8 lore raw

In reply to Michael S. Tsirkin

Quoted 5 lines · Michael S. Tsirkin

Quoted 4 lines · Marcel Apfelbaum
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.

Quoted 8 lines

Quoted 1 line
Signed-off-by Marcel Apfelbaum <[email protected]>
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>
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

2015-10-18 07:04 UTC #9 lore raw

Quoted 12 lines · Bruce Richardson

Quoted 11 lines · Michael S. Tsirkin
Quoted 5 lines · Marcel Apfelbaum
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 <[email protected]>
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>

Quoted 8 lines

Quoted 2 lines
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.

Signature · 1 line

MST

Thomas Monjalon

2015-10-30 17:49 UTC #10 lore raw

2015-10-18 10:04, Michael S. Tsirkin:

Quoted 20 lines · Bruce Richardson

Quoted 19 lines · Michael S. Tsirkin
Quoted 5 lines · Marcel Apfelbaum
Quoted 4 lines
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.
Quoted 8 lines
Quoted 1 line
Signed-off-by Marcel Apfelbaum <[email protected]>
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>
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

Quoted 6 lines

Quoted 1 line
/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

author

2015-11-01 09:01 UTC #11 lore raw

Quoted 28 lines · Thomas Monjalon

2015-10-18 10:04, Michael S. Tsirkin:
Quoted 26 lines · Bruce Richardson
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Marcel Apfelbaum
Quoted 5 lines
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 <[email protected]>
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>
Quoted 8 lines
Quoted 2 lines
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,

Quoted 3 lines

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.

Quoted 1 line

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

Quoted 1 line

Thomas Monjalon

2015-11-01 09:55 UTC #12 lore raw

2015-11-01 11:00, Marcel Apfelbaum:

Quoted 7 lines · Thomas Monjalon

Quoted 5 lines
2015-10-18 10:04, Michael S. Tsirkin:
Quoted 4 lines
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.

Quoted 8 lines

Quoted 3 lines
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

author

2015-11-01 11:22 UTC #13 lore raw

Quoted 14 lines · Thomas Monjalon

2015-11-01 11:00, Marcel Apfelbaum:
Quoted 13 lines · Thomas Monjalon
Quoted 9 lines
2015-10-18 10:04, Michael S. Tsirkin:
Quoted 4 lines
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)

Quoted 8 lines

Quoted 2 lines
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

Quoted 1 line

Michael S. Tsirkin

2015-11-09 19:55 UTC #14 lore raw

In reply to Thomas Monjalon

Quoted 23 lines · Thomas Monjalon

2015-10-18 10:04, Michael S. Tsirkin:
Quoted 22 lines · Bruce Richardson
Quoted 20 lines · Michael S. Tsirkin
Quoted 5 lines · Marcel Apfelbaum
Quoted 4 lines
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.
Quoted 8 lines
Quoted 1 line
Signed-off-by Marcel Apfelbaum <[email protected]>
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>
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

Quoted 8 lines

Quoted 4 lines
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).

Quoted 1 line

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.

Signature · 1 line

MST

Thomas Monjalon

Applied

2015-11-02 22:15 UTC #15 lore raw

In reply to Michael S. Tsirkin

Quoted 9 lines

Quoted 5 lines
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 <[email protected]>
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>

Applied, thanks

Xu, Qian Q

Applied

2015-11-03 03:45 UTC #16 lore raw

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:[email protected]] On Behalf Of Thomas Monjalon
Sent: Tuesday, November 03, 2015 6:14 AM
To: Marcel Apfelbaum
Cc: [email protected]; Michael S. Tsirkin
Subject: Re: [dpdk-dev] [PATCH] vhost-user: enable virtio 1.0

Quoted 9 lines

Quoted 5 lines
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 <[email protected]>
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>

Applied, thanks

Xu, Qian Q

Applied

2015-11-03 03:49 UTC #17 lore raw

Sorry, correct the kernel info, my kernel version is 4.1.8-100.fc21.x86_64.

Thanks
Qian

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Xu, Qian Q
Sent: Tuesday, November 03, 2015 11:45 AM
To: Thomas Monjalon; Marcel Apfelbaum
Cc: [email protected]; 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;
                                                                                                          ^

Thanks
Qian

Quoted 9 lines

Quoted 5 lines
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 <[email protected]>
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>

Applied, thanks

Marcel Apfelbaum

author

2015-11-03 08:03 UTC #18 lore raw

Quoted 1 line · Qian Q

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

Quoted 58 lines

Thanks
Qian
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Xu, Qian Q
Sent: Tuesday, November 03, 2015 11:45 AM
To: Thomas Monjalon; Marcel Apfelbaum
Cc: [email protected]; 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:[email protected]] On Behalf Of Thomas Monjalon
Sent: Tuesday, November 03, 2015 6:14 AM
To: Marcel Apfelbaum
Cc: [email protected]; Michael S. Tsirkin
Subject: Re: [dpdk-dev] [PATCH] vhost-user: enable virtio 1.0
Quoted 9 lines
Quoted 5 lines
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 <[email protected]>
Looks good to me
Acked-by Michael S. Tsirkin <[email protected]>
Applied, thanks

Xie, Huawei

2015-11-03 08:16 UTC #19 lore raw

Quoted 6 lines · Marcel Apfelbaum

Quoted 2 lines · Qian Q
Sorry, correct the kernel info, my kernel version is 4.1.8-100.fc21.x86_64.
Hi,

Quoted 7 lines

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.

Quoted 3 lines

Thanks,
Marcel

Thomas Monjalon

2015-11-03 08:27 UTC #20 lore raw

2015-11-03 08:16, Xie, Huawei:

Quoted 12 lines · Marcel Apfelbaum

Quoted 11 lines · Qian Q
Quoted 2 lines
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.

Quoted 5 lines

Quoted 2 lines
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

author

2015-11-03 08:30 UTC #21 lore raw

Quoted 13 lines · Thomas Monjalon

2015-11-03 08:16, Xie, Huawei:
Quoted 12 lines · Marcel Apfelbaum
Quoted 11 lines · Qian Q
Quoted 2 lines
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.

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
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

author

2015-11-03 08:31 UTC #22 lore raw

In reply to Xie, Huawei

Quoted 12 lines · Huawei

Quoted 11 lines · Marcel Apfelbaum
Quoted 2 lines · Qian Q
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.

Quoted 5 lines

Quoted 2 lines
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

Quoted 4 lines

Quoted 3 lines
Thanks,
Marcel

Yuanhan Liu

Reviewed-by on v1 1/1

2015-10-16 06:38 UTC #23 lore raw

In reply to Marcel Apfelbaum

Reviewed-by Yuanhan Liu <[email protected]>

And thanks for the work.

	--yliu

Quoted 55 lines · Marcel Apfelbaum

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 <[email protected]>

---

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(-)

lib/librte_vhost/virtio-net.c +8 -7

index a51327d..ee4650e 100644
@@ -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;

Signature · 1 line

2.1.0