diff mbox

virtio-net with virtio-mmio

Message ID CAA1vHqKz3xYNF_ZiS508xoH21gLadB3TKqhLxP7nWPcq8Xx0LA@mail.gmail.com
State New
Headers show

Commit Message

Ying-Shiuan Pan Jan. 2, 2012, 5:19 a.m. UTC
Hi, Stefan

Thanks for your response.


2011/12/30 Stefan Hajnoczi <stefanha@gmail.com>
>
> On Wed, Dec 28, 2011 at 06:16:42PM +0800, Ying-Shiuan Pan wrote:
> > I'm very interested in virtio-mmio Peter Maydell did for QEMU,
> > (http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01870.html)
> >
> > actually, I've tested the virtio-blk, and it is working.
> > I applied those patch to QEMU-1.0 and brought the virtio_mmio.c from
> > Linux-3.2-rc back to Linux-linaro-2.6.38.
> >
> > I also found a small bug in virito-mmio.c: virtio_mmio_write(),
> > Peter forgot to break in each case of switch block.
> > After fixed the small bug, the virtio-balloon works as well.
>
> PMM: Do you have a git branch where you could apply Ying-Shiuan's fix?
hmm... I've sent Peter my patch :)
>
> > Then, when I attempted to enable the virtio-net, the initialization part is
> > fine,
> > however, the QEMU crashed and printed this message:
> > "virtio-net header not in first element"
> >
> > It happens when the front-end virtio-net is invoking virtqueue_kick() at
> > the end of try_fill_recv().
> > Then, QEMU gets this message and invokes virtio_net_receive(), then the
> > error happens.
>
> virtio-net is looking at a virtqueue element (a packet buffer provided
> by the guest OS into which QEMU will copy the received packet) but the
> virtqueue element does not have the expected format.  You can check the
> virtio specification for the details on the virtio-net buffer layout:
> http://ozlabs.org/~rusty/virtio-spec/
>
> It sounds like the vring is corrupted or QEMU's virtio code is not
> correctly reading the virtqueue element (which is basically an iovec
> array).
>
> virtio-net is more advanced than virtio-blk in how it uses virtio so
> it's not surprising that you've discovered an issue.  Debugging this
> comes down to looking at the vring descriptors and the virtqueue
> elements that QEMU extracts.
>
> It sounds like there's a problem with the rx queue, you could try
> indirect_desc=off to disable indirect vring descriptors to see if that
> is part of the problem.
>
> Stefan
I guessed that the problem was in adding header,
virtio-net should insert its header to the ring buffer before its
packet, correct me if I'm wrong.

virtio-net driver (backend part) can use add_recvbuf_small() /
add_recvbuf_big() / add_recvbuf_mergeable()
depending on what features the virtio-net device (QEMU part) provides.
I found that both small and big add the header, while the mergeable
one does not.
Besides, in that add_recvbuf_big(), the comments mentioned about a QEMU bug??

Anyway, I found a workaround solution, that is disable the
VIRTIO_NET_F_MRG_RXBUF feature
and make virtio-net driver use add_recvbuf_big()

However, I am still not clear about how the problem happened.
If I luckily figure out, I'll post it. :)

So~ currently, virito-balloon, virtio-blk, and virtio-net can work
properly  with virtio-mmio. :D

The following is the *workaround solution* which is for virtio-net
driver part (frontend).



----
Best Regards,
Ying-Shiuan Pan

Comments

