[Qemu-devel] [PATCH 0/2] virtio-net: fix config size for older guests

[Qemu-devel] [PATCH 0/2] virtio-net: fix config size for older guests

[Anthony Liguori]

virtio-net multiqueue added a new field to the config layout.  The config
layout is rounded up to the nearest power of 2 and this field caused
field causes the size to go from 8 bytes to 16 bytes.

For reasons unknown, the increase in config size causes the Windows
virtio-net driver to fail.  This is probably a bug in the driver but
it exposes a larger bug in QEMU.

While we disable features for compatibility machines, we always use the
latest config size.  This presents a slightly different machine model
than what we should be exposing.

This series fixes this specific problem such that -M pc-1.3 should work
again with the Windows drivers.  -M pc will still fail.

We need to find a better way to do this but in absence of a nicer approach
this seems likes a good compromise for 1.4.  I think we need to choose
something like the MAX() config size required by all of the enabled host
features.

Besides -M pc-1.3, another work around would be:

  -global virtio-net-pci.mq=off

This should work after this patch series is applied.  We can put this
work around in the release notes as suggested for users of the
virtio-win drivers until they are fixed.

Reported-by: Michael Tokarev <mjt@tls.msk.ru>

FYI: only lightly tested on my end but so far, it seems to work.

[Qemu-devel] [PATCH 1/2] virtio-net: pass host features to virtio_net_init

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/s390x/s390-virtio-bus.c | 3 ++-
 hw/s390x/virtio-ccw.c      | 3 ++-
 hw/virtio-pci.c            | 3 ++-
 hw/virtio.h                | 3 ++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index d467781..089ed92 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -153,7 +153,8 @@ static int s390_virtio_net_init(VirtIOS390Device *dev)
 {
     VirtIODevice *vdev;
 
-    vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net);
+    vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net,
+                           dev->host_features);
     if (!vdev) {
         return -1;
     }
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 231f81e..d92e427 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -555,7 +555,8 @@ static int virtio_ccw_net_init(VirtioCcwDevice *dev)
 {
     VirtIODevice *vdev;
 
-    vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net);
+    vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net,
+                           dev->host_features[0]);
     if (!vdev) {
         return -1;
     }
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 9abbcdf..a869f53 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -997,7 +997,8 @@ static int virtio_net_init_pci(PCIDevice *pci_dev)
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
     VirtIODevice *vdev;
 
-    vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic, &proxy->net);
+    vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic, &proxy->net,
+                           proxy->host_features);
 
     vdev->nvectors = proxy->nvectors;
     virtio_init_pci(proxy, vdev);
diff --git a/hw/virtio.h b/hw/virtio.h
index a29a54d..1e206b8 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -243,7 +243,8 @@ typedef struct VirtIOBlkConf VirtIOBlkConf;
 VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk);
 struct virtio_net_conf;
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
-                              struct virtio_net_conf *net);
+                              struct virtio_net_conf *net,
+                              uint32_t host_features);
 typedef struct virtio_serial_conf virtio_serial_conf;
 VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *serial);
 VirtIODevice *virtio_balloon_init(DeviceState *dev);
-- 
1.8.0

[Qemu-devel] [PATCH 2/2] virtio-net: do not expose a larger config size if multiqueue isn't enabled

[Anthony Liguori]

The windows PV drivers seem to get very upset if the config size changes.
Even when we disable features, we don't change the config size today which
is a bug.

This patch is a pretty rough way to solve this problem and it only handles
the multiqueue case specifically.

Cc: Michael Tokarev <mjt@tls.msk.ru>
Reported-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/virtio-net.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index e37358a..8d15813 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -73,6 +73,7 @@ typedef struct VirtIONet
     int multiqueue;
     uint16_t max_queues;
     uint16_t curr_queues;
+    int config_size;
 } VirtIONet;
 
 static VirtIONetQueue *virtio_net_get_subqueue(NetClientState *nc)
