diff mbox

Headsup: windows virtio networking does not work on current git

Message ID 20130204064143.GB329@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Feb. 4, 2013, 6:41 a.m. UTC 
On Sun, Feb 03, 2013 at 04:57:38PM -0600, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Sun, Feb 03, 2013 at 03:09:39PM -0600, Anthony Liguori wrote:
> >> Michael Tokarev <mjt@tls.msk.ru> writes:
> >> 
> >> > 03.02.2013 17:23, Yan Vugenfirer wrote:
> >> >
> >> >>> If it helps, mq changes the config size from 8 to 16 bytes.  If the
> >> >>> driver was making an assumption about an 8-byte config size, that's
> >> >>> likely what the problem is.
> >> >>>
> >> >> That's exactly the problem.
> >> >
> >> > So what do we do?  It isn't nice to break existing setups.
> >> > Maybe mq can be disabled by default (together with Antony's
> >> > patch) until fixed drivers will be more widely available?
> >> 
> >> I've got a patch that does exactly like this.  It's pretty ugly though
> >> because of the relationship between virtio-pci and virtio-net.  I'm
> >> going to try to see if I can find a cleaner way to do it on Monday.
> >> 
> >> I'm also contemplating just disabling mq by default.  Since a special
> >> command line is needed to enable it anyway (on the backend), having to
> >> specify mq=on for the device doesn't seem so bad.
> >
> >
> > It's after midnight here so didn't test yet, but wouldn't the following
> > achieve this in a way that is a bit cleaner?
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> >
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index 5699f5e..3342a76 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);
> > +    }
> > +
> 
> This is too late because the config size has already been set.
> 
> >      if (!peer_has_vnet_hdr(n)) {
> >          features &= ~(0x1 << VIRTIO_NET_F_CSUM);
> >          features &= ~(0x1 << VIRTIO_NET_F_HOST_TSO4);
> > @@ -1284,7 +1288,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));
> 
> This makes me a bit nervous.  It relies on setting the config size to
> one thing before we've masked a feature.
> 
> What do you think about just defaulting mq=off?  The more I think about
> it, the nicer of an option that is.
> 
> I'm not a huge fan of automagically changing the value of a feature bit
> so I think defaulting it off is a bit more straight forward.
> 
> We can certainly revisit for 1.5+.
> 
> Regards,
> 
> Anthony Liguori

Thinking about it some more, we have the queues option so
adding an mq option as well is not useful.
Let's clear it unless # of queues is set?
What do you think about the below?
This time tested with an sq guest but not an mq guest yet.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---
diff mbox

Patch

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index 5699f5e..b8c8439 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);
@@ -1284,7 +1288,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 1d5c721..19f63cf 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -69,6 +69,14 @@  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))
 
+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,6 +199,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