Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-16 Thread Anthony Liguori
Paolo Bonzini writes: >> > We technically should save the addresses and sizes too. It makes >> > it a heck of a lot safer then re-reading guest memory since we do some >> > validation on the size of the sg elements. >> >> Not really. >> >> The guest puts the descriptors in the ring and leaves

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-16 Thread Michael S. Tsirkin
On Thu, Dec 13, 2012 at 11:48:17AM +0100, Kevin Wolf wrote: > Am 12.12.2012 17:37, schrieb Michael S. Tsirkin: > > On Wed, Dec 12, 2012 at 04:52:53PM +0100, Paolo Bonzini wrote: > >> Il 12/12/2012 16:25, Michael S. Tsirkin ha scritto: > >>> On Wed, Dec 12, 2012 at 04:01:27PM +0100, Paolo Bonzini wr

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-13 Thread Paolo Bonzini
> > We technically should save the addresses and sizes too. It makes > > it a heck of a lot safer then re-reading guest memory since we do some > > validation on the size of the sg elements. > > Not really. > > The guest puts the descriptors in the ring and leaves them there until > the device

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-13 Thread Rusty Russell
Anthony Liguori writes: > Yes, take a look at the series I sent out that scrubs all of this to > just send the index and the addresses of the element. > > We technically should save the addresses and sizes too. It makes it a > heck of a lot safer then re-reading guest memory since we do some > va

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-13 Thread Kevin Wolf
Am 12.12.2012 17:37, schrieb Michael S. Tsirkin: > On Wed, Dec 12, 2012 at 04:52:53PM +0100, Paolo Bonzini wrote: >> Il 12/12/2012 16:25, Michael S. Tsirkin ha scritto: >>> On Wed, Dec 12, 2012 at 04:01:27PM +0100, Paolo Bonzini wrote: Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto: >

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Paolo Bonzini
Il 12/12/2012 22:19, Michael S. Tsirkin ha scritto: > On Wed, Dec 12, 2012 at 10:00:37PM +0100, Paolo Bonzini wrote: >> Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto: > Saving inuse counter is useless. We need to know which requests > are outstanding if we want to retry them on remote.

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
On Wed, Dec 12, 2012 at 03:33:32PM -0600, Anthony Liguori wrote: > "Michael S. Tsirkin" writes: > > > On Wed, Dec 12, 2012 at 10:00:37PM +0100, Paolo Bonzini wrote: > >> Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto: > >> >>> Saving inuse counter is useless. We need to know which requests >

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Anthony Liguori
"Michael S. Tsirkin" writes: > On Wed, Dec 12, 2012 at 10:00:37PM +0100, Paolo Bonzini wrote: >> Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto: >> >>> Saving inuse counter is useless. We need to know which requests >> >>> are outstanding if we want to retry them on remote. >> >> >> >> And th

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
On Wed, Dec 12, 2012 at 10:00:37PM +0100, Paolo Bonzini wrote: > Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto: > >>> Saving inuse counter is useless. We need to know which requests > >>> are outstanding if we want to retry them on remote. > >> > >> And that's what virtio-blk and virtio-scsi h

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Paolo Bonzini
Il 12/12/2012 20:23, Michael S. Tsirkin ha scritto: >>> Saving inuse counter is useless. We need to know which requests >>> are outstanding if we want to retry them on remote. >> >> And that's what virtio-blk and virtio-scsi have been doing for years. > > I don't see it - all I see in save is virt

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
On Wed, Dec 12, 2012 at 06:39:15PM +0100, Paolo Bonzini wrote: > Il 12/12/2012 18:14, Michael S. Tsirkin ha scritto: > > On Wed, Dec 12, 2012 at 05:51:51PM +0100, Paolo Bonzini wrote: > >> Il 12/12/2012 17:37, Michael S. Tsirkin ha scritto: > You wrote "the only way to know head 1 is outstandi

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Paolo Bonzini
Il 12/12/2012 18:14, Michael S. Tsirkin ha scritto: > On Wed, Dec 12, 2012 at 05:51:51PM +0100, Paolo Bonzini wrote: >> Il 12/12/2012 17:37, Michael S. Tsirkin ha scritto: You wrote "the only way to know head 1 is outstanding is because backend has stored this info somewhere". But the ba

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
On Wed, Dec 12, 2012 at 05:51:51PM +0100, Paolo Bonzini wrote: > Il 12/12/2012 17:37, Michael S. Tsirkin ha scritto: > > > You wrote "the only way to know head 1 is outstanding is because backend > > > has stored this info somewhere". But the backend _is_ tracking it (by > > > serializing and then

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Paolo Bonzini
Il 12/12/2012 17:37, Michael S. Tsirkin ha scritto: > > You wrote "the only way to know head 1 is outstanding is because backend > > has stored this info somewhere". But the backend _is_ tracking it (by > > serializing and then restoring the VirtQueueElement) and no leak happens > > because virtqu

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
On Wed, Dec 12, 2012 at 04:52:53PM +0100, Paolo Bonzini wrote: > Il 12/12/2012 16:25, Michael S. Tsirkin ha scritto: > > On Wed, Dec 12, 2012 at 04:01:27PM +0100, Paolo Bonzini wrote: > >> Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto: > Ok, so we need some API for virtio-{blk,scsi} to co

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Paolo Bonzini
Il 12/12/2012 16:25, Michael S. Tsirkin ha scritto: > On Wed, Dec 12, 2012 at 04:01:27PM +0100, Paolo Bonzini wrote: >> Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto: Ok, so we need some API for virtio-{blk,scsi} to communicate back the indexes of in-flight requests to virtio. The i

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
On Wed, Dec 12, 2012 at 04:01:27PM +0100, Paolo Bonzini wrote: > Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto: > > > Ok, so we need some API for virtio-{blk,scsi} to communicate back the > > > indexes of in-flight requests to virtio. The indexes are known from the > > > VirtQueueElement, so

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Paolo Bonzini
Il 12/12/2012 15:47, Michael S. Tsirkin ha scritto: > > Ok, so we need some API for virtio-{blk,scsi} to communicate back the > > indexes of in-flight requests to virtio. The indexes are known from the > > VirtQueueElement, so that's fine. > > > > Even better would be a virtio_save_request/virtio

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
On Wed, Dec 12, 2012 at 03:36:10PM +0100, Paolo Bonzini wrote: > Il 12/12/2012 15:30, Michael S. Tsirkin ha scritto: > > > Same for virtio-scsi. Each request in that case is sent as part of the > > > SCSIDevice that it refers to, via callbacks in SCSIBusInfo. > > It is in virtio_scsi_load_request

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Paolo Bonzini
Il 12/12/2012 15:30, Michael S. Tsirkin ha scritto: > > Same for virtio-scsi. Each request in that case is sent as part of the > > SCSIDevice that it refers to, via callbacks in SCSIBusInfo. It is in virtio_scsi_load_request. > Looks like this will leak ring entries. > > All I see is: virtio_sc

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
On Wed, Dec 12, 2012 at 03:01:55PM +0100, Paolo Bonzini wrote: > Il 12/12/2012 14:50, Stefan Hajnoczi ha scritto: > > VirtIOBlock->rq can trigger the assertion. > > > > IIUC hw/virtio-blk.c may handle I/O errors by keeping the request > > pending and on a list (->rq). This allows the user to rest

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
On Wed, Dec 12, 2012 at 02:50:50PM +0100, Stefan Hajnoczi wrote: > On Wed, Dec 12, 2012 at 12:51:01PM +0200, Michael S. Tsirkin wrote: > > Add sanity check to address the following concern: > > > > During migration, all we pass the index of the request; > > the rest can be re-read from the ring. >

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Paolo Bonzini
Il 12/12/2012 14:50, Stefan Hajnoczi ha scritto: > VirtIOBlock->rq can trigger the assertion. > > IIUC hw/virtio-blk.c may handle I/O errors by keeping the request > pending and on a list (->rq). This allows the user to restart them > after, for example, adding more space to the host file system

Re: [Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Stefan Hajnoczi
On Wed, Dec 12, 2012 at 12:51:01PM +0200, Michael S. Tsirkin wrote: > Add sanity check to address the following concern: > > During migration, all we pass the index of the request; > the rest can be re-read from the ring. > > This is not generally enough if any available requests are outstanding.

[Qemu-devel] [PATCHv2] virtio: verify that all outstanding buffers are flushed

2012-12-12 Thread Michael S. Tsirkin
Add sanity check to address the following concern: During migration, all we pass the index of the request; the rest can be re-read from the ring. This is not generally enough if any available requests are outstanding. Imagine a ring of size 4. Below A means available U means used. A 1 A 2 U 2 A