Stefan Hajnoczi Jan. 2, 2012, 5:19 p.m. UTC | #1
On Mon, Jan 2, 2012 at 5:19 AM, Ying-Shiuan Pan
<yingshiuan.pan@gmail.com> wrote:
> 2011/12/30 Stefan Hajnoczi <stefanha@gmail.com>
>>
>> On Wed, Dec 28, 2011 at 06:16:42PM +0800, Ying-Shiuan Pan wrote:
>> > I'm very interested in virtio-mmio Peter Maydell did for QEMU,
>> > (http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01870.html)
>> >
>> > actually, I've tested the virtio-blk, and it is working.
>> > I applied those patch to QEMU-1.0 and brought the virtio_mmio.c from
>> > Linux-3.2-rc back to Linux-linaro-2.6.38.
>> >
>> > I also found a small bug in virito-mmio.c: virtio_mmio_write(),
>> > Peter forgot to break in each case of switch block.
>> > After fixed the small bug, the virtio-balloon works as well.
>>
>> PMM: Do you have a git branch where you could apply Ying-Shiuan's fix?
> hmm... I've sent Peter my patch :)
>>
>> > Then, when I attempted to enable the virtio-net, the initialization part is
>> > fine,
>> > however, the QEMU crashed and printed this message:
>> > "virtio-net header not in first element"
>> >
>> > It happens when the front-end virtio-net is invoking virtqueue_kick() at
>> > the end of try_fill_recv().
>> > Then, QEMU gets this message and invokes virtio_net_receive(), then the
>> > error happens.
>>
>> virtio-net is looking at a virtqueue element (a packet buffer provided
>> by the guest OS into which QEMU will copy the received packet) but the
>> virtqueue element does not have the expected format.  You can check the
>> virtio specification for the details on the virtio-net buffer layout:
>> http://ozlabs.org/~rusty/virtio-spec/
>>
>> It sounds like the vring is corrupted or QEMU's virtio code is not
>> correctly reading the virtqueue element (which is basically an iovec
>> array).
>>
>> virtio-net is more advanced than virtio-blk in how it uses virtio so
>> it's not surprising that you've discovered an issue.  Debugging this
>> comes down to looking at the vring descriptors and the virtqueue
>> elements that QEMU extracts.
>>
>> It sounds like there's a problem with the rx queue, you could try
>> indirect_desc=off to disable indirect vring descriptors to see if that
>> is part of the problem.
>>
>> Stefan
> I guessed that the problem was in adding header,
> virtio-net should insert its header to the ring buffer before its
> packet, correct me if I'm wrong.
>
> virtio-net driver (backend part) can use add_recvbuf_small() /
> add_recvbuf_big() / add_recvbuf_mergeable()
> depending on what features the virtio-net device (QEMU part) provides.
> I found that both small and big add the header, while the mergeable
> one does not.
> Besides, in that add_recvbuf_big(), the comments mentioned about a QEMU bug??
>
> Anyway, I found a workaround solution, that is disable the
> VIRTIO_NET_F_MRG_RXBUF feature
> and make virtio-net driver use add_recvbuf_big()
>
> However, I am still not clear about how the problem happened.

Can you confirm which "virtio-net header not in first element" error
is occurring?  There are two instances in qemu/hw/virtio-net.c:
virtio_net_receive() and virtio_net_flush_tx().

If you are really seeing the error from virtio_net_receive() then
feature negotiation is broken.  The guest will advertise mergeable
receive buffers and the host (QEMU) should detect this in
virtio_net_set_features().  This causes QEMU to set
VirtIONet->mergeable_rx_bufs to true.

If VirtIONet->mergeable_rx_bufs is true then the error you are seeing
in virtio_net_receive() cannot happen:

    if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != guest_hdr_len) {
        error_report("virtio-net header not in first element");
        exit(1);
    }

The only explanation is that the guest and host did not negotiate the
mergeable receive buffers feature properly.  That could mean Peter's
QEMU patches are not calling invoking virtio_net_set_features() at the
appropriate time or something else is broken - probably a virtio-mmio
issue.

AFAIK mergeable receive buffers are used by default and work correctly
under virtio-pci.

If you can debug this a little more we'll figure out what the problem is.

