Patchwork [PATCHv2] virtio-net: remove mq feature flag

login
register
mail settings
Submitter Michael S. Tsirkin
Date Feb. 4, 2013, 7:41 a.m.
Message ID <20130204074147.GA7753@redhat.com>
Download mbox | patch
Permalink /patch/217837/
State New
Headers show

Comments

Michael S. Tsirkin - Feb. 4, 2013, 7:41 a.m.
mq flag is not needed: we can look at the number of queues and set
the flag accordingly.
Removing this feature removes ambiguity (what does it mean to have
queues=2 with mq=off?), and simplifies compatibility hacks.
work-around for buggy windows
guests.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pc_piix.c    |  4 ----
 hw/virtio-net.c |  8 +++++++-
 hw/virtio-net.h | 15 ++++++++++++---
 3 files changed, 19 insertions(+), 8 deletions(-)
Jason Wang - Feb. 4, 2013, 8:21 a.m.
On 02/04/2013 03:41 PM, Michael S. Tsirkin wrote:
> mq flag is not needed: we can look at the number of queues and set
> the flag accordingly.
> Removing this feature removes ambiguity (what does it mean to have
> queues=2 with mq=off?), and simplifies compatibility hacks.
> work-around for buggy windows
> guests.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Looks a nice fix.
Acked-by: Jason Wang <jasowang@redhat.com>

Tested with windows guest and Linux guest (both mq and sq). So:

Tested-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/pc_piix.c    |  4 ----
>  hw/virtio-net.c |  8 +++++++-
>  hw/virtio-net.h | 15 ++++++++++++---
>  3 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 0af436c..ba09714 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -313,10 +313,6 @@ static QEMUMachine pc_i440fx_machine_v1_4 = {
>              .driver   = "virtio-net-pci",\
>              .property = "ctrl_mac_addr",\
>              .value    = "off",      \
> -        },{ \
> -            .driver   = "virtio-net-pci", \
> -            .property = "mq", \
> -            .value    = "off", \
>          }
>  
>  static QEMUMachine pc_machine_v1_3 = {
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index e37358a..8db1787 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -346,6 +346,10 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
>  
>      features |= (1 << VIRTIO_NET_F_MAC);
>  
> +    if (n->max_queues > 1) {
> +        features |= (0x1 << VIRTIO_NET_F_MQ);
> +    }
> +
>      if (!peer_has_vnet_hdr(n)) {
>          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
>          features &= ~(0x1 << VIRTIO_NET_F_HOST_TSO4);
> @@ -1285,7 +1289,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
>      int i;
>  
>      n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
> -                                        sizeof(struct virtio_net_config),
> +                                        conf->queues > 1 ?
> +                                            sizeof(struct virtio_net_config) :
> +                                            sizeof(struct virtio_net_config_compat),
>                                          sizeof(VirtIONet));
>  
>      n->vdev.get_config = virtio_net_get_config;
> diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> index f5fea6e..b1f7647 100644
> --- a/hw/virtio-net.h
> +++ b/hw/virtio-net.h
> @@ -69,6 +69,17 @@ typedef struct virtio_net_conf
>  /* Maximum packet size we can receive from tap device: header + 64k */
>  #define VIRTIO_NET_MAX_BUFSIZE (sizeof(struct virtio_net_hdr) + (64 << 10))
>  
> +/*
> + * Windows drivers from 22 Jan 2013 and older fail when config size != 32.
> + */
> +struct virtio_net_config_compat
> +{
> +    /* The config defining mac address ($ETH_ALEN bytes) */
> +    uint8_t mac[ETH_ALEN];
> +    /* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
> +    uint16_t status;
> +} QEMU_PACKED;
> +
>  struct virtio_net_config
>  {
>      /* The config defining mac address ($ETH_ALEN bytes) */
> @@ -190,7 +201,5 @@ struct virtio_net_ctrl_mq {
>          DEFINE_PROP_BIT("ctrl_rx", _state, _field, VIRTIO_NET_F_CTRL_RX, true), \
>          DEFINE_PROP_BIT("ctrl_vlan", _state, _field, VIRTIO_NET_F_CTRL_VLAN, true), \
>          DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true), \
> -        DEFINE_PROP_BIT("ctrl_mac_addr", _state, _field, VIRTIO_NET_F_CTRL_MAC_ADDR, true), \
> -        DEFINE_PROP_BIT("mq", _state, _field, VIRTIO_NET_F_MQ, true)
> -
> +        DEFINE_PROP_BIT("ctrl_mac_addr", _state, _field, VIRTIO_NET_F_CTRL_MAC_ADDR, true)
>  #endif
Michael Tokarev - Feb. 4, 2013, 8:26 a.m.
04.02.2013 11:41, Michael S. Tsirkin wrote:
> mq flag is not needed: we can look at the number of queues and set
> the flag accordingly.
> Removing this feature removes ambiguity (what does it mean to have
> queues=2 with mq=off?), and simplifies compatibility hacks.
> work-around for buggy windows
> guests.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Nice fix.  I tested it with a few non-mq guests (winXP and win7
with slightly older drivers and with current (but still buggy)
drivers), and linux both mq and non-mq - it appears to work just
fine.

Tested-By: Michael Tokarev <mjt@tls.msk.ru>

With one comment below:

> @@ -1285,7 +1289,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
>       int i;
>
>       n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
> -                                        sizeof(struct virtio_net_config),
> +                                        conf->queues > 1 ?
> +                                            sizeof(struct virtio_net_config) :
> +                                            sizeof(struct virtio_net_config_compat),
>                                           sizeof(VirtIONet));

Please add a comment in this place describing what we're doing.
Later on it will be less easy to remember or to find this
thread, and the code does not tell what's going on and why
this is needed ;)   You added a tiny comment around the definition
of virtio_net_config_compat struct, but it does not explain why
the criteria to enable it is conf->queues being larger than 1.


