Re: [Qemu-devel] [PATCH] virtio-net: unbreak the minix guest

[Jason Wang]

On 04/25/2013 02:57 PM, Michael S. Tsirkin wrote:
> On Thu, Apr 25, 2013 at 02:19:53PM +0800, Jason Wang wrote:
>> Multiqueue patchset conditionally add control vq only when guest negotiate 
>> the
>> feature. Though the spec is not clear on this but it breaks the minix guest
>> since it will identify the ctrl vq even if it does not support it. Though 
>> this
>> behavior seems a violation on the spec "If the VIRTIO_NET_F_CTRL_VQ feature 
>> bit
>> is negotiated, identify the control virtqueue.", to keep the backward
>> compatibility, always add the ctrl vq at end of the queues.
>>
>> Also remove the meaningless ctrl_vq initialization and vq deletion.
>>
>> Reported-by: Aurelien Jarno <address@hidden>
>> Cc: Aurelien Jarno <address@hidden>
>> Signed-off-by: Jason Wang <address@hidden>
>> ---
>>  hw/net/virtio-net.c |    9 ++++-----
>>  1 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 4d2cdd2..1068f4e 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -1045,7 +1045,7 @@ static void virtio_net_set_multiqueue(VirtIONet *n, 
>> int multiqueue, int ctrl)
>>  
>>      n->multiqueue = multiqueue;
>>  
>> -    for (i = 2; i <= n->max_queues * 2 + 1; i++) {
>> +    for (i = 2; i <= n->max_queues * 2; i++) {
>>          virtio_del_queue(vdev, i);
>>      }
>>  
> Unrelated cleanup? Separate patch pls.

Ok.
>
>> @@ -1067,9 +1067,9 @@ static void virtio_net_set_multiqueue(VirtIONet *n, 
>> int multiqueue, int ctrl)
>>          n->vqs[i].n = n;
>>      }
>>  
>> -    if (ctrl) {
>> -        n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
>> -    }
>> +    /* Some guest (e.g minix) may identifiy ctrl vq even if it does not
>> +     * support. */
> /*
>  * Note: Minux Guests (version XYZ) use ctrl vq but don't ack
>  * VIRTIO_NET_F_CTRL_VQ.  Create ctrl vq unconditionally to avoid
>  * breaking them.
>  */

Sure.
>> +    n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
>>  
>>      virtio_net_set_queues(n);
>>  }
>> @@ -1317,7 +1317,6 @@ static int virtio_net_device_init(VirtIODevice *vdev)
>>                                             virtio_net_handle_tx_bh);
>>          n->vqs[0].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[0]);
>>      }
>> -    n->ctrl_vq = virtio_add_queue(vdev, 64, virtio_net_handle_ctrl);
> Why drop this here? Unrelated cleanup? Separate patch pls.

Ok, will send v2. Thanks
>>      qemu_macaddr_default_if_unset(&n->nic_conf.macaddr);
>>      memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac));
>>      n->status = VIRTIO_NET_S_LINK_UP;
>> -- 
>> 1.7.1