[Qemu-devel] [PATCH] vhost_net: do not expose MTU feature bit to kernel backend

[Qemu-devel] [PATCH] vhost_net: do not expose MTU feature bit to kernel backend

[Maxime Coquelin]

This patch removes MTU from the list of features supported by
the kernel backend, so that vhost kernel backend does not have
to advertise it to be negotiated.

Cc: Aaron Conole <aconole@redhat.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/net/vhost_net.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 22874a9..bdd8633 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -51,7 +51,6 @@ static const int kernel_feature_bits[] = {
     VIRTIO_RING_F_EVENT_IDX,
     VIRTIO_NET_F_MRG_RXBUF,
     VIRTIO_F_VERSION_1,
-    VIRTIO_NET_F_MTU,
     VIRTIO_F_IOMMU_PLATFORM,
     VHOST_INVALID_FEATURE_BIT
 };
-- 
2.9.3

Re: [Qemu-devel] [PATCH] vhost_net: do not expose MTU feature bit to kernel backend

[Aaron Conole]

Maxime Coquelin <maxime.coquelin@redhat.com> writes:

> This patch removes MTU from the list of features supported by
> the kernel backend, so that vhost kernel backend does not have
> to advertise it to be negotiated.
>
> Cc: Aaron Conole <aconole@redhat.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>

Re: [Qemu-devel] [PATCH] vhost_net: do not expose MTU feature bit to kernel backend

[Michael S. Tsirkin]

On Sat, May 20, 2017 at 10:06:58AM +0200, Maxime Coquelin wrote:
> This patch removes MTU from the list of features supported by
> the kernel backend, so that vhost kernel backend does not have
> to advertise it to be negotiated.
> 
> Cc: Aaron Conole <aconole@redhat.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

This will break cross-version migration when the kernel
does not advertise it though. I'm afraid we'll need
a compat flag.

> ---
>  hw/net/vhost_net.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 22874a9..bdd8633 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -51,7 +51,6 @@ static const int kernel_feature_bits[] = {
>      VIRTIO_RING_F_EVENT_IDX,
>      VIRTIO_NET_F_MRG_RXBUF,
>      VIRTIO_F_VERSION_1,
> -    VIRTIO_NET_F_MTU,
>      VIRTIO_F_IOMMU_PLATFORM,
>      VHOST_INVALID_FEATURE_BIT
>  };
> -- 
> 2.9.3

Re: [Qemu-devel] [PATCH] vhost_net: do not expose MTU feature bit to kernel backend

[Maxime Coquelin]

On 05/22/2017 07:24 PM, Michael S. Tsirkin wrote:
> On Sat, May 20, 2017 at 10:06:58AM +0200, Maxime Coquelin wrote:
>> This patch removes MTU from the list of features supported by
>> the kernel backend, so that vhost kernel backend does not have
>> to advertise it to be negotiated.
>>
>> Cc: Aaron Conole <aconole@redhat.com>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> This will break cross-version migration when the kernel
> does not advertise it though. I'm afraid we'll need
> a compat flag.

Ok, you are right.
The difficulty with a compat flag is that vhost-net is not a driver,
so we cannot add it a new property to make it kernel-backend only.

For consistency, maybe we can add a new internal property to virtio-net
driver, to bypass backend negotiation for this feature for all backends?

I will propose a new version shortly.

Thanks,
Maxime
>> ---
>>   hw/net/vhost_net.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index 22874a9..bdd8633 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -51,7 +51,6 @@ static const int kernel_feature_bits[] = {
>>       VIRTIO_RING_F_EVENT_IDX,
>>       VIRTIO_NET_F_MRG_RXBUF,
>>       VIRTIO_F_VERSION_1,
>> -    VIRTIO_NET_F_MTU,
>>       VIRTIO_F_IOMMU_PLATFORM,
>>       VHOST_INVALID_FEATURE_BIT
>>   };
>> -- 
>> 2.9.3