@@ -99,20 +100,20 @@ static VirtIONet *to_virtio_net(VirtIODevice *vdev)
 static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
 {
     VirtIONet *n = to_virtio_net(vdev);
-    struct virtio_net_config netcfg;
+    struct virtio_net_config netcfg = {};
 
     stw_p(&netcfg.status, n->status);
     stw_p(&netcfg.max_virtqueue_pairs, n->max_queues);
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
-    memcpy(config, &netcfg, sizeof(netcfg));
+    memcpy(config, &netcfg, n->config_size);
 }
 
 static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
 {
     VirtIONet *n = to_virtio_net(vdev);
-    struct virtio_net_config netcfg;
+    struct virtio_net_config netcfg = {};
 
-    memcpy(&netcfg, config, sizeof(netcfg));
+    memcpy(&netcfg, config, n->config_size);
 
     if (!(n->vdev.guest_features >> VIRTIO_NET_F_CTRL_MAC_ADDR & 1) &&
         memcmp(netcfg.mac, n->mac, ETH_ALEN)) {
@@ -1279,14 +1280,21 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
 }
 
 VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
-                              virtio_net_conf *net)
+                              virtio_net_conf *net, uint32_t host_features)
 {
     VirtIONet *n;
     int i;
+    int config_size;
 
+    if (host_features & VIRTIO_NET_F_MQ) {
+        config_size = sizeof(struct virtio_net_config);
+    } else {
+        config_size = 8;
+    }
     n = (VirtIONet *)virtio_common_init("virtio-net", VIRTIO_ID_NET,
-                                        sizeof(struct virtio_net_config),
+                                        config_size,
                                         sizeof(VirtIONet));
+    n->config_size = config_size;
 
     n->vdev.get_config = virtio_net_get_config;
     n->vdev.set_config = virtio_net_set_config;
-- 
1.8.0

Re: [Qemu-devel] [PATCH 1/2] virtio-net: pass host features to virtio_net_init

[Andreas Färber]

Am 03.02.2013 00:06, schrieb Anthony Liguori:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/s390x/s390-virtio-bus.c | 3 ++-
>  hw/s390x/virtio-ccw.c      | 3 ++-
>  hw/virtio-pci.c            | 3 ++-
>  hw/virtio.h                | 3 ++-
>  4 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index d467781..089ed92 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -153,7 +153,8 @@ static int s390_virtio_net_init(VirtIOS390Device *dev)
>  {
>      VirtIODevice *vdev;
>  
> -    vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net);
> +    vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net,
> +                           dev->host_features);
>      if (!vdev) {
>          return -1;
>      }
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 231f81e..d92e427 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -555,7 +555,8 @@ static int virtio_ccw_net_init(VirtioCcwDevice *dev)
>  {
>      VirtIODevice *vdev;
>  
> -    vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net);
> +    vdev = virtio_net_init((DeviceState *)dev, &dev->nic, &dev->net,
> +                           dev->host_features[0]);
>      if (!vdev) {
>          return -1;
>      }
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 9abbcdf..a869f53 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -997,7 +997,8 @@ static int virtio_net_init_pci(PCIDevice *pci_dev)
>      VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
>      VirtIODevice *vdev;
>  
> -    vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic, &proxy->net);
> +    vdev = virtio_net_init(&pci_dev->qdev, &proxy->nic, &proxy->net,
> +                           proxy->host_features);
>  
>      vdev->nvectors = proxy->nvectors;
>      virtio_init_pci(proxy, vdev);
> diff --git a/hw/virtio.h b/hw/virtio.h
> index a29a54d..1e206b8 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -243,7 +243,8 @@ typedef struct VirtIOBlkConf VirtIOBlkConf;
>  VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk);
>  struct virtio_net_conf;
>  VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf,
> -                              struct virtio_net_conf *net);
> +                              struct virtio_net_conf *net,
> +                              uint32_t host_features);
>  typedef struct virtio_serial_conf virtio_serial_conf;
>  VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *serial);
>  VirtIODevice *virtio_balloon_init(DeviceState *dev);

The virtio_net_init() implementation went into 2/2 so this breaks.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg