| Message ID | 20130204074147.GA7753@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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>
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 >
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
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(-)