[Qemu-devel] [PATCH v2] virtio_net: Bypass backends for MTU feature negotiation

[Maxime Coquelin]

This patch adds a new internal "x-mtu-bypass-backend" property
to bypass backends for MTU feature negotiation.

When this property is set, the MTU feature is negotiated as soon
as supported by the guest and a MTU value is set via the host_mtu
parameter. In case the backend advertises the feature (e.g. DPDK's
vhost-user backend), the feature negotiation is propagated down to
the backend.

When this property is not set, the backend has to support the MTU
feature for its negotiation to succeed.

For compatibility purpose, this property is disabled for machine
types v2.9 and older.

Cc: Aaron Conole <aconole@redhat.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---

Tests performed:
- Vhost-net kernel backendi, host_mtu=2000:
 * default machine type: guest MTU = 2000
 * pc-q35-2.9 machine type: Guest MTU = 1500

- Vhost-net user backend, host_mtu=2000:
 * DPDK v17.05 (MTU feature supported on backend side)
  - default machine type: guest MTU = 2000
  - pc-q35-2.9 machine type: guest MTU = 2000)
 * DPDK v16.11 (MTU feature not supported on backend side)
  - default machine type: guest MTU = 2000
  - pc-q35-2.9 machine type: guest MTU = 1500)

 hw/net/virtio-net.c            | 17 ++++++++++++++++-
 include/hw/compat.h            |  4 ++++
 include/hw/virtio/virtio-net.h |  1 +
 include/hw/virtio/virtio.h     |  1 +
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7d091c9..39c336e 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -589,7 +589,15 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
     if (!get_vhost_net(nc->peer)) {
         return features;
     }
-    return vhost_net_get_features(get_vhost_net(nc->peer), features);
+    features = vhost_net_get_features(get_vhost_net(nc->peer), features);
+    vdev->backend_features = features;
+
+    if (n->mtu_bypass_backend &&
+            (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
+        features |= (1ULL << VIRTIO_NET_F_MTU);
+    }
+
+    return features;
 }
 
 static uint64_t virtio_net_bad_features(VirtIODevice *vdev)
@@ -640,6 +648,11 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
     VirtIONet *n = VIRTIO_NET(vdev);
     int i;
 
+    if (n->mtu_bypass_backend &&
+            !virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_MTU)) {
+        features &= ~(1ULL << VIRTIO_NET_F_MTU);
+    }
+
     virtio_net_set_multiqueue(n,
                               virtio_has_feature(features, VIRTIO_NET_F_MQ));
 
@@ -2090,6 +2103,8 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT16("rx_queue_size", VirtIONet, net_conf.rx_queue_size,
                        VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE),
     DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
+    DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
+                     true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 55b1765..181f450 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -6,6 +6,10 @@
         .driver   = "pci-bridge",\
         .property = "shpc",\
         .value    = "off",\
+    },{\
+        .driver   = "virtio-net-device",\
+        .property = "x-mtu-bypass-backend",\
+        .value    = "off",\
     },
 
 #define HW_COMPAT_2_8 \
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 1eec9a2..602b486 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -97,6 +97,7 @@ typedef struct VirtIONet {
     QEMUTimer *announce_timer;
     int announce_counter;
     bool needs_vnet_hdr_swap;
+    bool mtu_bypass_backend;
 } VirtIONet;
 
 void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 7b6edba..80c45c3 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -79,6 +79,7 @@ struct VirtIODevice
     uint16_t queue_sel;
     uint64_t guest_features;
     uint64_t host_features;
+    uint64_t backend_features;
     size_t config_len;
     void *config;
     uint16_t config_vector;
-- 
2.9.4

Re: [Qemu-devel] [PATCH v2] virtio_net: Bypass backends for MTU feature negotiation

[Vlad Yasevich]

