qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Kevin Wolf <kwolf@redhat.com>,
	Anthony Liguori <aliguori@us.ibm.com>,
	rusty@rustcorp.com.au, yvugenfi@redhat.com,
	qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org,
	Blue Swirl <blauwirbel@gmail.com>,
	khoa@us.ibm.com, Stefan Hajnoczi <stefanha@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>, Asias He <asias@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v6 12/12] virtio-blk: add x-data-plane=on|off performance feature
Date: Tue, 18 Dec 2012 17:22:40 +0200	[thread overview]
Message-ID: <20121218152239.GE27400@redhat.com> (raw)
In-Reply-To: <20121218145717.GA11063@stefanha-thinkpad.redhat.com>

On Tue, Dec 18, 2012 at 03:57:17PM +0100, Stefan Hajnoczi wrote:
> > > @@ -407,6 +409,14 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > >          .num_writes = 0,
> > >      };
> > >  
> > > +    /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
> > > +     * dataplane here instead of waiting for .set_status().
> > > +     */
> > 
> > By the way which guests are these?
> 
> I ran a Windows 8 guest today with build 48 virtio-win drivers.  It
> notifies before the device gets its .set_status() callback invoked.
> But I could swear I've seen Linux guests do this too.


That's very broken. But looking at linux drivers it also
seems linux guests do this even today.
We have:

       err = drv->probe(dev);
        if (err)
                add_status(dev, VIRTIO_CONFIG_S_FAILED);
        else {
                add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
                if (drv->scan)
                        drv->scan(dev);
        }

this means that unless drivers implement scan() they
will make device active before DRIVER_OK is written
as the result linux can access it and kick.
And almost no drivers implement scan.
Nasty.

Rusty, what do you think? Worth fixing?

It does mean that for now we are stuck
with a work around, but I think we need it
in virtio core in qemu, it's not dataplane
specific.

-- 
MST

  reply	other threads:[~2012-12-18 15:20 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-10 13:09 [Qemu-devel] [PATCH v6 00/12] virtio: virtio-blk data plane Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 01/12] raw-posix: add raw_get_aio_fd() for virtio-blk-data-plane Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 02/12] configure: add CONFIG_VIRTIO_BLK_DATA_PLANE Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 03/12] dataplane: add host memory mapping code Stefan Hajnoczi
2012-12-11 14:13   ` Michael S. Tsirkin
2012-12-11 15:27     ` Stefan Hajnoczi
2012-12-11 15:42       ` Michael S. Tsirkin
2012-12-11 16:32         ` Anthony Liguori
2012-12-11 18:09           ` Michael S. Tsirkin
2012-12-12 15:34             ` Stefan Hajnoczi
2012-12-12 15:49               ` Michael S. Tsirkin
2012-12-14 11:45                 ` Stefan Hajnoczi
2012-12-16 16:11                   ` Michael S. Tsirkin
2012-12-17  9:09                     ` Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 04/12] dataplane: add virtqueue vring code Stefan Hajnoczi
2012-12-11 14:18   ` Michael S. Tsirkin
2012-12-12 15:55     ` Stefan Hajnoczi
2012-12-12 16:32       ` Michael S. Tsirkin
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 05/12] dataplane: add event loop Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 06/12] dataplane: add Linux AIO request queue Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 07/12] iov: add iov_discard_front/back() to remove data Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 08/12] test-iov: add iov_discard_front/back() testcases Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 09/12] iov: add qemu_iovec_concat_iov() Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 10/12] virtio-blk: restore VirtIOBlkConf->config_wce flag Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 11/12] dataplane: add virtio-blk data plane code Stefan Hajnoczi
2012-12-10 13:09 ` [Qemu-devel] [PATCH v6 12/12] virtio-blk: add x-data-plane=on|off performance feature Stefan Hajnoczi
2012-12-16 16:08   ` Michael S. Tsirkin
2012-12-18 14:57     ` Stefan Hajnoczi
2012-12-18 15:22       ` Michael S. Tsirkin [this message]
2012-12-20  4:04         ` Rusty Russell
2012-12-10 13:13 ` [Qemu-devel] [PATCH v6 00/12] virtio: virtio-blk data plane Stefan Hajnoczi
2012-12-11  8:53 ` Stefan Hajnoczi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20121218152239.GE27400@redhat.com \
    --to=mst@redhat.com \
    --cc=aliguori@us.ibm.com \
    --cc=asias@redhat.com \
    --cc=blauwirbel@gmail.com \
    --cc=khoa@us.ibm.com \
    --cc=kwolf@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=rusty@rustcorp.com.au \
    --cc=stefanha@gmail.com \
    --cc=stefanha@redhat.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=yvugenfi@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).