Stefan
Ying-Shiuan Pan Jan. 3, 2012, 6:37 a.m. UTC | #2
2012/1/3 Stefan Hajnoczi <stefanha@gmail.com>:
> On Mon, Jan 2, 2012 at 5:19 AM, Ying-Shiuan Pan
> <yingshiuan.pan@gmail.com> wrote:
>> 2011/12/30 Stefan Hajnoczi <stefanha@gmail.com>
>>>
>>> On Wed, Dec 28, 2011 at 06:16:42PM +0800, Ying-Shiuan Pan wrote:
>>> > I'm very interested in virtio-mmio Peter Maydell did for QEMU,
>>> > (http://lists.nongnu.org/archive/html/qemu-devel/2011-11/msg01870.html)
>>> >
>>> > actually, I've tested the virtio-blk, and it is working.
>>> > I applied those patch to QEMU-1.0 and brought the virtio_mmio.c from
>>> > Linux-3.2-rc back to Linux-linaro-2.6.38.
>>> >
>>> > I also found a small bug in virito-mmio.c: virtio_mmio_write(),
>>> > Peter forgot to break in each case of switch block.
>>> > After fixed the small bug, the virtio-balloon works as well.
>>>
>>> PMM: Do you have a git branch where you could apply Ying-Shiuan's fix?
>> hmm... I've sent Peter my patch :)
>>>
>>> > Then, when I attempted to enable the virtio-net, the initialization part is
>>> > fine,
>>> > however, the QEMU crashed and printed this message:
>>> > "virtio-net header not in first element"
>>> >
>>> > It happens when the front-end virtio-net is invoking virtqueue_kick() at
>>> > the end of try_fill_recv().
>>> > Then, QEMU gets this message and invokes virtio_net_receive(), then the
>>> > error happens.
>>>
>>> virtio-net is looking at a virtqueue element (a packet buffer provided
>>> by the guest OS into which QEMU will copy the received packet) but the
>>> virtqueue element does not have the expected format.  You can check the
>>> virtio specification for the details on the virtio-net buffer layout:
>>> http://ozlabs.org/~rusty/virtio-spec/
>>>
>>> It sounds like the vring is corrupted or QEMU's virtio code is not
>>> correctly reading the virtqueue element (which is basically an iovec
>>> array).
>>>
>>> virtio-net is more advanced than virtio-blk in how it uses virtio so
>>> it's not surprising that you've discovered an issue.  Debugging this
>>> comes down to looking at the vring descriptors and the virtqueue
>>> elements that QEMU extracts.
>>>
>>> It sounds like there's a problem with the rx queue, you could try
>>> indirect_desc=off to disable indirect vring descriptors to see if that
>>> is part of the problem.
>>>
>>> Stefan
>> I guessed that the problem was in adding header,
>> virtio-net should insert its header to the ring buffer before its
>> packet, correct me if I'm wrong.
>>
>> virtio-net driver (backend part) can use add_recvbuf_small() /
>> add_recvbuf_big() / add_recvbuf_mergeable()
>> depending on what features the virtio-net device (QEMU part) provides.
>> I found that both small and big add the header, while the mergeable
>> one does not.
>> Besides, in that add_recvbuf_big(), the comments mentioned about a QEMU bug??
>>
>> Anyway, I found a workaround solution, that is disable the
>> VIRTIO_NET_F_MRG_RXBUF feature
>> and make virtio-net driver use add_recvbuf_big()
>>
>> However, I am still not clear about how the problem happened.
>
> Can you confirm which "virtio-net header not in first element" error
> is occurring?  There are two instances in qemu/hw/virtio-net.c:
> virtio_net_receive() and virtio_net_flush_tx().

Yes, in virtio_net_receive().
In the probing phase, the virtnet_probe() is invoked. Almost at the
end of virnet_probe(),
try_fill_recv() is invoked, and after try_fill_recv() fill the RX
virtqueue, it will kick the virtqueue,
Then, virtio_net_receive() in QEMU is triggered.

If I modify the virtio_net_receive() to make it ignore this error,
the the problem also occurs in virtio_net_flush_tx().

>
> If you are really seeing the error from virtio_net_receive() then
> feature negotiation is broken.  The guest will advertise mergeable
> receive buffers and the host (QEMU) should detect this in
> virtio_net_set_features().  This causes QEMU to set
> VirtIONet->mergeable_rx_bufs to true.
>
> If VirtIONet->mergeable_rx_bufs is true then the error you are seeing
> in virtio_net_receive() cannot happen:
>
>    if (!n->mergeable_rx_bufs && elem.in_sg[0].iov_len != guest_hdr_len) {
>        error_report("virtio-net header not in first element");
>        exit(1);
>    }
>
> The only explanation is that the guest and host did not negotiate the
> mergeable receive buffers feature properly.  That could mean Peter's
> QEMU patches are not calling invoking virtio_net_set_features() at the
> appropriate time or something else is broken - probably a virtio-mmio
> issue.

Oh!!!!!!!  Great!!!!!!
That's the key point!!!!!

Because virtio_net register its own set_features() method, and some
features are setting in that function.
While virtio_blk and virtio_balloon does not use set_features(),
that's why they did not encounter such problem.

>
> AFAIK mergeable receive buffers are used by default and work correctly
> under virtio-pci.
>
> If you can debug this a little more we'll figure out what the problem is.
>
> Stefan
I will submit patches later, thanks a lot!!

----
Best Regards,
潘穎軒Ying-Shiuan Pan
diff mbox

Patch

diff --git a/virtio_net.c b/virtio_net.c
index 82dba5a..811e2d9 100644
--- a/virtio_net.c
+++ b/virtio_net.c
@@ -955,8 +955,10 @@  static int virtnet_probe(struct virtio_device *vdev)
            virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN))
                vi->big_packets = true;

+#ifndef ARM
        if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
                vi->mergeable_rx_bufs = true;
+#endif

        /* We expect two virtqueues, receive then send,
         * and optionally control. */