On 05/23/2017 08:31 AM, Maxime Coquelin wrote:
> This patch adds a new internal "x-mtu-bypass-backend" property
> to bypass backends for MTU feature negotiation.
> 
> When this property is set, the MTU feature is negotiated as soon
> as supported by the guest and a MTU value is set via the host_mtu
> parameter. In case the backend advertises the feature (e.g. DPDK's
> vhost-user backend), the feature negotiation is propagated down to
> the backend.
> 
> When this property is not set, the backend has to support the MTU
> feature for its negotiation to succeed.
> 
> For compatibility purpose, this property is disabled for machine
> types v2.9 and older.
> 
> Cc: Aaron Conole <aconole@redhat.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad

> ---
> 
> Tests performed:
> - Vhost-net kernel backendi, host_mtu=2000:
>  * default machine type: guest MTU = 2000
>  * pc-q35-2.9 machine type: Guest MTU = 1500
> 
> - Vhost-net user backend, host_mtu=2000:
>  * DPDK v17.05 (MTU feature supported on backend side)
>   - default machine type: guest MTU = 2000
>   - pc-q35-2.9 machine type: guest MTU = 2000)
>  * DPDK v16.11 (MTU feature not supported on backend side)
>   - default machine type: guest MTU = 2000
>   - pc-q35-2.9 machine type: guest MTU = 1500)
> 
>  hw/net/virtio-net.c            | 17 ++++++++++++++++-
>  include/hw/compat.h            |  4 ++++
>  include/hw/virtio/virtio-net.h |  1 +
>  include/hw/virtio/virtio.h     |  1 +
>  4 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7d091c9..39c336e 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -589,7 +589,15 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>      if (!get_vhost_net(nc->peer)) {
>          return features;
>      }
> -    return vhost_net_get_features(get_vhost_net(nc->peer), features);
> +    features = vhost_net_get_features(get_vhost_net(nc->peer), features);
> +    vdev->backend_features = features;
> +
> +    if (n->mtu_bypass_backend &&
> +            (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) {
> +        features |= (1ULL << VIRTIO_NET_F_MTU);
> +    }
> +
> +    return features;
>  }
>  
>  static uint64_t virtio_net_bad_features(VirtIODevice *vdev)
> @@ -640,6 +648,11 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
>      VirtIONet *n = VIRTIO_NET(vdev);
>      int i;
>  
> +    if (n->mtu_bypass_backend &&
> +            !virtio_has_feature(vdev->backend_features, VIRTIO_NET_F_MTU)) {
> +        features &= ~(1ULL << VIRTIO_NET_F_MTU);
> +    }
> +
>      virtio_net_set_multiqueue(n,
>                                virtio_has_feature(features, VIRTIO_NET_F_MQ));
>  
> @@ -2090,6 +2103,8 @@ static Property virtio_net_properties[] = {
>      DEFINE_PROP_UINT16("rx_queue_size", VirtIONet, net_conf.rx_queue_size,
>                         VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE),
>      DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
> +    DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
> +                     true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 55b1765..181f450 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -6,6 +6,10 @@
>          .driver   = "pci-bridge",\
>          .property = "shpc",\
>          .value    = "off",\
> +    },{\
> +        .driver   = "virtio-net-device",\
> +        .property = "x-mtu-bypass-backend",\
> +        .value    = "off",\
>      },
>  
>  #define HW_COMPAT_2_8 \
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 1eec9a2..602b486 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -97,6 +97,7 @@ typedef struct VirtIONet {
>      QEMUTimer *announce_timer;
>      int announce_counter;
>      bool needs_vnet_hdr_swap;
> +    bool mtu_bypass_backend;
>  } VirtIONet;
>  
>  void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 7b6edba..80c45c3 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -79,6 +79,7 @@ struct VirtIODevice
>      uint16_t queue_sel;
>      uint64_t guest_features;
>      uint64_t host_features;
> +    uint64_t backend_features;
>      size_t config_len;
>      void *config;
>      uint16_t config_vector;
>