Re: [Qemu-devel] Headsup: windows virtio networking does not work on current git

[Anthony Liguori]

"Michael S. Tsirkin" <address@hidden> writes:

> On Sun, Feb 03, 2013 at 03:09:39PM -0600, Anthony Liguori wrote:
>> Michael Tokarev <address@hidden> 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 <address@hidden>
>
>
> 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

>  
>      n->vdev.get_config = virtio_net_get_config;
> diff --git a/hw/virtio-net.h b/hw/virtio-net.h
> index 1d5c721..95ad54f 100644
> --- a/hw/virtio-net.h
> +++ b/hw/virtio-net.h
> @@ -69,6 +69,15 @@ 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) */