* [PATCH qemu] virtio-net: add feature bit for any header s/g
From: Michael S. Tsirkin @ 2012-09-28 9:31 UTC (permalink / raw)
To: Thomas Lendacky
Cc: Anthony Liguori, kvm, qemu-devel, virtualization, avi,
Sasha Levin
Old qemu versions required that 1st s/g entry is the header.
My recent patchset titled "virtio-net: iovec handling cleanup"
removed this limitation but a feature
bit is needed so guests know it's safe to lay out
header differently.
This patch applies on top and adds such a feature bit.
virtio net header inline with the data is beneficial
for latency and small packet bandwidth.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/virtio-net.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/virtio-net.h b/hw/virtio-net.h
index 36aa463..e7187e4 100644
--- a/hw/virtio-net.h
+++ b/hw/virtio-net.h
@@ -44,6 +44,7 @@
#define VIRTIO_NET_F_CTRL_RX 18 /* Control channel RX mode support */
#define VIRTIO_NET_F_CTRL_VLAN 19 /* Control channel VLAN filtering */
#define VIRTIO_NET_F_CTRL_RX_EXTRA 20 /* Extra RX mode control support */
+#define VIRTIO_NET_F_ANY_HEADER_SG 22 /* Host can handle any header s/g */
#define VIRTIO_NET_S_LINK_UP 1 /* Link is up */
@@ -186,5 +187,6 @@ struct virtio_net_ctrl_mac {
DEFINE_PROP_BIT("ctrl_vq", _state, _field, VIRTIO_NET_F_CTRL_VQ, true), \
DEFINE_PROP_BIT("ctrl_rx", _state, _field, VIRTIO_NET_F_CTRL_RX, true), \
DEFINE_PROP_BIT("ctrl_vlan", _state, _field, VIRTIO_NET_F_CTRL_VLAN, true), \
- DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true)
+ DEFINE_PROP_BIT("ctrl_rx_extra", _state, _field, VIRTIO_NET_F_CTRL_RX_EXTRA, true), \
+ DEFINE_PROP_BIT("any_header_sg", _state, _field, VIRTIO_NET_F_ANY_HEADER_SG, true)
#endif
--
MST
^ permalink raw reply related
* Re: [PATCHv6 0/3] virtio_console: Add rproc_serial device
From: Amit Shah @ 2012-09-28 12:48 UTC (permalink / raw)
To: sjur.brandeland
Cc: Michael S. Tsirkin, sjurbren, linux-kernel, virtualization,
Masami Hiramatsu, Linus Walleij
In-Reply-To: <1348580837-10919-1-git-send-email-sjur.brandeland@stericsson.com>
On (Tue) 25 Sep 2012 [15:47:14], sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> I thought rebasing rproc_serial to linux-next was going to be trivial.
> But when starting the merge I realized that I had to refactor the
> the patches from Masami Hiramatsu. The splice support has the same issue
> as I faced, with different type of buffers in the out_vq.
> So I ended up refactoring the splice functionality. The code
> size got smaller so hopefully this a step in the right direction.
>
> This refactoring also make introduction of rproc_serial cleaner.
>
> As requested I also added a patch for not initializing buffers.
>
> I have tested the VIRTIO_CONSOLE device by looping large amount of data
> through character device and tty, with lockdep and slub-debug on.
> This looks stable for me. I've also done a simple test of splice.
Thanks; I just passed these patches through my testsuite and things
are working fine. I'll review them in some time.
Amit
^ permalink raw reply
* Re: [PATCH 3/3] virtio_console: Don't initialize buffers to zero
From: Amit Shah @ 2012-10-01 8:24 UTC (permalink / raw)
To: sjur.brandeland; +Cc: sjurbren, linux-kernel, virtualization
In-Reply-To: <1348580837-10919-4-git-send-email-sjur.brandeland@stericsson.com>
On (Tue) 25 Sep 2012 [15:47:17], sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Skip initializing the receive buffers.
This tells 'what', but not 'why'. Please add some more description.
For the generic virtio ports case, at least, my original thinking was
to not send random guest data to the host device. However, we don't
have any device isolation in the host yet, and doing that will be
expensive, so it's not going to be done in the near future.. this can
be safely skipped.
Also, please make this patch 1/3, so we don't end up doing
kzalloc->kmalloc+memset->kmalloc
Thanks,
Amit
^ permalink raw reply
* Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
From: Amit Shah @ 2012-10-01 9:35 UTC (permalink / raw)
To: sjur.brandeland
Cc: Michael S. Tsirkin, sjurbren, linux-kernel, virtualization,
Masami Hiramatsu, Linus Walleij
In-Reply-To: <1348580837-10919-2-git-send-email-sjur.brandeland@stericsson.com>
On (Tue) 25 Sep 2012 [15:47:15], sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> This merge reduces code size by unifying the approach for
> sending scatter-lists and regular buffers. Any type of
> write operation (splice, write, put_chars) will now allocate
> a port_buffer and send_buf() and free_buf() can always be used.
Thanks for this cleanup; I should've caught it at the review of the
virtio-trace patchset itself -- sorry for that.
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> cc: Rusty Russell <rusty@rustcorp.com.au>
> cc: Michael S. Tsirkin <mst@redhat.com>
> cc: Amit Shah <amit.shah@redhat.com>
> cc: Linus Walleij <linus.walleij@linaro.org>
> cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
> drivers/char/virtio_console.c | 141 ++++++++++++++++++-----------------------
> 1 files changed, 62 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 8ab9c3d..f4f7b04 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -111,6 +111,11 @@ struct port_buffer {
> size_t len;
> /* offset in the buf from which to consume data */
> size_t offset;
> +
> + /* If sgpages == 0 then buf is used, else sg is used */
> + unsigned int sgpages;
> +
> + struct scatterlist sg[1];
> };
>
> /*
> @@ -338,23 +343,46 @@ static inline bool use_multiport(struct ports_device *portdev)
>
> static void free_buf(struct port_buffer *buf)
> {
> + int i;
unsigned int
> +
> kfree(buf->buf);
buf->buf isn't set to NULL in case sgpages is > 0. Please set
buf->buf to NULL (and initialise other fields to default values) in
alloc_buf() (and leave this as is).
> +
> + if (buf->sgpages)
This 'if' is not necessary; just having the for loop will do the right
thing.
> + for (i = 0; i < buf->sgpages; i++) {
> + struct page *page = sg_page(&buf->sg[i]);
> + if (!page)
> + break;
> + put_page(page);
> + }
> +
> kfree(buf);
> }
>
> -static struct port_buffer *alloc_buf(size_t buf_size)
> +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> + int nrbufs)
Indentation is off.
> {
> struct port_buffer *buf;
> + size_t alloc_size;
>
> - buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> + /* Allocate buffer and the scatter list */
> + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
> + buf = kmalloc(alloc_size, GFP_ATOMIC);
This looks hacky, along with the 'struct scatterlist sg[1]' in
port_buffer above. Use a pointer instead? At the least, please
include a comment in struct port_buffer mentioning sg has to be the
last element in the struct.
> if (!buf)
> goto fail;
> - buf->buf = kzalloc(buf_size, GFP_KERNEL);
> +
> + buf->sgpages = nrbufs;
> + if (nrbufs > 0)
> + return buf;
> +
> + buf->buf = kmalloc(buf_size, GFP_ATOMIC);
That's a lot of GFP_ATOMICS; even for the cases that don't need them.
Maybe add a gfp param that only allocates GFP_ATOMIC memory from
callers in interrupt context. All existing code got switched to using
GFP_ATOMIC buffers as well, that's definitely not good.
> if (!buf->buf)
> goto free_buf;
> buf->len = 0;
> buf->offset = 0;
> buf->size = buf_size;
> +
> + /* Prepare scatter buffer for sending */
> + sg_init_one(buf->sg, buf->buf, buf_size);
> return buf;
>
> free_buf:
> @@ -476,52 +504,25 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
> return 0;
> }
>
> -struct buffer_token {
> - union {
> - void *buf;
> - struct scatterlist *sg;
> - } u;
> - /* If sgpages == 0 then buf is used, else sg is used */
> - unsigned int sgpages;
> -};
> -
> -static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
> -{
> - int i;
> - struct page *page;
> -
> - for (i = 0; i < nrpages; i++) {
> - page = sg_page(&sg[i]);
> - if (!page)
> - break;
> - put_page(page);
> - }
> - kfree(sg);
> -}
>
> /* Callers must take the port->outvq_lock */
> static void reclaim_consumed_buffers(struct port *port)
> {
> - struct buffer_token *tok;
> + struct port_buffer *buf;
> unsigned int len;
>
> if (!port->portdev) {
> /* Device has been unplugged. vqs are already gone. */
> return;
> }
> - while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
> - if (tok->sgpages)
> - reclaim_sg_pages(tok->u.sg, tok->sgpages);
> - else
> - kfree(tok->u.buf);
> - kfree(tok);
> + while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
> + free_buf(buf);
> port->outvq_full = false;
> }
> }
>
> -static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
> - int nents, size_t in_count,
> - struct buffer_token *tok, bool nonblock)
> +static ssize_t send_buf(struct port *port, struct port_buffer *buf, int nents,
> + size_t in_count, bool nonblock)
Indentation is off.
> {
> struct virtqueue *out_vq;
> ssize_t ret;
> @@ -534,7 +535,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
>
> reclaim_consumed_buffers(port);
>
> - ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
> + ret = virtqueue_add_buf(out_vq, buf->sg, nents, 0, buf, GFP_ATOMIC);
>
> /* Tell Host to go! */
> virtqueue_kick(out_vq);
> @@ -559,8 +560,11 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
> * we need to kmalloc a GFP_ATOMIC buffer each time the
> * console driver writes something out.
> */
> - while (!virtqueue_get_buf(out_vq, &len))
> + for (buf = virtqueue_get_buf(out_vq, &len); !buf;
> + buf = virtqueue_get_buf(out_vq, &len))
> cpu_relax();
Looks awkward, how about
while (!(buf = virtqueue_get_buf(out_vq, &len))
cpu_relax();
> +
> + free_buf(buf);
> done:
> spin_unlock_irqrestore(&port->outvq_lock, flags);
>
> @@ -572,36 +576,6 @@ done:
> return in_count;
> }
>
> -static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
> - bool nonblock)
> -{
> - struct scatterlist sg[1];
> - struct buffer_token *tok;
> -
> - tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
> - if (!tok)
> - return -ENOMEM;
> - tok->sgpages = 0;
> - tok->u.buf = in_buf;
> -
> - sg_init_one(sg, in_buf, in_count);
> -
> - return __send_to_port(port, sg, 1, in_count, tok, nonblock);
> -}
> -
> -static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
> - size_t in_count, bool nonblock)
> -{
> - struct buffer_token *tok;
> -
> - tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
> - if (!tok)
> - return -ENOMEM;
> - tok->sgpages = nents;
> - tok->u.sg = sg;
> -
> - return __send_to_port(port, sg, nents, in_count, tok, nonblock);
> -}
>
> /*
> * Give out the data that's requested from the buffer that we have
> @@ -748,7 +722,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
> size_t count, loff_t *offp)
> {
> struct port *port;
> - char *buf;
> + struct port_buffer *buf;
> ssize_t ret;
> bool nonblock;
>
> @@ -766,11 +740,11 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
>
> count = min((size_t)(32 * 1024), count);
>
> - buf = kmalloc(count, GFP_KERNEL);
> + buf = alloc_buf(port->out_vq, count, 0);
> if (!buf)
> return -ENOMEM;
>
> - ret = copy_from_user(buf, ubuf, count);
> + ret = copy_from_user(buf->buf, ubuf, count);
> if (ret) {
> ret = -EFAULT;
> goto free_buf;
> @@ -784,13 +758,13 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
> * through to the host.
> */
> nonblock = true;
> - ret = send_buf(port, buf, count, nonblock);
> + ret = send_buf(port, buf, 1, count, nonblock);
>
> if (nonblock && ret > 0)
> goto out;
>
> free_buf:
> - kfree(buf);
> + free_buf(buf);
> out:
> return ret;
> }
> @@ -856,6 +830,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
> struct port *port = filp->private_data;
> struct sg_list sgl;
> ssize_t ret;
> + struct port_buffer *buf;
> struct splice_desc sd = {
> .total_len = len,
> .flags = flags,
> @@ -867,17 +842,17 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
> if (ret < 0)
> return ret;
>
> + buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
> sgl.n = 0;
> sgl.len = 0;
> sgl.size = pipe->nrbufs;
> - sgl.sg = kmalloc(sizeof(struct scatterlist) * sgl.size, GFP_KERNEL);
> - if (unlikely(!sgl.sg))
> - return -ENOMEM;
> -
> + sgl.sg = buf->sg;
> sg_init_table(sgl.sg, sgl.size);
> ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
> if (likely(ret > 0))
> - ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
> + ret = send_buf(port, buf, sgl.n, sgl.len, true);
> + else
> + free_buf(buf);
>
> return ret;
> }
> @@ -1031,6 +1006,7 @@ static const struct file_operations port_fops = {
> static int put_chars(u32 vtermno, const char *buf, int count)
> {
> struct port *port;
> + struct port_buffer *port_buf;
>
> if (unlikely(early_put_chars))
> return early_put_chars(vtermno, buf, count);
> @@ -1039,7 +1015,13 @@ static int put_chars(u32 vtermno, const char *buf, int count)
> if (!port)
> return -EPIPE;
>
> - return send_buf(port, (void *)buf, count, false);
> + port_buf = alloc_buf(port->out_vq, count, 0);
> + if (port_buf == NULL)
> + return -ENOMEM;
> +
> + memcpy(port_buf->buf, buf, count);
Hm, this introduces an unnecessary copy for console ports, and is
going to make console IO slower. Can you think of a way to avoid
this? (Remember, this is done in interrupt context.)
Amit
^ permalink raw reply
* Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
From: Amit Shah @ 2012-10-01 9:39 UTC (permalink / raw)
To: Sjur BRENDELAND
Cc: sjurbren@gmail.com, Michael S. Tsirkin, Linus Walleij,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
yrl.pp-manager.tt@hitachi.com, Masami Hiramatsu
In-Reply-To: <81C3A93C17462B4BBD7E272753C1057923BD8365F7@EXDCVYMBSTM005.EQ1STM.local>
On (Wed) 26 Sep 2012 [09:48:12], Sjur BRENDELAND wrote:
> > > This merge reduces code size by unifying the approach for
> > > sending scatter-lists and regular buffers. Any type of
> > > write operation (splice, write, put_chars) will now allocate
> > > a port_buffer and send_buf() and free_buf() can always be used.
> >
> > Thanks!
> > This looks much nicer and simpler. I just have some comments below.
>
> OK, good to hear that you agree to this kind of change. I'll do a respin
> of this patch fixing the issues you have pointed out.
>
> > > static void free_buf(struct port_buffer *buf)
> > > {
> > > + int i;
> > > +
> > > kfree(buf->buf);
> >
> > this should be done only when !buf->sgpages, or (see below)
>
> Agree, I'll put this statement in an else branch to the if-statement below.
Not necessary; see my comments in another mail.
> > > -static struct port_buffer *alloc_buf(size_t buf_size)
> > > +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> > > + int nrbufs)
> > > {
> > > struct port_buffer *buf;
> > > + size_t alloc_size;
> > >
> > > - buf = kmalloc(sizeof(*buf), GFP_KERNEL);
> > > + /* Allocate buffer and the scatter list */
> > > + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
> >
> > This allocates one redundant sg entry when nrbuf > 0,
> > but I think it is OK. (just a comment)
>
> I did this on purpose for the sake of simplicity, but I can
> change this to something like:
> alloc_size = sizeof(*buf) + sizeof(buf->sg) * max(nrbufs - 1, 1);
>
>
> > > + buf = kmalloc(alloc_size, GFP_ATOMIC);
> >
> > This should be kzalloc(), or buf->buf and others are not initialized,
> > which will cause unexpected kfree bug at kfree(buf->buf) in free_buf.
>
> Agree, kzalloc() is better in this case.
Not really -- one thing I've picked up from Rusty is to use kmallocs
and explicitly initialise values. Then, missing such initialisations
(like in this case) will cause tools like valgrind to show the error
pretty quickly.
> > > if (!buf)
> > > goto fail;
> > > - buf->buf = kzalloc(buf_size, GFP_KERNEL);
> > > +
> > > + buf->sgpages = nrbufs;
> > > + if (nrbufs > 0)
> > > + return buf;
> > > +
> > > + buf->buf = kmalloc(buf_size, GFP_ATOMIC);
> >
> > You can also use kzalloc here as previous code does.
> > But if the reason why using kzalloc comes from the security,
> > I think kmalloc is enough here, since the host can access
> > all the guest pages anyway.
>
> With this new patch alloc_buf() is used both for both RX and TX.
> The out_vq did previously use malloc(). But I have preserved
> the legacy behavior for the in_vq by calling memset() in function
> fill_queue().
But we're dropping the memset/kzalloc anyway.
Amit
^ permalink raw reply
* Re: [PATCHv5 2/3] virtio_console: Add support for remoteproc serial
From: Amit Shah @ 2012-10-01 9:52 UTC (permalink / raw)
To: sjur.brandeland
Cc: Arnd Bergmann, Michael S. Tsirkin, sjurbren, linux-kernel,
virtualization, Linus Walleij
In-Reply-To: <1348580837-10919-3-git-send-email-sjur.brandeland@stericsson.com>
On (Tue) 25 Sep 2012 [15:47:16], sjur.brandeland@stericsson.com wrote:
> +static DEFINE_SPINLOCK(dma_bufs_lock);
> +static LIST_HEAD(pending_free_dma_bufs);
> +
> static void free_buf(struct port_buffer *buf)
> {
> int i;
> + unsigned long flags;
>
> - kfree(buf->buf);
> + if (!buf->dev)
> + kfree(buf->buf);
Doesn't hurt to just kfree a NULL pointer; so check is not needed.
> - if (buf->sgpages)
> + if (buf->sgpages) {
> for (i = 0; i < buf->sgpages; i++) {
> struct page *page = sg_page(&buf->sg[i]);
> if (!page)
> break;
> put_page(page);
> }
> + return;
> + }
No kfree(buf)?
> + if (buf->dev && is_rproc_enabled) {
> +
> + /* dma_free_coherent requires interrupts to be enabled. */
> + if (irqs_disabled()) {
> + /* queue up dma-buffers to be freed later */
> + spin_lock_irqsave(&dma_bufs_lock, flags);
> + list_add_tail(&buf->list, &pending_free_dma_bufs);
> + spin_unlock_irqrestore(&dma_bufs_lock, flags);
> + return;
> + }
> + dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
> +
> + /* Release device refcnt and allow it to be freed */
> + might_sleep();
> + put_device(buf->dev);
> + }
>
> kfree(buf);
> }
>
> +static void reclaim_dma_bufs(void)
> +{
> + unsigned long flags;
> + struct port_buffer *buf, *tmp;
> + LIST_HEAD(tmp_list);
> +
> + WARN_ON(irqs_disabled());
> + if (list_empty(&pending_free_dma_bufs))
> + return;
> +
> + BUG_ON(!is_rproc_enabled);
> +
> + /* Create a copy of the pending_free_dma_bufs while holding the lock*/
> + spin_lock_irqsave(&dma_bufs_lock, flags);
> + list_cut_position(&tmp_list, &pending_free_dma_bufs,
> + pending_free_dma_bufs.prev);
> + spin_unlock_irqrestore(&dma_bufs_lock, flags);
> +
> + /* Release the dma buffers, without irqs enabled */
> + list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
> + list_del(&buf->list);
> + free_buf(buf);
> + }
> +}
> +
> static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> int nrbufs)
> {
> struct port_buffer *buf;
> size_t alloc_size;
>
> + if (is_rproc_serial(vq->vdev) && !irqs_disabled())
> + reclaim_dma_bufs();
> +
> /* Allocate buffer and the scatter list */
> alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs;
> - buf = kmalloc(alloc_size, GFP_ATOMIC);
> + buf = kzalloc(alloc_size, GFP_ATOMIC);
Why change from kmalloc to kzalloc? It's better to split out such
changes into separate patch for easier review.
> if (!buf)
> goto fail;
>
> @@ -374,11 +444,30 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
> if (nrbufs > 0)
> return buf;
>
> - buf->buf = kmalloc(buf_size, GFP_ATOMIC);
> + if (is_rproc_serial(vq->vdev)) {
> + /*
> + * Allocate DMA memory from ancestor. When a virtio
> + * device is created by remoteproc, the DMA memory is
> + * associated with the grandparent device:
> + * vdev => rproc => platform-dev.
> + * The code here would have been less quirky if
> + * DMA_MEMORY_INCLUDES_CHILDREN had been supported
> + * in dma-coherent.c
> + */
> + if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
> + goto free_buf;
> + buf->dev = vq->vdev->dev.parent->parent;
> +
> + /* Increase device refcnt to avoid freeing it*/
missing space before */
> + get_device(buf->dev);
> + buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf->dma,
> + GFP_ATOMIC);
> + } else {
> + buf->buf = kmalloc(buf_size, GFP_ATOMIC);
> + }
> +
> if (!buf->buf)
> goto free_buf;
> - buf->len = 0;
> - buf->offset = 0;
As noted in a comment to previous patch, please use kmalloc and
explicit initialisation of variables.
> buf->size = buf_size;
>
> /* Prepare scatter buffer for sending */
> @@ -838,6 +927,10 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
> .u.data = &sgl,
> };
>
> + /* rproc_serial does not support splice */
> + if (is_rproc_serial(port->out_vq->vdev))
> + return -EINVAL;
Will something break if this is not done?
Amit
^ permalink raw reply
* Re: [PATCH 0/2] virtio-scsi fixes for 3.6
From: Paolo Bonzini @ 2012-10-01 13:11 UTC (permalink / raw)
Cc: linux-scsi, virtualization, linux-kernel, JBottomley, kvm
In-Reply-To: <1343309288-32615-1-git-send-email-pbonzini@redhat.com>
Il 26/07/2012 15:28, Paolo Bonzini ha scritto:
> James,
>
> patch 1 fixes scanning of LUNs whose number is greater than 255. QEMU
> passes a max_lun of 16383 (because it uses SAM numbering) but in Linux
> it must become 32768 (because LUNs above 255 are "relocated" to 16640).
> Patch 2 is a resubmission of the patch for online resizing of virtio-scsi
> LUNs, which needs to be rebased.
>
> LUNs above 255 now work for all of scanning, hotplug, hotunplug and
> resize.
>
> Thanks,
>
> Paolo
>
> Paolo Bonzini (2):
> virtio-scsi: fix LUNs greater than 255
> virtio-scsi: support online resizing of disks
>
> drivers/scsi/virtio_scsi.c | 37 +++++++++++++++++++++++++++++++++++--
> include/linux/virtio_scsi.h | 2 ++
> 2 files changed, 37 insertions(+), 2 deletions(-)
>
Ping, are these patches going into 3.7?
Paolo
^ permalink raw reply
* Linux-next changes for module and virtio trees.
From: Rusty Russell @ 2012-10-02 6:26 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linux-next, linux-kernel, kvm-devel, virtualization
Hi Stephen,
Please remove my quilt tree
http://ozlabs.org/~rusty/kernel/rr-latest/ from linux-next, and use my
git trees from now on:
git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git
Branches:
modules-next
virtio-next
For others: beware that these will rebase, usually once after the merge
window closes (so I can work on a more modern tree), and again just
before it opens (where I squash in fixup patches).
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 0/2] virtio-scsi fixes for 3.6
From: Paolo Bonzini @ 2012-10-02 9:12 UTC (permalink / raw)
To: James Bottomley; +Cc: virtualization, linux-kernel, kvm, linux-scsi
In-Reply-To: <1349165906.2406.13.camel@dabdike.int.hansenpartnership.com>
Il 02/10/2012 10:18, James Bottomley ha scritto:
> On Mon, 2012-10-01 at 15:11 +0200, Paolo Bonzini wrote:
>> Il 26/07/2012 15:28, Paolo Bonzini ha scritto:
>>> James,
>>>
>>> patch 1 fixes scanning of LUNs whose number is greater than 255. QEMU
>>> passes a max_lun of 16383 (because it uses SAM numbering) but in Linux
>>> it must become 32768 (because LUNs above 255 are "relocated" to 16640).
>>> Patch 2 is a resubmission of the patch for online resizing of virtio-scsi
>>> LUNs, which needs to be rebased.
>>>
>>> LUNs above 255 now work for all of scanning, hotplug, hotunplug and
>>> resize.
>>>
>>> Thanks,
>>>
>>> Paolo
>>>
>>> Paolo Bonzini (2):
>>> virtio-scsi: fix LUNs greater than 255
>>> virtio-scsi: support online resizing of disks
>>>
>>> drivers/scsi/virtio_scsi.c | 37 +++++++++++++++++++++++++++++++++++--
>>> include/linux/virtio_scsi.h | 2 ++
>>> 2 files changed, 37 insertions(+), 2 deletions(-)
>>>
>>
>> Ping, are these patches going into 3.7?
>
> They're 3.7 candidates yes (enhancements certainly aren't 3.6).
> I seem to have become lost with the virtio-scsi updates since what I have
> marked for inclusion is a patch series that's a partial intersection
> with this. I'll flush my queue for virto-scsi, please resend all the
> missing patches you want in 3.7.
Ok, will do so soon and tag the appropriate ones for stable.
Paolo
^ permalink raw reply
* Re: [PATCH] virtio-blk: Disable callback in virtblk_done()
From: Stefan Hajnoczi @ 2012-10-02 14:42 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Michael S. Tsirkin, kvm, virtualization
In-Reply-To: <50642400.5030906@redhat.com>
On Thu, Sep 27, 2012 at 12:01 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 27/09/2012 02:10, Rusty Russell ha scritto:
>>>> >> + do {
>>>> >> + virtqueue_disable_cb(vq);
>>>> >> + while ((vbr = virtqueue_get_buf(vblk->vq, &len)) != NULL) {
>>>> >> + if (vbr->bio) {
>>>> >> + virtblk_bio_done(vbr);
>>>> >> + bio_done = true;
>>>> >> + } else {
>>>> >> + virtblk_request_done(vbr);
>>>> >> + req_done = true;
>>>> >> + }
>>>> >> }
>>>> >> - }
>>>> >> + } while (!virtqueue_enable_cb(vq));
>>>> >> /* In case queue is stopped waiting for more buffers. */
>>>> >> if (req_done)
>>>> >> blk_start_queue(vblk->disk->queue);
>> Fascinating. Please just confirm that VIRTIO_RING_F_EVENT_IDX is
>> enabled?
>
> Yeah, it's a nice and cheap trick. Stefan, I see that you had this in
> virtio-scsi since even before I picked it up. Do you remember how you
> came up with it?
I've played with disable_cb/enable_cb previously in virtio-blk and
virtio-scsi when aliguori had suggested reducing notifies. I
definitely didn't invent it :).
Stefan
^ permalink raw reply
* Re: Linux-next changes for module and virtio trees.
From: Stephen Rothwell @ 2012-10-02 15:12 UTC (permalink / raw)
To: Rusty Russell; +Cc: linux-next, linux-kernel, kvm-devel, virtualization
In-Reply-To: <871uhh4cen.fsf@rustcorp.com.au>
[-- Attachment #1.1: Type: text/plain, Size: 952 bytes --]
Hi Rusty,
On Tue, 02 Oct 2012 15:56:56 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> Please remove my quilt tree
> http://ozlabs.org/~rusty/kernel/rr-latest/ from linux-next, and use my
> git trees from now on:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git
> Branches:
> modules-next
> virtio-next
Done.
> For others: beware that these will rebase, usually once after the merge
> window closes (so I can work on a more modern tree), and again just
> before it opens (where I squash in fixup patches).
The first should not be necessary since everything in these trees should
have been merged by Linus during the merge window, so a merge of -rc1 or
later should be a fast forward (though merging with the tag itself may
produce a real merge). The second can be tricky to pick the right
moment :-)
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Rusty Russell @ 2012-10-03 6:44 UTC (permalink / raw)
To: Michael S. Tsirkin, Thomas Lendacky
Cc: kvm, netdev, linux-kernel, virtualization, avi, Sasha Levin
In-Reply-To: <cover.1348824232.git.mst@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> Thinking about Sasha's patches, we can reduce ring usage
> for virtio net small packets dramatically if we put
> virtio net header inline with the data.
> This can be done for free in case guest net stack allocated
> extra head room for the packet, and I don't see
> why would this have any downsides.
I've been wanting to do this for the longest time... but...
> Even though with my recent patches qemu
> no longer requires header to be the first s/g element,
> we need a new feature bit to detect this.
> A trivial qemu patch will be sent separately.
There's a reason I haven't done this. I really, really dislike "my
implemention isn't broken" feature bits. We could have an infinite
number of them, for each bug in each device.
So my plan was to tie this assumption to the new PCI layout. And have a
stress-testing patch like the one below in the kernel (see my virtio-wip
branch for stuff like this). Turn it on at boot with
"virtio_ring.torture" on the kernel commandline.
BTW, I've fixed lguest, but my kvm here (Ubuntu precise, kvm-qemu 1.0)
is too old. Building the latest git now...
Cheers,
Rusty.
Subject: virtio: CONFIG_VIRTIO_DEVICE_TORTURE
Virtio devices are not supposed to depend on the framing of the scatter-gather
lists, but various implementations did. Safeguard this in future by adding
an option to deliberately create perverse descriptors.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 8d5bddb..930a4ea 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -5,6 +5,15 @@ config VIRTIO
bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST,
CONFIG_RPMSG or CONFIG_S390_GUEST.
+config VIRTIO_DEVICE_TORTURE
+ bool "Virtio device torture tests"
+ depends on VIRTIO && DEBUG_KERNEL
+ help
+ This makes the virtio_ring implementation creatively change
+ the format of requests to make sure that devices are
+ properly implemented. This will make your virtual machine
+ slow *and* unreliable! Say N.
+
menu "Virtio drivers"
config VIRTIO_PCI
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index e639584..8893753 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -124,6 +124,149 @@ struct vring_virtqueue
#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
+#ifdef CONFIG_VIRTIO_DEVICE_TORTURE
+static bool torture;
+module_param(torture, bool, 0644);
+
+struct torture {
+ unsigned int orig_out, orig_in;
+ void *orig_data;
+ struct scatterlist sg[4];
+ struct scatterlist orig_sg[];
+};
+
+static size_t tot_len(struct scatterlist sg[], unsigned num)
+{
+ size_t len, i;
+
+ for (len = 0, i = 0; i < num; i++)
+ len += sg[i].length;
+
+ return len;
+}
+
+static void copy_sg_data(const struct scatterlist *dst, unsigned dnum,
+ const struct scatterlist *src, unsigned snum)
+{
+ unsigned len;
+ struct scatterlist s, d;
+
+ s = *src;
+ d = *dst;
+
+ while (snum && dnum) {
+ len = min(s.length, d.length);
+ memcpy(sg_virt(&d), sg_virt(&s), len);
+ d.offset += len;
+ d.length -= len;
+ s.offset += len;
+ s.length -= len;
+ if (!s.length) {
+ BUG_ON(snum == 0);
+ src++;
+ snum--;
+ s = *src;
+ }
+ if (!d.length) {
+ BUG_ON(dnum == 0);
+ dst++;
+ dnum--;
+ d = *dst;
+ }
+ }
+}
+
+static bool torture_replace(struct scatterlist **sg,
+ unsigned int *out,
+ unsigned int *in,
+ void **data,
+ gfp_t gfp)
+{
+ static size_t seed;
+ struct torture *t;
+ size_t outlen, inlen, ourseed, len1;
+ void *buf;
+
+ if (!torture)
+ return true;
+
+ outlen = tot_len(*sg, *out);
+ inlen = tot_len(*sg + *out, *in);
+
+ /* This will break horribly on large block requests. */
+ t = kmalloc(sizeof(*t) + (*out + *in) * sizeof(t->orig_sg[1])
+ + outlen + 1 + inlen + 1, gfp);
+ if (!t)
+ return false;
+
+ sg_init_table(t->sg, 4);
+ buf = &t->orig_sg[*out + *in];
+
+ memcpy(t->orig_sg, *sg, sizeof(**sg) * (*out + *in));
+ t->orig_out = *out;
+ t->orig_in = *in;
+ t->orig_data = *data;
+ *data = t;
+
+ ourseed = ACCESS_ONCE(seed);
+ seed++;
+
+ *sg = t->sg;
+ if (outlen) {
+ /* Split outbuf into two parts, one byte apart. */
+ *out = 2;
+ len1 = ourseed % (outlen + 1);
+ sg_set_buf(&t->sg[0], buf, len1);
+ buf += len1 + 1;
+ sg_set_buf(&t->sg[1], buf, outlen - len1);
+ buf += outlen - len1;
+ copy_sg_data(t->sg, *out, t->orig_sg, t->orig_out);
+ }
+
+ if (inlen) {
+ /* Split inbuf into two parts, one byte apart. */
+ *in = 2;
+ len1 = ourseed % (inlen + 1);
+ sg_set_buf(&t->sg[*out], buf, len1);
+ buf += len1 + 1;
+ sg_set_buf(&t->sg[*out + 1], buf, inlen - len1);
+ buf += inlen - len1;
+ }
+ return true;
+}
+
+static void *torture_done(struct torture *t)
+{
+ void *data;
+
+ if (!torture)
+ return t;
+
+ if (t->orig_in)
+ copy_sg_data(t->orig_sg + t->orig_out, t->orig_in,
+ t->sg + (t->orig_out ? 2 : 0), 2);
+
+ data = t->orig_data;
+ kfree(t);
+ return data;
+}
+
+#else
+static bool torture_replace(struct scatterlist **sg,
+ unsigned int *out,
+ unsigned int *in,
+ void **data,
+ gfp_t gfp)
+{
+ return true;
+}
+
+static void *torture_done(void *data)
+{
+ return data;
+}
+#endif /* CONFIG_VIRTIO_DEVICE_TORTURE */
+
/* Set up an indirect table of descriptors and add it to the queue. */
static int vring_add_indirect(struct vring_virtqueue *vq,
struct scatterlist sg[],
@@ -213,6 +356,9 @@ int virtqueue_add_buf(struct virtqueue *_vq,
BUG_ON(data == NULL);
+ if (!torture_replace(&sg, &out, &in, &data, gfp))
+ return -ENOMEM;
+
#ifdef DEBUG
{
ktime_t now = ktime_get();
@@ -246,6 +392,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
if (out)
vq->notify(&vq->vq);
END_USE(vq);
+ torture_done(data);
return -ENOSPC;
}
@@ -476,7 +623,7 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
#endif
END_USE(vq);
- return ret;
+ return torture_done(ret);
}
EXPORT_SYMBOL_GPL(virtqueue_get_buf);
^ permalink raw reply related
* Re: [PATCH 0/3] virtio-net: inline header support
From: Paolo Bonzini @ 2012-10-03 10:53 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, Sasha Levin,
virtualization, Thomas Lendacky, avi
In-Reply-To: <87vces2gxq.fsf__45058.6618776017$1349247807$gmane$org@rustcorp.com.au>
Il 03/10/2012 08:44, Rusty Russell ha scritto:
> There's a reason I haven't done this. I really, really dislike "my
> implemention isn't broken" feature bits. We could have an infinite
> number of them, for each bug in each device.
However, this bug affects (almost) all implementations and (almost) all
devices. It even makes sense to reserve a transport feature bit for it
instead of a device feature bit.
Paolo
^ permalink raw reply
* Re: [Qemu-devel] Proposal for virtio standardization.
From: Anthony Liguori @ 2012-10-04 18:49 UTC (permalink / raw)
To: Rusty Russell, Adam Litke, Amit Shah, Avi Kivity, Avishay Traeger,
Jason Wang, Michael S. Tsirkin, Ohad Ben-Cohen, Paolo Bonzini,
Pawel Moll, Sasha Levin, Cornelia Huck
Cc: virtualization, qemu-devel, kvm, LKML
In-Reply-To: <87zk4c2tqq.fsf@rustcorp.com.au>
Rusty Russell <rusty@rustcorp.com.au> writes:
> Hi all,
>
> I've had several requests for a more formal approach to the
> virtio draft spec, and (after some soul-searching) I'd like to try that.
>
> The proposal is to use OASIS as the standards body, as it's
> fairly light-weight as these things go. For me this means paperwork and
> setting up a Working Group and getting the right people involved as
> Voting members starting with the current contributors; for most of you
> it just means a new mailing list, though I'll be cross-posting any
> drafts and major changes here anyway.
>
> I believe that a documented standard (aka virtio 1.0) will
> increase visibility and adoption in areas outside our normal linux/kvm
> universe. There's been some of that already, but this is the clearest
> path to accelerate it. Not the easiest path, but I believe that a solid
> I/O standard is a Good Thing for everyone.
>
> Yet I also want to decouple new and experimental development
> from the standards effort; running code comes first. New feature bits
> and new device numbers should be reservable without requiring a full
> spec change.
>
> So the essence of my proposal is:
> 1) I start a Working Group within OASIS where we can aim for virtio spec
> 1.0.
>
> 2) The current spec is textually reordered so the core is clearly
> bus-independent, with PCI, mmio, etc appendices.
>
> 3) Various clarifications, formalizations and cleanups to the spec text,
> and possibly elimination of old deprecated features.
>
> 4) The only significant change to the spec is that we use PCI
> capabilities, so we can have infinite feature bits.
> (see http://lists.linuxfoundation.org/pipermail/virtualization/2011-December/019198.html)
We discussed this on IRC last night. I don't think PCI capabilites are
a good mechanism to use...
PCI capabilities are there to organize how the PCI config space is
allocated to allow vendor extensions to co-exist with future PCI
extensions.
But we've never used the PCI config space within virtio-pci. We do
everything in BAR0. I don't think there's any real advantage of using
the config space vs. a BAR for virtio-pci.
The nice thing about using a BAR is that the region is MMIO or PIO.
This maps really nicely to non-PCI transports too. But extending the
PCI config space (especially dealing with capability allocation) is
pretty gnarly and there isn't an obvious equivalent outside of PCI.
There are very devices that we emulate today that make use of extended
PCI device registers outside the platform devices (that have no BARs).
Regards,
Anthony Liguori
>
> 5) Changes to the ring layout and other such things are deferred to a
> future virtio version; whether this is done within OASIS or
> externally depends on how well this works for the 1.0 release.
>
> Thoughts?
> Rusty.
^ permalink raw reply
* [PULL] virtio
From: Rusty Russell @ 2012-10-05 1:39 UTC (permalink / raw)
To: Linus Torvalds
Cc: kvm, Michael S. Tsirkin, Kent Overstreet, Peter Senna Tschudin,
virtualization, Masami Hiramatsu, Christoph Hellwig,
Alexey Khoroshilov, Brian Foley, Shaohua Li, Dan Carpenter,
Pawel Moll, Steven Rostedt, Amit Shah, Jens Axboe, LKML,
Tejun Heo, Paolo Bonzini
The following changes since commit 925a6f0bf8bd122d5d2429af7f0ca0fecf4ae71f:
Merge tag 'hwspinlock-3.6-fix' of git://git.kernel.org/pub/scm/linux/kernel/git/ohad/hwspinlock (2012-09-18 11:58:54 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git virtio-next
for you to fetch changes up to ca16f580a5db7e60bfafe59a50bb133bd3347491:
lguest: fix occasional crash in example launcher. (2012-10-04 12:12:59 +0930)
----------------------------------------------------------------
New workflow: same git trees pulled by linux-next get sent straight to Linus.
Git is awkward at shuffling patches compared with quilt or mq, but that doesn't
happen often once things get into my -next branch.
----------------------------------------------------------------
Alexey Khoroshilov (1):
virtio: console: fix error handling in init() function
Asias He (3):
virtio-blk: Add bio-based IO path for virtio-blk
virtio-blk: Add REQ_FLUSH and REQ_FUA support to bio path
virtio-blk: Disable callback in virtblk_done()
Brian Foley (2):
virtio_mmio: fix off by one error allocating queue
virtio_mmio: Don't attempt to create empty virtqueues
Dan Carpenter (1):
virtio-blk: fix NULL checking in virtblk_alloc_req()
Jason Wang (2):
virtio-ring: move queue_index to vring_virtqueue
virtio: introduce an API to set affinity for a virtqueue
Masami Hiramatsu (5):
virtio/console: Add splice_write support
virtio/console: Add a failback for unstealable pipe buffer
virtio/console: Wait until the port is ready on splice
ftrace: Allow stealing pages from pipe buffer
virtio/console: Allocate scatterlist according to the current pipe size
Michael S. Tsirkin (3):
virtio-balloon: dependency fix
virtio: support reserved vqs
virtio: don't crash when device is buggy
Peter Senna Tschudin (1):
drivers/virtio/virtio_pci.c: fix error return code
Rusty Russell (4):
virtio_balloon: not EXPERIMENTAL any more.
virtio: add help to CONFIG_VIRTIO option.
virtio: remove CONFIG_VIRTIO_RING
lguest: fix occasional crash in example launcher.
Yoshihiro YUNOMAE (2):
tools: Add guest trace agent as a user tool
tools: Fix pthread flag for Makefile of trace-agent used by virtio-trace
arch/s390/Kconfig | 1 -
arch/x86/lguest/Kconfig | 1 -
drivers/block/virtio_blk.c | 306 +++++++++++++++++++++++----
drivers/char/virtio_console.c | 210 ++++++++++++++++--
drivers/lguest/lguest_device.c | 5 +-
drivers/remoteproc/remoteproc_virtio.c | 5 +-
drivers/rpmsg/Kconfig | 1 -
drivers/s390/kvm/kvm_virtio.c | 5 +-
drivers/virtio/Kconfig | 17 +-
drivers/virtio/Makefile | 3 +-
drivers/virtio/virtio.c | 2 +-
drivers/virtio/virtio_mmio.c | 29 ++-
drivers/virtio/virtio_pci.c | 68 +++++-
drivers/virtio/virtio_ring.c | 14 +-
include/linux/virtio.h | 2 +
include/linux/virtio_config.h | 23 ++
include/linux/virtio_ring.h | 3 +-
kernel/trace/trace.c | 8 +-
tools/lguest/lguest.c | 1 +
tools/virtio/virtio-trace/Makefile | 13 ++
tools/virtio/virtio-trace/README | 118 +++++++++++
tools/virtio/virtio-trace/trace-agent-ctl.c | 137 ++++++++++++
tools/virtio/virtio-trace/trace-agent-rw.c | 192 +++++++++++++++++
tools/virtio/virtio-trace/trace-agent.c | 270 +++++++++++++++++++++++
tools/virtio/virtio-trace/trace-agent.h | 75 +++++++
25 files changed, 1402 insertions(+), 107 deletions(-)
create mode 100644 tools/virtio/virtio-trace/Makefile
create mode 100644 tools/virtio/virtio-trace/README
create mode 100644 tools/virtio/virtio-trace/trace-agent-ctl.c
create mode 100644 tools/virtio/virtio-trace/trace-agent-rw.c
create mode 100644 tools/virtio/virtio-trace/trace-agent.c
create mode 100644 tools/virtio/virtio-trace/trace-agent.h
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Rusty Russell @ 2012-10-05 5:43 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, Sasha Levin,
virtualization, Thomas Lendacky, avi
In-Reply-To: <506D8DD5.20904@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 04/10/2012 14:51, Rusty Russell ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 04/10/2012 02:11, Rusty Russell ha scritto:
>>>>>> There's a reason I haven't done this. I really, really dislike "my
>>>>>> implemention isn't broken" feature bits. We could have an infinite
>>>>>> number of them, for each bug in each device.
>>>>>
>>>>> However, this bug affects (almost) all implementations and (almost) all
>>>>> devices. It even makes sense to reserve a transport feature bit for it
>>>>> instead of a device feature bit.
>>>>
>>>> Perhaps, but we have to fix the bugs first!
>>>
>>> Yes. :) Isn't that what mst's patch does?
>>>
>>>> As I said, my torture patch broke qemu immediately. Since noone has
>>>> leapt onto fixing that, I'll take a look now...
>>>
>>> I can look at virtio-scsi.
>>
>> Actually, you can't, see my reply to Anthony...
>>
>> Message-ID: <87lifm1y1n.fsf@rustcorp.com.au>
>
> struct virtio_scsi_req_cmd {
> // Read-only
> u8 lun[8];
> u64 id;
> u8 task_attr;
> u8 prio;
> u8 crn;
> char cdb[cdb_size];
> char dataout[];
> // Write-only part
> u32 sense_len;
> u32 residual;
> u16 status_qualifier;
> u8 status;
> u8 response;
> u8 sense[sense_size];
> char datain[];
> };
>
> where cdb_size and sense_size come from configuration space. The device
> right now expects everything before dataout/datain to be in a single
> descriptor, but that's in no way part of the spec. Am I missing
> something egregious?
Since you wrote it, I hope not :)
That's good. But virtio_blk's scsi command is insoluble AFAICT. As I
said to Anthony, the best rules are "always" and "never", so I'd really
rather not have to grandfather that in.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Paolo Bonzini @ 2012-10-05 7:47 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Sasha Levin, Anthony Liguori, avi, Thomas Lendacky
In-Reply-To: <87lifm1y1n.fsf@rustcorp.com.au>
Il 04/10/2012 09:44, Rusty Russell ha scritto:
> -In particular, no implementation should use the descriptor
> -boundaries to determine the size of any header in a request.[footnote:
> -The current qemu device implementations mistakenly insist that
> -the first descriptor cover the header in these cases exactly, so
> -a cautious driver should arrange it so.
> +[footnote:
> +It was previously asserted that framing should be independent of
> +message contents, yet invariably drivers layed out messages in
> +reliable ways and devices assumed it. In addition, the
> +specifications for virtio_blk and virtio_scsi require intuiting
> +field lengths from frame boundaries.
> ]
Not true for virtio_scsi...
Paolo
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Paolo Bonzini @ 2012-10-06 12:54 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Sasha Levin, avi, Thomas Lendacky
In-Reply-To: <87391t1nkq.fsf__40391.6521034718$1349505001$gmane$org@rustcorp.com.au>
Il 05/10/2012 07:43, Rusty Russell ha scritto:
>> > struct virtio_scsi_req_cmd {
>> > // Read-only
>> > u8 lun[8];
>> > u64 id;
>> > u8 task_attr;
>> > u8 prio;
>> > u8 crn;
>> > char cdb[cdb_size];
>> > char dataout[];
>> > // Write-only part
>> > u32 sense_len;
>> > u32 residual;
>> > u16 status_qualifier;
>> > u8 status;
>> > u8 response;
>> > u8 sense[sense_size];
>> > char datain[];
>> > };
>> >
>> > where cdb_size and sense_size come from configuration space. The device
>> > right now expects everything before dataout/datain to be in a single
>> > descriptor, but that's in no way part of the spec. Am I missing
>> > something egregious?
> Since you wrote it, I hope not :)
Yeah, I guess the confusion came from cdb_size and sense_size being in
configuration space.
> That's good. But virtio_blk's scsi command is insoluble AFAICT. As I
> said to Anthony, the best rules are "always" and "never", so I'd really
> rather not have to grandfather that in.
It is, but we can add a rule that if the (transport) flag
VIRTIO_RING_F_ANY_HEADER_SG is set, the cdb field is always 32 bytes in
virtio-blk.
Paolo
^ permalink raw reply
* [PATCH] xen/x86: remove duplicated include from enlighten.c
From: Wei Yongjun @ 2012-10-07 14:06 UTC (permalink / raw)
To: konrad.wilk, jeremy, tglx, mingo, hpa
Cc: linux-kernel, yongjun_wei, x86, xen-devel, virtualization
From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
Remove duplicated include.
dpatch engine is used to auto generate this patch.
(https://github.com/weiyj/dpatch)
Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
arch/x86/xen/enlighten.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index bf788d3..061b148 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -81,8 +81,6 @@
#include "smp.h"
#include "multicalls.h"
-#include <xen/events.h>
-
EXPORT_SYMBOL_GPL(hypercall_page);
DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
^ permalink raw reply related
* Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-08 2:21 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Michael S. Tsirkin, qemu-devel, kvm, virtualization
In-Reply-To: <874nmajcmj.fsf@codemonkey.ws>
(Topic updated, cc's trimmed).
Anthony Liguori <aliguori@us.ibm.com> writes:
> Rusty Russell <rusty@rustcorp.com.au> writes:
>> 4) The only significant change to the spec is that we use PCI
>> capabilities, so we can have infinite feature bits.
>> (see http://lists.linuxfoundation.org/pipermail/virtualization/2011-December/019198.html)
>
> We discussed this on IRC last night. I don't think PCI capabilites are
> a good mechanism to use...
>
> PCI capabilities are there to organize how the PCI config space is
> allocated to allow vendor extensions to co-exist with future PCI
> extensions.
>
> But we've never used the PCI config space within virtio-pci. We do
> everything in BAR0. I don't think there's any real advantage of using
> the config space vs. a BAR for virtio-pci.
Note before anyone gets confused; we were talking about using the PCI
config space to indicate what BAR(s) the virtio stuff is in. An
alternative would be to simply specify a new layout format in BAR1.
The arguments for a more flexible format that I know of:
1) virtio-pci has already extended the pci-specific part of the
configuration once (for MSI-X), so I don't want to assume it won't
happen again.
2) ISTR an argument about mapping the ISR register separately, for
performance, but I can't find a reference to it.
> This maps really nicely to non-PCI transports too.
This isn't right. Noone else can use the PCI layout. While parts are
common, other parts are pci-specific (MSI-X and ISR for example), and
yet other parts are specified by PCI elsewhere (eg interrupt numbers).
> But extending the
> PCI config space (especially dealing with capability allocation) is
> pretty gnarly and there isn't an obvious equivalent outside of PCI.
That's OK, because general changes should be done with feature bits, and
the others all have an infinite number. Being the first, virtio-pci has
some unique limitations we'd like to fix.
> There are very devices that we emulate today that make use of extended
> PCI device registers outside the platform devices (that have no BARs).
This sentence confused me?
Thanks,
Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Anthony Liguori @ 2012-10-08 13:58 UTC (permalink / raw)
To: Rusty Russell; +Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <87y5jhpuu2.fsf@rustcorp.com.au>
Rusty Russell <rusty@rustcorp.com.au> writes:
> (Topic updated, cc's trimmed).
>
> Anthony Liguori <aliguori@us.ibm.com> writes:
>> Rusty Russell <rusty@rustcorp.com.au> writes:
>>> 4) The only significant change to the spec is that we use PCI
>>> capabilities, so we can have infinite feature bits.
>>> (see http://lists.linuxfoundation.org/pipermail/virtualization/2011-December/019198.html)
>>
>> We discussed this on IRC last night. I don't think PCI capabilites are
>> a good mechanism to use...
>>
>> PCI capabilities are there to organize how the PCI config space is
>> allocated to allow vendor extensions to co-exist with future PCI
>> extensions.
>>
>> But we've never used the PCI config space within virtio-pci. We do
>> everything in BAR0. I don't think there's any real advantage of using
>> the config space vs. a BAR for virtio-pci.
>
> Note before anyone gets confused; we were talking about using the PCI
> config space to indicate what BAR(s) the virtio stuff is in. An
> alternative would be to simply specify a new layout format in BAR1.
>
> The arguments for a more flexible format that I know of:
>
> 1) virtio-pci has already extended the pci-specific part of the
> configuration once (for MSI-X), so I don't want to assume it won't
> happen again.
"configuration" is the wrong word here.
The virtio-pci BAR0 layout is:
0..19 virtio-pci registers
20+ virtio configuration space
MSI-X needed to add additional virtio-pci registers, so now we have:
0..19 virtio-pci registers
if MSI-X:
20..23 virtio-pci MSI-X registers
24+ virtio configuration space
else:
20+ virtio configuration space
I agree, this stinks.
But I think we could solve this in a different way. I think we could
just move the virtio configuration space to BAR1 by using a transport
feature bit.
That then frees up the entire BAR0 for use as virtio-pci registers. We
can then always include the virtio-pci MSI-X register space and
introduce all new virtio-pci registers as simply being appended.
This new feature bit then becomes essentially a virtio configuration
latch. When unacked, virtio configuration hides new registers, when
acked, those new registers are exposed.
Another option is to simply put new registers after the virtio
configuration blob.
> 2) ISTR an argument about mapping the ISR register separately, for
> performance, but I can't find a reference to it.
I think the rationale is that ISR really needs to be PIO but everything
else doesn't. PIO is much faster on x86 because it doesn't require
walking page tables or instruction emulation to handle the exit.
The argument to move the remaining registers to MMIO is to allow 64-bit
accesses to registers which isn't possible with PIO.
>> This maps really nicely to non-PCI transports too.
>
> This isn't right. Noone else can use the PCI layout. While parts are
> common, other parts are pci-specific (MSI-X and ISR for example), and
> yet other parts are specified by PCI elsewhere (eg interrupt numbers).
>
>> But extending the
>> PCI config space (especially dealing with capability allocation) is
>> pretty gnarly and there isn't an obvious equivalent outside of PCI.
>
> That's OK, because general changes should be done with feature bits, and
> the others all have an infinite number. Being the first, virtio-pci has
> some unique limitations we'd like to fix.
>
>> There are very devices that we emulate today that make use of extended
>> PCI device registers outside the platform devices (that have no BARs).
>
> This sentence confused me?
There is a missing "few". "There are very few devices..."
Extending the PCI configuration space is unusual for PCI devices. That
was the point.
Regards,
Anthony Liguori
>
> Thanks,
> Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Gerd Hoffmann @ 2012-10-08 14:58 UTC (permalink / raw)
To: Anthony Liguori; +Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <87bogddq0l.fsf@codemonkey.ws>
Hi,
> But I think we could solve this in a different way. I think we could
> just move the virtio configuration space to BAR1 by using a transport
> feature bit.
Why hard-code stuff?
I think it makes alot of sense to have a capability simliar to msi-x
which simply specifies bar and offset of the register sets:
[root@fedora ~]# lspci -vvs4
00:04.0 SCSI storage controller: Red Hat, Inc Virtio block device
[ ... ]
Region 0: I/O ports at c000 [size=64]
Region 1: Memory at fc029000 (32-bit) [size=4K]
Capabilities: [40] MSI-X: Enable+ Count=2 Masked-
Vector table: BAR=1 offset=00000000
PBA: BAR=1 offset=00000800
So we could have for virtio something like this:
Capabilities: [??] virtio-regs:
legacy: BAR=0 offset=0
virtio-pci: BAR=1 offset=1000
virtio-cfg: BAR=1 offset=1800
> That then frees up the entire BAR0 for use as virtio-pci registers. We
> can then always include the virtio-pci MSI-X register space and
> introduce all new virtio-pci registers as simply being appended.
BAR0 needs to stay as-is for compatibility reasons. New devices which
don't have to care about old guests don't need to provide a 'legacy'
register region.
Most devices have mmio at BAR1 for msi-x support anyway, we can place
the virtio-pci and virtio configuration registers there too by default.
I wouldn't hardcode that though.
> This new feature bit then becomes essentially a virtio configuration
> latch. When unacked, virtio configuration hides new registers, when
> acked, those new registers are exposed.
I'd just expose them all all the time.
>> 2) ISTR an argument about mapping the ISR register separately, for
>> performance, but I can't find a reference to it.
>
> I think the rationale is that ISR really needs to be PIO but everything
> else doesn't. PIO is much faster on x86 because it doesn't require
> walking page tables or instruction emulation to handle the exit.
Is this still a pressing issue? With MSI-X enabled ISR isn't needed,
correct? Which would imply that pretty much only old guests without
MSI-X support need this, and we don't need to worry that much when
designing something new ...
cheers,
Gerd
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Anthony Liguori @ 2012-10-08 15:09 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <5072EA14.30809@redhat.com>
Gerd Hoffmann <kraxel@redhat.com> writes:
> Hi,
>
>> But I think we could solve this in a different way. I think we could
>> just move the virtio configuration space to BAR1 by using a transport
>> feature bit.
>
> Why hard-code stuff?
>
> I think it makes alot of sense to have a capability simliar to msi-x
> which simply specifies bar and offset of the register sets:
>
> [root@fedora ~]# lspci -vvs4
> 00:04.0 SCSI storage controller: Red Hat, Inc Virtio block device
> [ ... ]
> Region 0: I/O ports at c000 [size=64]
> Region 1: Memory at fc029000 (32-bit) [size=4K]
> Capabilities: [40] MSI-X: Enable+ Count=2 Masked-
> Vector table: BAR=1 offset=00000000
> PBA: BAR=1 offset=00000800
MSI-X capability is a standard PCI capability which is why lspci can
parse it.
>
> So we could have for virtio something like this:
>
> Capabilities: [??] virtio-regs:
> legacy: BAR=0 offset=0
> virtio-pci: BAR=1 offset=1000
> virtio-cfg: BAR=1 offset=1800
This would be a vendor specific PCI capability so lspci wouldn't
automatically know how to parse it.
You could just as well teach lspci to parse BAR0 to figure out what
features are supported.
>> That then frees up the entire BAR0 for use as virtio-pci registers. We
>> can then always include the virtio-pci MSI-X register space and
>> introduce all new virtio-pci registers as simply being appended.
>
> BAR0 needs to stay as-is for compatibility reasons. New devices which
> don't have to care about old guests don't need to provide a 'legacy'
> register region.
A latch feature bit would allow the format to change without impacting
compatibility at all.
>>> 2) ISTR an argument about mapping the ISR register separately, for
>>> performance, but I can't find a reference to it.
>>
>> I think the rationale is that ISR really needs to be PIO but everything
>> else doesn't. PIO is much faster on x86 because it doesn't require
>> walking page tables or instruction emulation to handle the exit.
>
> Is this still a pressing issue? With MSI-X enabled ISR isn't needed,
> correct? Which would imply that pretty much only old guests without
> MSI-X support need this, and we don't need to worry that much when
> designing something new ...
It wasn't that long ago that MSI-X wasn't supported.. I think we should
continue to keep ISR as PIO as it is a fast path.
Regards,
Anthony Liguori
>
> cheers,
> Gerd
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Gerd Hoffmann @ 2012-10-08 20:13 UTC (permalink / raw)
To: Anthony Liguori; +Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <87k3v1gfw1.fsf@codemonkey.ws>
Hi,
>> So we could have for virtio something like this:
>>
>> Capabilities: [??] virtio-regs:
>> legacy: BAR=0 offset=0
>> virtio-pci: BAR=1 offset=1000
>> virtio-cfg: BAR=1 offset=1800
>
> This would be a vendor specific PCI capability so lspci wouldn't
> automatically know how to parse it.
Sure, would need a patch to actually parse+print the cap,
/me was just trying to make my point clear in a simple way.
>>>> 2) ISTR an argument about mapping the ISR register separately, for
>>>> performance, but I can't find a reference to it.
>>>
>>> I think the rationale is that ISR really needs to be PIO but everything
>>> else doesn't. PIO is much faster on x86 because it doesn't require
>>> walking page tables or instruction emulation to handle the exit.
>>
>> Is this still a pressing issue? With MSI-X enabled ISR isn't needed,
>> correct? Which would imply that pretty much only old guests without
>> MSI-X support need this, and we don't need to worry that much when
>> designing something new ...
>
> It wasn't that long ago that MSI-X wasn't supported.. I think we should
> continue to keep ISR as PIO as it is a fast path.
No problem if we allow to have both legacy layout and new layout at the
same time. Guests can continue to use ISR @ BAR0 in PIO space for
existing virtio devices, even in case they want use mmio for other
registers -> all fine.
New virtio devices can support MSI-X from day one and decide to not
expose a legacy layout PIO bar.
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Michael S. Tsirkin @ 2012-10-08 20:41 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, netdev, linux-kernel, Sasha Levin, virtualization, avi,
Thomas Lendacky
In-Reply-To: <87vces2gxq.fsf@rustcorp.com.au>
On Wed, Oct 03, 2012 at 04:14:17PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> > Thinking about Sasha's patches, we can reduce ring usage
> > for virtio net small packets dramatically if we put
> > virtio net header inline with the data.
> > This can be done for free in case guest net stack allocated
> > extra head room for the packet, and I don't see
> > why would this have any downsides.
>
> I've been wanting to do this for the longest time... but...
>
> > Even though with my recent patches qemu
> > no longer requires header to be the first s/g element,
> > we need a new feature bit to detect this.
> > A trivial qemu patch will be sent separately.
>
> There's a reason I haven't done this. I really, really dislike "my
> implemention isn't broken" feature bits. We could have an infinite
> number of them, for each bug in each device.
>
> So my plan was to tie this assumption to the new PCI layout.
I don't object but old qemu has this limitation for s390 as well,
and that's not using PCI, right? So how do we detect
new hypervisor there?
--
MST
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox