Hi Ilya, Generically speaking, this patch looks good to me. But I guess still need more time to check this issue later; I still failed to reproduce it on my side after all. So, please allow a late merge.
Thanks. --yliu On Mon, May 30, 2016 at 02:05:07PM +0300, Ilya Maximets wrote: > Ping. > > Best regards, Ilya Maximets. > > On 23.05.2016 14:04, Ilya Maximets wrote: > > On 23.05.2016 13:57, Yuanhan Liu wrote: > >> On Fri, May 20, 2016 at 03:50:04PM +0300, Ilya Maximets wrote: > >>> In current implementation guest application can reinitialize vrings > >>> by executing start after stop. In the same time host application > >>> can still poll virtqueue while device stopped in guest and it will > >>> crash with segmentation fault while vring reinitialization because > >>> of dereferencing of bad descriptor addresses. > >>> > >>> OVS crash for example: > >>> <------------------------------------------------------------------------> > >>> [test-pmd inside guest VM] > >>> > >>> testpmd> port stop all > >>> Stopping ports... > >>> Checking link statuses... > >>> Port 0 Link Up - speed 10000 Mbps - full-duplex > >>> Done > >>> testpmd> port config all rxq 2 > >>> testpmd> port config all txq 2 > >>> testpmd> port start all > >>> Configuring Port 0 (socket 0) > >>> Port 0: 52:54:00:CB:44:C8 > >>> Checking link statuses... > >>> Port 0 Link Up - speed 10000 Mbps - full-duplex > >>> Done > >> > >> I actually didn't manage to reproduce it on my side, with the > >> vhost-example instead of OVS though. Is that all the commands > >> to reproduce it, and run them just after start test-pmd? > > > > Actually, I think, packet flow should be enabled while performing > > above actions and some traffic already should be sent through port > > to change last used idx on vhost side. > > > > Something like: > > start > > ..wait a while.. see that packets are flowing. > > stop > > port stop > > port config > > port config > > port start > >> > >>> [OVS on host] > >>> Program received signal SIGSEGV, Segmentation fault. > >>> rte_memcpy (n=2056, src=0xc, dst=0x7ff4d5247000) at rte_memcpy.h > >>> > >>> (gdb) bt > >>> #0 rte_memcpy (n=2056, src=0xc, dst=0x7ff4d5247000) > >>> #1 copy_desc_to_mbuf > >>> #2 rte_vhost_dequeue_burst > >>> #3 netdev_dpdk_vhost_rxq_recv > >>> ... > >>> > >>> (gdb) bt full > >>> #0 rte_memcpy > >>> ... > >>> #1 copy_desc_to_mbuf > >>> desc_addr = 0 > >>> mbuf_offset = 0 > >>> desc_offset = 12 > >>> ... > >>> <------------------------------------------------------------------------> > >>> > >>> Fix that by checking addresses of descriptors before using them. > >>> > >>> Note: For mergeable buffers this patch checks only guest's address for > >>> zero, but in non-meargeable case host's address checked. This is done > >>> because checking of host's address in mergeable case requires additional > >>> refactoring to keep virtqueue in consistent state in case of error. > >>> > >>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com> > >>> --- > >>> > >>> Actually, current virtio implementation looks broken for me. Because > >>> 'virtio_dev_start' breaks virtqueue while it still available from the > >>> vhost > >>> side. > >>> > >>> There was 2 patches about this behaviour: > >>> > >>> 1. a85786dc816f ("virtio: fix states handling during initialization") > >>> 2. 9a0615af7746 ("virtio: fix restart") > >>> > >>> The second patch fixes somehow issue intoduced in the first patch, but > >>> actually > >>> also breaks vhost in the way described above. > >>> It's not pretty clear for me what to do in current situation with virtio, > >>> because it will be broken for guest application even if vhost will not > >>> crash. > >>> > >>> May be it'll be better to forbid stopping of virtio device and force user > >>> to > >>> exit and start again (may be implemented in hidden from user way)? > >>> > >>> This patch adds additional sane checks, so it should be applied anyway, > >>> IMHO. > >> > >> Agreed. > >> > >> --yliu > >> > >>