From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54001) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtrDL-0004ot-PE for qemu-devel@nongnu.org; Thu, 27 Nov 2014 00:02:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtrDF-0001ao-J0 for qemu-devel@nongnu.org; Thu, 27 Nov 2014 00:01:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34301) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtrDF-0001ak-CA for qemu-devel@nongnu.org; Thu, 27 Nov 2014 00:01:53 -0500 Message-ID: <5476B029.4070009@redhat.com> Date: Thu, 27 Nov 2014 13:01:29 +0800 From: Jason Wang MIME-Version: 1.0 References: <547290D7.2020506@cn.fujitsu.com> <5472F1DA.4080508@m2r.biz> <5472F980.6030208@cn.fujitsu.com> <54741ED7.2060500@redhat.com> <547563E6.2090505@redhat.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Xen-devel] virtio leaks cpu mappings, was: qemu crash with virtio on Xen domUs (backtrace included) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: mst@redhat.com, qemu-devel@nongnu.org, xen devel , Fabio Fantoni , aliguori@amazon.com, anthony PERARD , Paolo Bonzini On 11/26/2014 06:53 PM, Stefano Stabellini wrote: > On Wed, 26 Nov 2014, Jason Wang wrote: >> >On 11/25/2014 09:53 PM, Stefano Stabellini wrote: >>> > >On Tue, 25 Nov 2014, Jason Wang wrote: >>>> > >>On 11/25/2014 02:44 AM, Stefano Stabellini wrote: >>>>> > >>>On Mon, 24 Nov 2014, Stefano Stabellini wrote: >>>>>> > >>>>On Mon, 24 Nov 2014, Stefano Stabellini wrote: >>>>>>> > >>>>>CC'ing Paolo. >>>>>>> > >>>>> >>>>>>> > >>>>> >>>>>>> > >>>>>Wen, >>>>>>> > >>>>>thanks for the logs. >>>>>>> > >>>>> >>>>>>> > >>>>>I investigated a little bit and it seems to me that the bug occurs when >>>>>>> > >>>>>QEMU tries to unmap only a portion of a memory region previously mapped. >>>>>>> > >>>>>That doesn't work with xen-mapcache. >>>>>>> > >>>>> >>>>>>> > >>>>>See these logs for example: >>>>>>> > >>>>> >>>>>>> > >>>>>DEBUG address_space_map phys_addr=78ed8b44 vaddr=7fab50afbb68 len=0xa >>>>>>> > >>>>>DEBUG address_space_unmap vaddr=7fab50afbb68 len=0x6 >>>>>> > >>>>Sorry the logs don't quite match, it was supposed to be: >>>>>> > >>>> >>>>>> > >>>>DEBUG address_space_map phys_addr=78ed8b44 vaddr=7fab50afbb64 len=0xa >>>>>> > >>>>DEBUG address_space_unmap vaddr=7fab50afbb68 len=0x6 >>>>> > >>>It looks like the problem is caused by iov_discard_front, called by >>>>> > >>>virtio_net_handle_ctrl. By changing iov_base after the sg has already >>>>> > >>>been mapped (cpu_physical_memory_map), it causes a leak in the mapping >>>>> > >>>because the corresponding cpu_physical_memory_unmap will only unmap a >>>>> > >>>portion of the original sg. On Xen the problem is worse because >>>>> > >>>xen-mapcache aborts. >>>>> > >>> >>>>> > >>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>>> > >>>index 2ac6ce5..b2b5c2d 100644 >>>>> > >>>--- a/hw/net/virtio-net.c >>>>> > >>>+++ b/hw/net/virtio-net.c >>>>> > >>>@@ -775,7 +775,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) >>>>> > >>> struct iovec *iov; >>>>> > >>> unsigned int iov_cnt; >>>>> > >>> >>>>> > >>>- while (virtqueue_pop(vq, &elem)) { >>>>> > >>>+ while (virtqueue_pop_nomap(vq, &elem)) { >>>>> > >>> if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) || >>>>> > >>> iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) { >>>>> > >>> error_report("virtio-net ctrl missing headers"); >>>>> > >>>@@ -784,8 +784,12 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq) >>>>> > >>> >>>>> > >>> iov = elem.out_sg; >>>>> > >>> iov_cnt = elem.out_num; >>>>> > >>>- s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl)); >>>>> > >>> iov_discard_front(&iov, &iov_cnt, sizeof(ctrl)); >>>>> > >>>+ >>>>> > >>>+ virtqueue_map_sg(elem.in_sg, elem.in_addr, elem.in_num, 1); >>>>> > >>>+ virtqueue_map_sg(elem.out_sg, elem.out_addr, elem.out_num, 0); >>>>> > >>>+ >>>>> > >>>+ s = iov_to_buf(iov, iov_cnt, 0, &ctrl, sizeof(ctrl)); >>>> > >>Does this really work? >>> > >It seems to work here, as in it doesn't crash QEMU and I am able to boot >>> > >a guest with network. I didn't try any MAC related commands. >>> > > >> > >> >It was because the guest (not a recent kernel?) never issue commands >> >through control vq. >> > >> >We'd better hide the implementation details such as virtqueue_map_sg() >> >in virtio core instead of letting device call it directly. >>>> > >>The code in fact skips the location that contains >>>> > >>virtio_net_ctrl_hdr. And virtio_net_handle_mac() still calls >>>> > >>iov_discard_front(). >>>> > >> >>>> > >>How about copy iov to a temp variable and use it in this function? >>> > >That would only work if I moved the cpu_physical_memory_unmap call >>> > >outside of virtqueue_fill, so that we can pass different iov to them. >>> > >We need to unmap the same iov that was previously mapped by >>> > >virtqueue_pop. >>> > > >> > >> >I mean something like following or just passing the offset of iov to >> >virtio_net_handle_*(). > Sorry, you are right, your patch works too. I tried something like this > yesterday but I was confused because even if a crash doesn't happen > anymore, virtio-net still doesn't work on Xen (it boots but the network > doesn't work properly within the guest). > But that seems to be a separate issue and it affects my series too. > > A possible problem with this approach is that virtqueue_push is now > called passing the original iov, not the shortened one. > > Are you sure that is OK? It's ok, except for unmapping, virtqueue_push does not care iov at all. > If so we can drop my series and use this instead. > I will submit a formal patch for this. Thanks