Thank you!

/mjt
Stefan Hajnoczi - Feb. 4, 2013, 8:50 a.m.
On Mon, Feb 04, 2013 at 09:41:47AM +0200, Michael S. Tsirkin wrote:
> mq flag is not needed: we can look at the number of queues and set
> the flag accordingly.
> Removing this feature removes ambiguity (what does it mean to have
> queues=2 with mq=off?), and simplifies compatibility hacks.
> work-around for buggy windows
> guests.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pc_piix.c    |  4 ----
>  hw/virtio-net.c |  8 +++++++-
>  hw/virtio-net.h | 15 ++++++++++++---
>  3 files changed, 19 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Blue Swirl - Feb. 4, 2013, 6:27 p.m.
On Mon, Feb 4, 2013 at 7:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> mq flag is not needed: we can look at the number of queues and set
> the flag accordingly.
> Removing this feature removes ambiguity (what does it mean to have
> queues=2 with mq=off?), and simplifies compatibility hacks.
> work-around for buggy windows
> guests.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/pc_piix.c    |  4 ----
>  hw/virtio-net.c |  8 +++++++-
>  hw/virtio-net.h | 15 ++++++++++++---
>  3 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 0af436c..ba09714 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -313,10 +313,6 @@ static QEMUMachine pc_i440fx_machine_v1_4 = {
>              .driver   = "virtio-net-pci",\
>              .property = "ctrl_mac_addr",\
>              .value    = "off",      \
> -        },{ \
> -            .driver   = "virtio-net-pci", \
> -            .property = "mq", \
> -            .value    = "off", \
>          }
>
>  static QEMUMachine pc_machine_v1_3 = {
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index e37358a..8db1787 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -346,6 +346,10 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
>
>      features |= (1 << VIRTIO_NET_F_MAC);
>
> +    if (n->max_queues > 1) {
> +        features |= (0x1 << VIRTIO_NET_F_MQ);
> +    }
> +
>      if (!peer_has_vnet_hdr(n)) {
>          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
>          features &= ~(0x1 << VIRTIO_NET_F_HOST_TSO4);
> @@ -1285,7 +1289,9 @@ VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
>      int i;
>
>      n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
> -                                        sizeof(struct virtio_net_config),
> +                                        conf->queues > 1 ?
> +                                            sizeof(struct virtio_net_config) :
> +                                            sizeof(struct virtio_net_config_compat),
>                                          sizeof(VirtIONet));
>
>      n->vdev.get_config = virtio_net_get_config;
> diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> index f5fea6e..b1f7647 100644
> --- a/hw/virtio-net.h
> +++ b/hw/virtio-net.h
> @@ -69,6 +69,17 @@ typedef struct virtio_net_conf
>  /* Maximum packet size we can receive from tap device: header + 64k */
>  #define VIRTIO_NET_MAX_BUFSIZE (sizeof(struct virtio_net_hdr) + (64 << 10))
>
> +/*
> + * Windows drivers from 22 Jan 2013 and older fail when config size != 32.
> + */
> +struct virtio_net_config_compat

VirtioNetConfigCompat, don't forget the typedef.

> +{
> +    /* The config defining mac address ($ETH_ALEN bytes) */
> +    uint8_t mac[ETH_ALEN];
> +    /* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
> +    uint16_t status;
> +} QEMU_PACKED;
> +
>  struct virtio_net_config
>  {
>      /* The config defining mac address ($ETH_ALEN bytes) */
> @@ -190,7 +201,5 @@ struct virtio_net_ctrl_mq {
>          DEFINE_PROP_BIT("ctrl_rx", _state, _field, VIRTIO_NET_F_CTRL_RX, true), \
>          DEFINE_PROP_BIT("ctrl_vlan", _state, _field, VIRTIO_NET_F_CTRL_VLAN, true), \
>          DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true), \
> -        DEFINE_PROP_BIT("ctrl_mac_addr", _state, _field, VIRTIO_NET_F_CTRL_MAC_ADDR, true), \
> -        DEFINE_PROP_BIT("mq", _state, _field, VIRTIO_NET_F_MQ, true)
> -
> +        DEFINE_PROP_BIT("ctrl_mac_addr", _state, _field, VIRTIO_NET_F_CTRL_MAC_ADDR, true)
>  #endif
> --
> MST
>

Patch

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 0af436c..ba09714 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -313,10 +313,6 @@  static QEMUMachine pc_i440fx_machine_v1_4 = {
             .driver   = "virtio-net-pci",\
             .property = "ctrl_mac_addr",\
             .value    = "off",      \
-        },{ \
-            .driver   = "virtio-net-pci", \
-            .property = "mq", \
-            .value    = "off", \
         }
 
 static QEMUMachine pc_machine_v1_3 = {
diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index e37358a..8db1787 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -346,6 +346,10 @@  static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
 
     features |= (1 << VIRTIO_NET_F_MAC);
 
+    if (n->max_queues > 1) {
+        features |= (0x1 << VIRTIO_NET_F_MQ);
+    }
+
     if (!peer_has_vnet_hdr(n)) {
         features &= ~(0x1 << VIRTIO_NET_F_CSUM);
         features &= ~(0x1 << VIRTIO_NET_F_HOST_TSO4);
@@ -1285,7 +1289,9 @@  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
     int i;
 
     n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
-                                        sizeof(struct virtio_net_config),
+                                        conf->queues > 1 ?
+                                            sizeof(struct virtio_net_config) :
+                                            sizeof(struct virtio_net_config_compat),
                                         sizeof(VirtIONet));
 
     n->vdev.get_config = virtio_net_get_config;
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index f5fea6e..b1f7647 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -69,6 +69,17 @@  typedef struct virtio_net_conf
 /* Maximum packet size we can receive from tap device: header + 64k */
 #define VIRTIO_NET_MAX_BUFSIZE (sizeof(struct virtio_net_hdr) + (64 << 10))
 
+/*
+ * Windows drivers from 22 Jan 2013 and older fail when config size != 32.
+ */
+struct virtio_net_config_compat
+{
+    /* The config defining mac address ($ETH_ALEN bytes) */
+    uint8_t mac[ETH_ALEN];
+    /* See VIRTIO_NET_F_STATUS and VIRTIO_NET_S_* above */
+    uint16_t status;
+} QEMU_PACKED;
+
 struct virtio_net_config
 {
     /* The config defining mac address ($ETH_ALEN bytes) */
@@ -190,7 +201,5 @@  struct virtio_net_ctrl_mq {
         DEFINE_PROP_BIT("ctrl_rx", _state, _field, VIRTIO_NET_F_CTRL_RX, true), \
         DEFINE_PROP_BIT("ctrl_vlan", _state, _field, VIRTIO_NET_F_CTRL_VLAN, true), \
         DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true), \
-        DEFINE_PROP_BIT("ctrl_mac_addr", _state, _field, VIRTIO_NET_F_CTRL_MAC_ADDR, true), \
-        DEFINE_PROP_BIT("mq", _state, _field, VIRTIO_NET_F_MQ, true)
-
+        DEFINE_PROP_BIT("ctrl_mac_addr", _state, _field, VIRTIO_NET_F_CTRL_MAC_ADDR, true)
 #endif