* Re: Using PCI config space to indicate config location
From: Michael S. Tsirkin @ 2012-10-10 8:30 UTC (permalink / raw)
To: Rusty Russell; +Cc: Anthony Liguori, qemu-devel, kvm, virtualization
In-Reply-To: <87y5jhpuu2.fsf@rustcorp.com.au>
On Mon, Oct 08, 2012 at 12:51:25PM +1030, Rusty Russell wrote:
> (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.
One problem we are still left with is this: device specific
config accesses are still non atomic.
This is a problem for multibyte fields such as MAC address
where MAC could change while we are accessing it.
I was thinking about some backwards compatible way to solve this, but if
we are willing to break compatiblity or use some mode switch, how about
we give up on virtio config space completely, and do everything besides
IO and ISR through guest memory?
--
MST
^ permalink raw reply
* [PATCH] vhost-blk: Add vhost-blk support v3
From: Asias He @ 2012-10-10 7:20 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization
vhost-blk is an in-kernel virito-blk device accelerator.
Due to lack of proper in-kernel AIO interface, this version converts
guest's I/O request to bio and use submit_bio() to submit I/O directly.
So this version any supports raw block device as guest's disk image,
e.g. /dev/sda, /dev/ram0. We can add file based image support to
vhost-blk once we have in-kernel AIO interface. There are some work in
progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown:
http://marc.info/?l=linux-fsdevel&m=133312234313122
Performance evaluation:
-----------------------------
1) LKVM
Fio with libaio ioengine on Fusion IO device using kvm tool
IOPS Before After Improvement
seq-read 107 121 +13.0%
seq-write 130 179 +37.6%
rnd-read 102 122 +19.6%
rnd-write 125 159 +27.0%
2) QEMU
Fio with libaio ioengine on Fusion IO device using QEMU
IOPS Before After Improvement
seq-read 76 123 +61.8%
seq-write 139 173 +24.4%
rnd-read 73 120 +64.3%
rnd-write 75 156 +108.0%
Userspace bits:
-----------------------------
1) LKVM
The latest vhost-blk userspace bits for kvm tool can be found here:
git@github.com:asias/linux-kvm.git blk.vhost-blk
2) QEMU
The latest vhost-blk userspace prototype for QEMU can be found here:
git@github.com:asias/qemu.git blk.vhost-blk
Changes in V3:
- Sending REQ_FLUSH bio instead of vfs_fsync, thanks Christoph!
- Check file passed by user is a raw block device file
Signed-off-by: Asias He <asias@redhat.com>
---
drivers/vhost/Kconfig | 1 +
drivers/vhost/Kconfig.blk | 10 +
drivers/vhost/Makefile | 2 +
drivers/vhost/blk.c | 669 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/vhost/blk.h | 8 +
5 files changed, 690 insertions(+)
create mode 100644 drivers/vhost/Kconfig.blk
create mode 100644 drivers/vhost/blk.c
create mode 100644 drivers/vhost/blk.h
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 202bba6..acd8038 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -11,4 +11,5 @@ config VHOST_NET
if STAGING
source "drivers/vhost/Kconfig.tcm"
+source "drivers/vhost/Kconfig.blk"
endif
diff --git a/drivers/vhost/Kconfig.blk b/drivers/vhost/Kconfig.blk
new file mode 100644
index 0000000..ff8ab76
--- /dev/null
+++ b/drivers/vhost/Kconfig.blk
@@ -0,0 +1,10 @@
+config VHOST_BLK
+ tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
+ depends on BLOCK && EXPERIMENTAL && m
+ ---help---
+ This kernel module can be loaded in host kernel to accelerate
+ guest block with virtio_blk. Not to be confused with virtio_blk
+ module itself which needs to be loaded in guest kernel.
+
+ To compile this driver as a module, choose M here: the module will
+ be called vhost_blk.
diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
index a27b053..1a8a4a5 100644
--- a/drivers/vhost/Makefile
+++ b/drivers/vhost/Makefile
@@ -2,3 +2,5 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o
vhost_net-y := vhost.o net.o
obj-$(CONFIG_TCM_VHOST) += tcm_vhost.o
+obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
+vhost_blk-y := blk.o
diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
new file mode 100644
index 0000000..4a4e793
--- /dev/null
+++ b/drivers/vhost/blk.c
@@ -0,0 +1,669 @@
+/*
+ * Copyright (C) 2011 Taobao, Inc.
+ * Author: Liu Yuan <tailai.ly@taobao.com>
+ *
+ * Copyright (C) 2012 Red Hat, Inc.
+ * Author: Asias He <asias@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ *
+ * virtio-blk server in host kernel.
+ */
+
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/vhost.h>
+#include <linux/virtio_blk.h>
+#include <linux/mutex.h>
+#include <linux/file.h>
+#include <linux/kthread.h>
+#include <linux/blkdev.h>
+
+#include "vhost.c"
+#include "vhost.h"
+#include "blk.h"
+
+#define BLK_HDR 0
+
+static DEFINE_IDA(vhost_blk_index_ida);
+
+enum {
+ VHOST_BLK_VQ_REQ = 0,
+ VHOST_BLK_VQ_MAX = 1,
+};
+
+struct req_page_list {
+ struct page **pages;
+ int pages_nr;
+};
+
+struct vhost_blk_req {
+ struct llist_node llnode;
+ struct req_page_list *pl;
+ struct vhost_blk *blk;
+
+ struct iovec *iov;
+ int iov_nr;
+
+ struct bio **bio;
+ atomic_t bio_nr;
+
+ sector_t sector;
+ int write;
+ u16 head;
+ long len;
+
+ u8 *status;
+};
+
+struct vhost_blk {
+ struct task_struct *host_kick;
+ struct vhost_blk_req *reqs;
+ struct vhost_virtqueue vq;
+ struct llist_head llhead;
+ struct vhost_dev dev;
+ u16 reqs_nr;
+ int index;
+};
+
+static inline int iov_num_pages(struct iovec *iov)
+{
+ return (PAGE_ALIGN((unsigned long)iov->iov_base + iov->iov_len) -
+ ((unsigned long)iov->iov_base & PAGE_MASK)) >> PAGE_SHIFT;
+}
+
+static int vhost_blk_setup(struct vhost_blk *blk)
+{
+ blk->reqs_nr = blk->vq.num;
+
+ blk->reqs = kmalloc(sizeof(struct vhost_blk_req) * blk->reqs_nr,
+ GFP_KERNEL);
+ if (!blk->reqs)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static inline int vhost_blk_set_status(struct vhost_blk_req *req, u8 status)
+{
+ struct vhost_blk *blk = req->blk;
+
+ if (copy_to_user(req->status, &status, sizeof(status))) {
+ vq_err(&blk->vq, "Failed to write status\n");
+ vhost_discard_vq_desc(&blk->vq, 1);
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+static void vhost_blk_enable_vq(struct vhost_blk *blk,
+ struct vhost_virtqueue *vq)
+{
+ wake_up_process(blk->host_kick);
+}
+
+static void vhost_blk_req_done(struct bio *bio, int err)
+{
+ struct vhost_blk_req *req = bio->bi_private;
+ struct vhost_blk *blk = req->blk;
+
+ if (err)
+ req->len = err;
+
+ if (atomic_dec_and_test(&req->bio_nr)) {
+ llist_add(&req->llnode, &blk->llhead);
+ wake_up_process(blk->host_kick);
+ }
+
+ bio_put(bio);
+}
+
+static void vhost_blk_req_umap(struct vhost_blk_req *req)
+{
+ struct req_page_list *pl;
+ int i, j;
+
+
+ if (!req->pl) {
+ kfree(req->bio);
+ return;
+ }
+
+ for (i = 0; i < req->iov_nr; i++) {
+ pl = &req->pl[i];
+ for (j = 0; j < pl->pages_nr; j++) {
+ if (!req->write)
+ set_page_dirty_lock(pl->pages[j]);
+ page_cache_release(pl->pages[j]);
+ }
+ }
+
+ kfree(req->pl);
+}
+
+static int vhost_blk_bio_make(struct vhost_blk_req *req,
+ struct block_device *bdev)
+{
+ int pages_nr_total, i, j, ret;
+ struct iovec *iov = req->iov;
+ int iov_nr = req->iov_nr;
+ struct page **pages, *page;
+ struct bio *bio = NULL;
+ int bio_nr = 0;
+
+ req->len = 0;
+ pages_nr_total = 0;
+ for (i = 0; i < iov_nr; i++) {
+ req->len += iov[i].iov_len;
+ pages_nr_total += iov_num_pages(&iov[i]);
+ }
+
+ if (unlikely(req->write == WRITE_FLUSH)) {
+ req->pl = NULL;
+ req->bio = kmalloc(sizeof(struct bio *), GFP_KERNEL);
+ bio = bio_alloc(GFP_KERNEL, 1);
+ if (!bio) {
+ kfree(req->bio);
+ return -ENOMEM;
+ }
+ bio->bi_sector = req->sector;
+ bio->bi_bdev = bdev;
+ bio->bi_private = req;
+ bio->bi_end_io = vhost_blk_req_done;
+ req->bio[bio_nr++] = bio;
+
+ goto out;
+ }
+
+ req->pl = kmalloc((iov_nr * sizeof(struct req_page_list)) +
+ (pages_nr_total * sizeof(struct page *)) +
+ (pages_nr_total * sizeof(struct bio *)),
+ GFP_KERNEL);
+ if (!req->pl)
+ return -ENOMEM;
+ pages = (struct page **)&req->pl[iov_nr];
+ req->bio = (struct bio **)&pages[pages_nr_total];
+
+ req->iov_nr = 0;
+ for (i = 0; i < iov_nr; i++) {
+ int pages_nr = iov_num_pages(&iov[i]);
+ unsigned long iov_base, iov_len;
+ struct req_page_list *pl;
+
+ iov_base = (unsigned long)iov[i].iov_base;
+ iov_len = (unsigned long)iov[i].iov_len;
+
+ ret = get_user_pages_fast(iov_base, pages_nr,
+ !req->write, pages);
+ if (ret != pages_nr)
+ goto fail;
+
+ req->iov_nr++;
+ pl = &req->pl[i];
+ pl->pages_nr = pages_nr;
+ pl->pages = pages;
+
+ for (j = 0; j < pages_nr; j++) {
+ unsigned int off, len;
+ page = pages[j];
+ off = iov_base & ~PAGE_MASK;
+ len = PAGE_SIZE - off;
+ if (len > iov_len)
+ len = iov_len;
+
+ while (!bio || bio_add_page(bio, page, len, off) <= 0) {
+ bio = bio_alloc(GFP_KERNEL, pages_nr);
+ if (!bio)
+ goto fail;
+ bio->bi_sector = req->sector;
+ bio->bi_bdev = bdev;
+ bio->bi_private = req;
+ bio->bi_end_io = vhost_blk_req_done;
+ req->bio[bio_nr++] = bio;
+ }
+ req->sector += len >> 9;
+ iov_base += len;
+ iov_len -= len;
+ }
+
+ pages += pages_nr;
+ }
+out:
+ atomic_set(&req->bio_nr, bio_nr);
+ return 0;
+
+fail:
+ for (i = 0; i < bio_nr; i++)
+ bio_put(req->bio[i]);
+ vhost_blk_req_umap(req);
+ return -ENOMEM;
+}
+
+static inline void vhost_blk_bio_send(struct vhost_blk_req *req)
+{
+ struct blk_plug plug;
+ int i, bio_nr;
+
+ bio_nr = atomic_read(&req->bio_nr);
+ blk_start_plug(&plug);
+ for (i = 0; i < bio_nr; i++)
+ submit_bio(req->write, req->bio[i]);
+ blk_finish_plug(&plug);
+}
+
+static int vhost_blk_req_submit(struct vhost_blk_req *req, struct file *file)
+{
+
+ struct inode *inode = file->f_mapping->host;
+ struct block_device *bdev = inode->i_bdev;
+ int ret;
+
+ ret = vhost_blk_bio_make(req, bdev);
+ if (ret < 0)
+ return ret;
+
+ vhost_blk_bio_send(req);
+
+ return ret;
+}
+
+static int vhost_blk_req_done_thread(void *data)
+{
+ mm_segment_t oldfs = get_fs();
+ struct vhost_blk *blk = data;
+ struct vhost_virtqueue *vq;
+ struct llist_node *llnode;
+ struct vhost_blk_req *req;
+ bool added;
+ u8 status;
+ int ret;
+
+ vq = &blk->vq;
+ set_fs(USER_DS);
+ use_mm(blk->dev.mm);
+ for (;;) {
+ llnode = llist_del_all(&blk->llhead);
+ if (!llnode) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule();
+ if (unlikely(kthread_should_stop()))
+ break;
+ continue;
+ }
+ added = false;
+ while (llnode) {
+ req = llist_entry(llnode, struct vhost_blk_req, llnode);
+ llnode = llist_next(llnode);
+
+ vhost_blk_req_umap(req);
+
+ status = req->len > 0 ?
+ VIRTIO_BLK_S_OK : VIRTIO_BLK_S_IOERR;
+ ret = copy_to_user(req->status, &status,
+ sizeof(status));
+ if (unlikely(ret)) {
+ vq_err(&blk->vq, "Failed to write status\n");
+ return -1;
+ }
+ vhost_add_used(&blk->vq, req->head, req->len);
+ added = true;
+ }
+ if (likely(added))
+ vhost_signal(&blk->dev, &blk->vq);
+
+ }
+ unuse_mm(blk->dev.mm);
+ set_fs(oldfs);
+ return 0;
+}
+
+static void vhost_blk_flush(struct vhost_blk *blk)
+{
+ vhost_poll_flush(&blk->vq.poll);
+}
+
+static struct file *vhost_blk_stop_vq(struct vhost_blk *blk,
+ struct vhost_virtqueue *vq)
+{
+ struct file *file;
+
+ mutex_lock(&vq->mutex);
+ file = rcu_dereference_protected(vq->private_data,
+ lockdep_is_held(&vq->mutex));
+ rcu_assign_pointer(vq->private_data, NULL);
+ mutex_unlock(&vq->mutex);
+
+ return file;
+
+}
+
+static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
+{
+
+ *file = vhost_blk_stop_vq(blk, &blk->vq);
+}
+
+/* Handle guest request */
+static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
+ struct virtio_blk_outhdr *hdr,
+ u16 head, u16 out, u16 in,
+ struct file *file)
+{
+ struct vhost_blk *blk = container_of(vq->dev, struct vhost_blk, dev);
+ struct vhost_blk_req *req;
+ int iov_nr, ret;
+ u8 status;
+
+ if (hdr->type == VIRTIO_BLK_T_IN || hdr->type == VIRTIO_BLK_T_GET_ID)
+ iov_nr = in - 1;
+ else
+ iov_nr = out - 1;
+
+ req = &blk->reqs[head];
+ req->head = head;
+ req->status = blk->vq.iov[iov_nr + 1].iov_base;
+ req->blk = blk;
+ req->iov = &vq->iov[BLK_HDR + 1];
+ req->iov_nr = iov_nr;
+ req->sector = hdr->sector;
+
+ switch (hdr->type) {
+ case VIRTIO_BLK_T_OUT:
+ req->write = WRITE;
+ ret = vhost_blk_req_submit(req, file);
+ break;
+ case VIRTIO_BLK_T_IN:
+ req->write = READ;
+ ret = vhost_blk_req_submit(req, file);
+ break;
+ case VIRTIO_BLK_T_FLUSH:
+ req->write = WRITE_FLUSH;
+ ret = vhost_blk_req_submit(req, file);
+ break;
+ case VIRTIO_BLK_T_GET_ID:
+ ret = snprintf(vq->iov[BLK_HDR + 1].iov_base,
+ VIRTIO_BLK_ID_BYTES, "vhost-blk%d", blk->index);
+ status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
+ if (!vhost_blk_set_status(req, status))
+ vhost_add_used_and_signal(&blk->dev, vq, head, ret);
+ break;
+ default:
+ pr_warn("Unsupported request type %d\n", hdr->type);
+ vhost_discard_vq_desc(vq, 1);
+ ret = -EFAULT;
+ break;
+ }
+
+ return ret;
+}
+
+/* Guest kick us for IO request */
+static void vhost_blk_handle_guest_kick(struct vhost_work *work)
+{
+ struct virtio_blk_outhdr hdr;
+ struct vhost_virtqueue *vq;
+ struct vhost_blk *blk;
+ struct blk_plug plug;
+ struct file *f;
+ int in, out;
+ u16 head;
+
+ vq = container_of(work, struct vhost_virtqueue, poll.work);
+ blk = container_of(vq->dev, struct vhost_blk, dev);
+
+ /* TODO: check that we are running from vhost_worker? */
+ f = rcu_dereference_check(vq->private_data, 1);
+ if (!f)
+ return;
+
+ vhost_disable_notify(&blk->dev, vq);
+ blk_start_plug(&plug);
+ for (;;) {
+ head = vhost_get_vq_desc(&blk->dev, vq, vq->iov,
+ ARRAY_SIZE(vq->iov),
+ &out, &in, NULL, NULL);
+ if (unlikely(head < 0))
+ break;
+
+ if (unlikely(head == vq->num)) {
+ if (unlikely(vhost_enable_notify(&blk->dev, vq))) {
+ vhost_disable_notify(&blk->dev, vq);
+ continue;
+ }
+ break;
+ }
+
+ if (unlikely(copy_from_user(&hdr, vq->iov[BLK_HDR].iov_base,
+ sizeof(hdr)))) {
+ vq_err(vq, "Failed to get block header!\n");
+ vhost_discard_vq_desc(vq, 1);
+ break;
+ }
+
+ if (vhost_blk_req_handle(vq, &hdr, head, out, in, f) < 0)
+ break;
+ }
+ blk_finish_plug(&plug);
+}
+
+static int vhost_blk_open(struct inode *inode, struct file *file)
+{
+ struct vhost_blk *blk;
+ int ret;
+
+ blk = kzalloc(sizeof(*blk), GFP_KERNEL);
+ if (!blk) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = ida_simple_get(&vhost_blk_index_ida, 0, 0, GFP_KERNEL);
+ if (ret < 0)
+ goto out_dev;
+ blk->index = ret;
+
+ blk->vq.handle_kick = vhost_blk_handle_guest_kick;
+
+ ret = vhost_dev_init(&blk->dev, &blk->vq, VHOST_BLK_VQ_MAX);
+ if (ret < 0)
+ goto out_dev;
+ file->private_data = blk;
+
+ blk->host_kick = kthread_create(vhost_blk_req_done_thread,
+ blk, "vhost-blk-%d", current->pid);
+ if (IS_ERR(blk->host_kick)) {
+ ret = PTR_ERR(blk->host_kick);
+ goto out_dev;
+ }
+
+ return ret;
+out_dev:
+ kfree(blk);
+out:
+ return ret;
+}
+
+static int vhost_blk_release(struct inode *inode, struct file *f)
+{
+ struct vhost_blk *blk = f->private_data;
+ struct file *file;
+
+ ida_simple_remove(&vhost_blk_index_ida, blk->index);
+ vhost_blk_stop(blk, &file);
+ vhost_blk_flush(blk);
+ vhost_dev_cleanup(&blk->dev, false);
+ if (file)
+ fput(file);
+ kthread_stop(blk->host_kick);
+ kfree(blk->reqs);
+ kfree(blk);
+
+ return 0;
+}
+
+static int vhost_blk_set_features(struct vhost_blk *blk, u64 features)
+{
+ mutex_lock(&blk->dev.mutex);
+ blk->dev.acked_features = features;
+ mutex_unlock(&blk->dev.mutex);
+
+ return 0;
+}
+
+static long vhost_blk_set_backend(struct vhost_blk *blk, unsigned index, int fd)
+{
+ struct vhost_virtqueue *vq = &blk->vq;
+ struct file *file, *oldfile;
+ struct inode *inode;
+ int ret;
+
+ mutex_lock(&blk->dev.mutex);
+ ret = vhost_dev_check_owner(&blk->dev);
+ if (ret)
+ goto out_dev;
+
+ if (index >= VHOST_BLK_VQ_MAX) {
+ ret = -ENOBUFS;
+ goto out_dev;
+ }
+
+ mutex_lock(&vq->mutex);
+
+ if (!vhost_vq_access_ok(vq)) {
+ ret = -EFAULT;
+ goto out_vq;
+ }
+
+ file = fget(fd);
+ if (IS_ERR(file)) {
+ ret = PTR_ERR(file);
+ goto out_vq;
+ }
+
+ /* Only raw block device is supported for now */
+ inode = file->f_mapping->host;
+ if (!S_ISBLK(inode->i_mode)) {
+ ret = -EFAULT;
+ goto out_file;
+ }
+
+ oldfile = rcu_dereference_protected(vq->private_data,
+ lockdep_is_held(&vq->mutex));
+ if (file != oldfile) {
+ rcu_assign_pointer(vq->private_data, file);
+ vhost_blk_enable_vq(blk, vq);
+
+ ret = vhost_init_used(vq);
+ if (ret)
+ goto out_file;
+ }
+
+ mutex_unlock(&vq->mutex);
+
+ if (oldfile) {
+ vhost_blk_flush(blk);
+ fput(oldfile);
+ }
+
+ mutex_unlock(&blk->dev.mutex);
+ return 0;
+
+out_file:
+ fput(file);
+out_vq:
+ mutex_unlock(&vq->mutex);
+out_dev:
+ mutex_unlock(&blk->dev.mutex);
+ return ret;
+}
+
+static long vhost_blk_reset_owner(struct vhost_blk *blk)
+{
+ struct file *file = NULL;
+ int err;
+
+ mutex_lock(&blk->dev.mutex);
+ err = vhost_dev_check_owner(&blk->dev);
+ if (err)
+ goto done;
+ vhost_blk_stop(blk, &file);
+ vhost_blk_flush(blk);
+ err = vhost_dev_reset_owner(&blk->dev);
+done:
+ mutex_unlock(&blk->dev.mutex);
+ if (file)
+ fput(file);
+ return err;
+}
+
+static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
+ unsigned long arg)
+{
+ struct vhost_blk *blk = f->private_data;
+ void __user *argp = (void __user *)arg;
+ struct vhost_vring_file backend;
+ u64 __user *featurep = argp;
+ u64 features;
+ int ret;
+
+ switch (ioctl) {
+ case VHOST_BLK_SET_BACKEND:
+ if (copy_from_user(&backend, argp, sizeof(backend)))
+ return -EFAULT;
+ return vhost_blk_set_backend(blk, backend.index, backend.fd);
+ case VHOST_GET_FEATURES:
+ features = VHOST_BLK_FEATURES;
+ if (copy_to_user(featurep, &features, sizeof(features)))
+ return -EFAULT;
+ return 0;
+ case VHOST_SET_FEATURES:
+ if (copy_from_user(&features, featurep, sizeof(features)))
+ return -EFAULT;
+ if (features & ~VHOST_BLK_FEATURES)
+ return -EOPNOTSUPP;
+ return vhost_blk_set_features(blk, features);
+ case VHOST_RESET_OWNER:
+ return vhost_blk_reset_owner(blk);
+ default:
+ mutex_lock(&blk->dev.mutex);
+ ret = vhost_dev_ioctl(&blk->dev, ioctl, arg);
+ if (!ret && ioctl == VHOST_SET_VRING_NUM)
+ ret = vhost_blk_setup(blk);
+ vhost_blk_flush(blk);
+ mutex_unlock(&blk->dev.mutex);
+ return ret;
+ }
+}
+
+static const struct file_operations vhost_blk_fops = {
+ .owner = THIS_MODULE,
+ .open = vhost_blk_open,
+ .release = vhost_blk_release,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = vhost_blk_ioctl,
+};
+
+static struct miscdevice vhost_blk_misc = {
+ MISC_DYNAMIC_MINOR,
+ "vhost-blk",
+ &vhost_blk_fops,
+};
+
+int vhost_blk_init(void)
+{
+ return misc_register(&vhost_blk_misc);
+}
+
+void vhost_blk_exit(void)
+{
+ misc_deregister(&vhost_blk_misc);
+}
+
+module_init(vhost_blk_init);
+module_exit(vhost_blk_exit);
+
+MODULE_VERSION("0.0.3");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Asias He");
+MODULE_DESCRIPTION("Host kernel accelerator for virtio_blk");
diff --git a/drivers/vhost/blk.h b/drivers/vhost/blk.h
new file mode 100644
index 0000000..2f674f0
--- /dev/null
+++ b/drivers/vhost/blk.h
@@ -0,0 +1,8 @@
+#include <linux/vhost.h>
+
+enum {
+ VHOST_BLK_FEATURES = (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
+ (1ULL << VIRTIO_RING_F_EVENT_IDX),
+};
+/* VHOST_BLK specific defines */
+#define VHOST_BLK_SET_BACKEND _IOW(VHOST_VIRTIO, 0x50, struct vhost_vring_file)
--
1.7.11.4
^ permalink raw reply related
* Re: Proposal for virtio standardization.
From: Rusty Russell @ 2012-10-10 3:46 UTC (permalink / raw)
To: Cornelia Huck
Cc: Anthony Liguori, Avishay Traeger, kvm, Pawel Moll,
Michael S. Tsirkin, qemu-devel, LKML, virtualization, Avi Kivity,
Amit Shah, Paolo Bonzini, Sasha Levin
In-Reply-To: <20121009160201.5303a7ca@BR9GNB5Z>
Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> On Thu, 27 Sep 2012 09:59:33 +0930
> Rusty Russell <rusty@rustcorp.com.au> wrote:
>> 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)
>
> "Infinite" only applies to virtio-pci, no?
Yes, you already have "infinite" feature bits for ccw, as does every bus
we did since PCI.
> Sounds like a good idea. I'll be happy to review the spec with an eye
> to virtio-ccw.
Thanks!
Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-10 3:44 UTC (permalink / raw)
To: Jamie Lokier
Cc: Anthony Liguori, kvm, Michael S. Tsirkin, qemu-devel,
virtualization
In-Reply-To: <20121009210901.GY14867@jl-vm1.vm.bytemark.co.uk>
Jamie Lokier <jamie@shareable.org> writes:
> Rusty Russell wrote:
>> I don't think it'll be that bad; reset clears the device to unknown,
>> bar0 moves it from unknown->legacy mode, bar1/2/3 changes it from
>> unknown->modern mode, and anything else is bad (I prefer being strict so
>> we catch bad implementations from the beginning).
>
> Will that work, if the guest with kernel that uses modern mode, kexecs
> to an older (but presumed reliable) kernel that only knows about legacy mode?
>
> I.e. will the replacement kernel, or (ideally) replacement driver on
> the rare occasion that is needed on a running kernel, be able to reset
> the device hard enough?
Well, you need to reset the device, so yes.
Cheers,
Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-10 3:44 UTC (permalink / raw)
To: Anthony Liguori, Gerd Hoffmann
Cc: Benjamin Herrenschmidt, virtualization, qemu-devel, kvm,
Michael S. Tsirkin
In-Reply-To: <874nm3ycj8.fsf@codemonkey.ws>
Anthony Liguori <aliguori@us.ibm.com> writes:
> Rusty Russell <rusty@rustcorp.com.au> writes:
>
>> Anthony Liguori <aliguori@us.ibm.com> writes:
>>> We'll never remove legacy so we shouldn't plan on it. There are
>>> literally hundreds of thousands of VMs out there with the current virtio
>>> drivers installed in them. We'll be supporting them for a very, very
>>> long time :-)
>>
>> You will be supporting this for qemu on x86, sure.
>
> And PPC.
Yeah, Ben was a bit early fixing the virtio bugs I guess. Oh well.
>> As I think we're
>> still in the growth phase for virtio, I prioritize future spec
>> cleanliness pretty high.
>>
>> But I think you'll be surprised how fast this is deprecated:
>> 1) Bigger queues for block devices (guest-specified ringsize)
>> 2) Smaller rings for openbios (guest-specified alignment)
>> 3) All-mmio mode (powerpc)
>> 4) Whatever network features get numbers > 31.
>
> We can do all of these things with incremental change to the existing
> layout. That's the only way what I'm suggesting is different.
Now extend your proposal to add all those features we want. We add
if()s to the driver that we need to keep forever. Let's just look at
what your proposal get us:
bool has_feature(struct virtio_pci_dev *dev, u32 num)
{
/* Do we need to set selector? */
if (dev->extended_features) {
unsigned long selector_off = VIRTIO_PCI_HOST_FEATURES_SELECT;
if (dev->using_bar0 && !dev->msi_active)
selector_off -= 32;
writel(dev->config + selector_off, num / 32);
num &= 31;
} else if (num > 31)
return false;
return readl(dev->config + VIRTIO_PCI_HOST_FEATURES) & (1 << num);
}
vs two cases which independent with methods set at detection time:
virtio_pci_legacy.c:
bool has_feature(struct virtio_pci_dev *dev, u32 num)
{
if (num > 31)
return false;
return readl(dev->config + VIRTIO_PCI_LEGACY_HOST_FEATURES) & (1 << num);
}
virtio_pci_new.c:
bool has_feature(struct virtio_pci_dev *dev, u32 num)
{
writel(dev->config + VIRTIO_PCI_HOST_FEATURES_SELECT, num / 32);
num &= 31;
return readl(dev->config + VIRTIO_PCI_HOST_FEATURES) & (1 << num);
}
> You want to reorder all of the fields and have a driver flag day. But I
> strongly suspect we'll decide we need to do the same exercise again in 4
> years when we now need to figure out how to take advantage of
> transactional memory or some other whiz-bang hardware feature.
It's not a flag day, as it's backwards compatible.
Sure, we might have a clean cut again. I'm not afraid to rewrite this
code to make it cleaner; perhaps this is somewhere qemu could benefit
from a similar approach.
> What I'm suggesting is:
>
>> struct {
>> __le32 host_features; /* read-only */
>> __le32 guest_features; /* read/write */
>> __le32 queue_pfn; /* read/write */
>> __le16 queue_size; /* read-only */
>> __le16 queue_sel; /* read/write */
>> __le16 queue_notify; /* read/write */
>> u8 status; /* read/write */
>> u8 isr; /* read-only, clear on read */
>> __le16 msi_config_vector; /* read/write */
>> __le16 msi_queue_vector; /* read/write */
>> __le32 host_feature_select; /* read/write */
>> __le32 guest_feature_select; /* read/write */
>> __le32 queue_pfn_hi; /* read/write */
>> };
>
> With the additional semantic that the virtio-config space is overlayed
> on top of the register set in BAR0 unless the
> VIRTIO_PCI_F_SEPARATE_CONFIG feature is acknowledged. This feature
> acts as a latch and when set, removes the config space overlay.
>
> If the config space overlays the registers, the offset in BAR0 of the
> overlay depends on whether MSI is enabled or not in the PCI device.
>
> BAR1 is an MMIO mirror of BAR0 except that the config space is never
> overlayed in BAR1 regardless of VIRTIO_PCI_F_SEPARATE_CONFIG.
Creating a horrible mess for new code in order to support old code.
But I have to maintain the new code, and this is horrible.
> You mean, "accesses to bar1/2/3" change it to modern mode. That's a
> pretty big side effect of a read.
They don't need to, but I'd prefer they did to keep implementations
from mixing old and new.
> See above. A guest could happily just use BAR1/BAR2 and completely
> ignore BAR0 provided that BAR1/BAR2 are present.
But x86 guests want BAR0 because IO space is faster than MMIO. Right?
So, not "happily".
> It also means the guest drivers don't need separate code paths for
> "legacy" vs. "modern". They can switch between the two by just changing
> a single pointer.
This is wrong. Of course they have separate code paths, but now you've
got lots of crossover, rather than two distinct ones.
> The implementation ends up being pretty trivial too. We can use the
> same MemoryRegion in QEMU for both PIO and MMIO. The kernel code stays
> the same. It just takes a couple if()s to handle whether there's a
> config overlay or not.
"A couple of ifs" for every feature, and they nest. You end up with
exponential complexity and an untestable mess. That's why this change
is an opportunity.
Not a feature bit for 64 bit device offsets, and another for extended
features and another for size negotiation and another for alignment
setting. That would be a maintenance nightmare for no good reason.
Cheers,
Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-10 2:54 UTC (permalink / raw)
To: Gerd Hoffmann, Anthony Liguori
Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <507487F0.2050609@redhat.com>
Gerd Hoffmann <kraxel@redhat.com> writes:
> So how about this:
>
> (1) Add a vendor specific pci capability for new-style virtio.
> Specifies the pci bar used for new-style virtio registers.
> Guests can use it to figure whenever new-style virtio is
> supported and to map the correct bar (which will probably
> be bar 1 in most cases).
This was closer to the original proposal[1], which I really liked (you
can layout bars however you want). Anthony thought that vendor
capabilities were a PCI-e feature, but it seems they're blessed in PCI
2.3.
So let's return to that proposal, giving something like this:
/* IDs for different capabilities. Must all exist. */
/* FIXME: Do we win from separating ISR, NOTIFY and COMMON? */
/* Common configuration */
#define VIRTIO_PCI_CAP_COMMON_CFG 1
/* Notifications */
#define VIRTIO_PCI_CAP_NOTIFY_CFG 2
/* ISR access */
#define VIRTIO_PCI_CAP_ISR_CFG 3
/* Device specific confiuration */
#define VIRTIO_PCI_CAP_DEVICE_CFG 4
/* This is the PCI capability header: */
struct virtio_pci_cap {
u8 cap_vndr; /* Generic PCI field: PCI_CAP_ID_VNDR */
u8 cap_next; /* Generic PCI field: next ptr. */
u8 cap_len; /* Generic PCI field: sizeof(struct virtio_pci_cap). */
u8 cfg_type; /* One of the VIRTIO_PCI_CAP_*_CFG. */
u8 bar; /* Where to find it. */
u8 unused;
__le16 offset; /* Offset within bar. */
__le32 length; /* Length. */
};
This means qemu can point the isr_cfg into the legacy area if it wants.
In fact, it can put everything in BAR0 if it wants.
Thoughts?
Rusty.
^ permalink raw reply
* Re: [PATCH] vhost-blk: Add vhost-blk support v2
From: Asias He @ 2012-10-10 2:27 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <20121009173937.GA28399@infradead.org>
Hello Christoph!
On 10/10/2012 01:39 AM, Christoph Hellwig wrote:
>> +static int vhost_blk_req_submit(struct vhost_blk_req *req, struct file *file)
>> +{
>> +
>> + struct inode *inode = file->f_mapping->host;
>> + struct block_device *bdev = inode->i_bdev;
>> + int ret;
>
> Please just pass the block_device directly instead of a file struct.
vhost_blk_req_submit() can be used to handle file based image later.
Using the file interface will work for both cases.
I do need a check in vhost_blk_set_backend() to tell if the user passed
file is a raw device file for now.
>> +
>> + ret = vhost_blk_bio_make(req, bdev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + vhost_blk_bio_send(req);
>> +
>> + return ret;
>> +}
>
> Then again how simple the this function is it probably should just go
> away entirely.
No, vhost_blk_req_submit() is used for both read and write ops. It makes
no sense to write the code twice. Plus, this function might be complexer
when the file based image support is added.
>> + set_fs(USER_DS);
>
> What do we actually need the set_fs for here?
See this commit: d7ffde35e31a81100d2d0d2c4013cbf527bb32ea
>> +
>> +static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
>> +{
>> +
>> + *file = vhost_blk_stop_vq(blk, &blk->vq);
>> +}
>
> What is the point of this helper? Also I can't see anyone actually
> using the returned struct file.
It is used in both vhost_blk_reset_owner() and vhost_blk_release(). The
returned struct file is used for fput(). We have similar helper in
vhost_net: vhost_net_stop().
>> + case VIRTIO_BLK_T_FLUSH:
>> + ret = vfs_fsync(file, 1);
>> + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
>> + if (!vhost_blk_set_status(req, status))
>> + vhost_add_used_and_signal(&blk->dev, vq, head, ret);
>> + break;
>
> Sending a fsync here is actually wrong in two different ways:
>
> a) it operates at the filesystem level instead of bio level
> b) it's a blocking operation
>
> It should instead send a REQ_FLUSH bio using the same submission scheme
> as the read/write requests.
Will fix this.
Thanks for the review!
--
Asias
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Jamie Lokier @ 2012-10-09 21:09 UTC (permalink / raw)
To: Rusty Russell
Cc: Anthony Liguori, kvm, Michael S. Tsirkin, qemu-devel,
virtualization
In-Reply-To: <87haq48hds.fsf@rustcorp.com.au>
Rusty Russell wrote:
> I don't think it'll be that bad; reset clears the device to unknown,
> bar0 moves it from unknown->legacy mode, bar1/2/3 changes it from
> unknown->modern mode, and anything else is bad (I prefer being strict so
> we catch bad implementations from the beginning).
Will that work, if the guest with kernel that uses modern mode, kexecs
to an older (but presumed reliable) kernel that only knows about legacy mode?
I.e. will the replacement kernel, or (ideally) replacement driver on
the rare occasion that is needed on a running kernel, be able to reset
the device hard enough?
-- Jamie
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Gerd Hoffmann @ 2012-10-09 20:24 UTC (permalink / raw)
To: Anthony Liguori; +Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <87wqyzll8r.fsf@codemonkey.ws>
Hi,
>> Why use two bars for this? You can put them into one mmio bar, together
>> with the msi-x vector table and PBA. Of course a pci capability
>> describing the location is helpful for that ;)
>
> You don't need a capability. You can also just add a "config offset"
> field to the register set and then make the semantics that it occurs in
> the same region.
Yes, that will work too. Removes some freedom to place the register
ranges, but given that we don't want burn bars and thus prefer to place
everything into a single mmio bar that shouldn't be an issue. Real
hardware does this too btw (xhci for example).
>> Main advantage of defining a register set with just isr is that it
>> reduces pio address space consumtion for new virtio devices which don't
>> have to worry about the legacy layout (8 bytes which is minimum size for
>> io bars instead of 64 bytes).
>
> Doing some rough math, we should have at least 16k of PIO space. That
> let's us have well over 500 virtio-pci devices with the current register
> layout.
I've seen worries nevertheless, but given we have virtio-scsi now which
can handle lots of disks without needing lots of virtio-pci devices it
is probably not that a big issue any more.
>>>> The detection is simple: if BAR1 has non-zero length, it's new-style,
>>>> otherwise legacy.
>>
>> Doesn't fly. BAR1 is in use today for MSI-X support.
>
> But the location is specified via capabilities so we can change the
> location to be within BAR1 at a non-conflicting offset.
Sure. Nevertheless "BAR1 has non-zero length" can't be used to detect
new-style virtio as old-style devices already have BAR1 with a non-zero
length.
So how about this:
(1) Add a vendor specific pci capability for new-style virtio.
Specifies the pci bar used for new-style virtio registers.
Guests can use it to figure whenever new-style virtio is
supported and to map the correct bar (which will probably
be bar 1 in most cases).
(2) Have virtio-offsets register set, located at the new-style bar,
offset 0:
struct virtio_offsets {
__le32 num_offsets; // so we can extend this later
__le32 virtio_pci_offset; // location of virtio-pci registers
__le32 virtio_cfg_offset; // location of virtio config space
};
(3) place virtio-pci registers (same as 0..23 in bar 0) in the new-style
bar, offset virtio_pci_offset
(4) place virtio config space (same as 20+/24+ in bar 0) in the
new-style bar, offset virtio_cfg_offset
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH] vhost-blk: Add vhost-blk support v2
From: Christoph Hellwig @ 2012-10-09 17:39 UTC (permalink / raw)
To: Asias He; +Cc: virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <1349769918-8611-1-git-send-email-asias@redhat.com>
> +static int vhost_blk_req_submit(struct vhost_blk_req *req, struct file *file)
> +{
> +
> + struct inode *inode = file->f_mapping->host;
> + struct block_device *bdev = inode->i_bdev;
> + int ret;
Please just pass the block_device directly instead of a file struct.
> +
> + ret = vhost_blk_bio_make(req, bdev);
> + if (ret < 0)
> + return ret;
> +
> + vhost_blk_bio_send(req);
> +
> + return ret;
> +}
Then again how simple the this function is it probably should just go
away entirely.
> + set_fs(USER_DS);
What do we actually need the set_fs for here?
> +
> +static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
> +{
> +
> + *file = vhost_blk_stop_vq(blk, &blk->vq);
> +}
What is the point of this helper? Also I can't see anyone actually
using the returned struct file.
> + case VIRTIO_BLK_T_FLUSH:
> + ret = vfs_fsync(file, 1);
> + status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> + if (!vhost_blk_set_status(req, status))
> + vhost_add_used_and_signal(&blk->dev, vq, head, ret);
> + break;
Sending a fsync here is actually wrong in two different ways:
a) it operates at the filesystem level instead of bio level
b) it's a blocking operation
It should instead send a REQ_FLUSH bio using the same submission scheme
as the read/write requests.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Anthony Liguori @ 2012-10-09 15:26 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <5073C52D.20802@redhat.com>
Gerd Hoffmann <kraxel@redhat.com> writes:
> Hi,
>
>>> Well, we also want to clean up the registers, so how about:
>>>
>>> BAR0: legacy, as is. If you access this, don't use the others.
>
> Ok.
>
>>> BAR1: new format virtio-pci layout. If you use this, don't use BAR0.
>>> BAR2: virtio-cfg. If you use this, don't use BAR0.
>
> Why use two bars for this? You can put them into one mmio bar, together
> with the msi-x vector table and PBA. Of course a pci capability
> describing the location is helpful for that ;)
You don't need a capability. You can also just add a "config offset"
field to the register set and then make the semantics that it occurs in
the same region.
>
>>> BAR3: ISR. If you use this, don't use BAR0.
>
> Again, I wouldn't hardcode that but use a capability.
>
>>> I prefer the cases exclusive (ie. use one or the other) as a clear path
>>> to remove the legacy layout; and leaving the ISR in BAR0 leaves us with
>>> an ugly corner case in future (ISR is BAR0 + 19? WTF?).
>
> Ok, so we have four register sets:
>
> (1) legacy layout
> (2) new virtio-pci
> (3) new virtio-config
> (4) new virtio-isr
>
> We can have a vendor pci capability, with a dword for each register set:
>
> bit 31 -- present bit
> bits 26-24 -- bar
> bits 23-0 -- offset
>
> So current drivers which must support legacy can use this:
>
> legacy layout -- present, bar 0, offset 0
> new virtio-pci -- present, bar 1, offset 0
> new virtio-config -- present, bar 1, offset 256
> new virtio-isr -- present, bar 0, offset 19
>
> [ For completeness: msi-x capability could add this: ]
>
> msi-x vector table bar 1, offset 512
> msi-x pba bar 1, offset 768
>
>> We'll never remove legacy so we shouldn't plan on it. There are
>> literally hundreds of thousands of VMs out there with the current virtio
>> drivers installed in them. We'll be supporting them for a very, very
>> long time :-)
>
> But new devices (virtio-qxl being a candidate) don't have old guests and
> don't need to worry.
>
> They could use this if they care about fast isr:
>
> legacy layout -- not present
> new virtio-pci -- present, bar 1, offset 0
> new virtio-config -- present, bar 1, offset 256
> new virtio-isr -- present, bar 0, offset 0
>
> Or this if they don't worry about isr performance:
>
> legacy layout -- not present
> new virtio-pci -- present, bar 0, offset 0
> new virtio-config -- present, bar 0, offset 256
> new virtio-isr -- not present
>
>> I don't think we gain a lot by moving the ISR into a separate BAR.
>> Splitting up registers like that seems weird to me too.
>
> Main advantage of defining a register set with just isr is that it
> reduces pio address space consumtion for new virtio devices which don't
> have to worry about the legacy layout (8 bytes which is minimum size for
> io bars instead of 64 bytes).
Doing some rough math, we should have at least 16k of PIO space. That
let's us have well over 500 virtio-pci devices with the current register
layout.
I don't think we're at risk of running out of space...
>> If we added an additional constraints that BAR1 was mirrored except for
>
> Why add constraints? We want something future-proof, don't we?
>
>>> The detection is simple: if BAR1 has non-zero length, it's new-style,
>>> otherwise legacy.
>
> Doesn't fly. BAR1 is in use today for MSI-X support.
But the location is specified via capabilities so we can change the
location to be within BAR1 at a non-conflicting offset.
>> I agree that this is the best way to extend, but I think we should still
>> use a transport feature bit. We want to be able to detect within QEMU
>> whether a guest is using these new features because we need to adjust
>> migration state accordingly.
>
> Why does migration need adjustments?
Because there is additional state in the "new" layout. We need to
understand whether a guest is relying on that state or not.
For instance, extended virtio features. If a guest is in the process
of reading extended virtio features, it may not have changed any state
but we must ensure that we don't migrate to an older verison of QEMU w/o
the extended virtio features.
This cannot be handled by subsections today because there is no guest
written state that's affected.
Regards,
Anthony Liguori
>
> [ Not that I want veto a feature bit, but I don't see the need yet ]
>
> 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: Anthony Liguori @ 2012-10-09 14:03 UTC (permalink / raw)
To: Avi Kivity, Rusty Russell
Cc: qemu-devel, virtualization, kvm, Michael S. Tsirkin
In-Reply-To: <5073F9D4.5060000@redhat.com>
Avi Kivity <avi@redhat.com> writes:
> On 10/09/2012 05:16 AM, Rusty Russell wrote:
>> Anthony Liguori <aliguori@us.ibm.com> writes:
>>> We'll never remove legacy so we shouldn't plan on it. There are
>>> literally hundreds of thousands of VMs out there with the current virtio
>>> drivers installed in them. We'll be supporting them for a very, very
>>> long time :-)
>>
>> You will be supporting this for qemu on x86, sure. As I think we're
>> still in the growth phase for virtio, I prioritize future spec
>> cleanliness pretty high.
>
> If a pure ppc hypervisor was on the table, this might have been
> worthwhile. As it is the codebase is shared, and the Linux drivers are
> shared, so cleaning up the spec doesn't help the code.
Note that distros have been (perhaps unknowingly) shipping virtio-pci
for PPC for some time now.
So even though there wasn't a hypervisor that supported virtio-pci, the
guests already support it and are out there in the wild.
There's a lot of value in maintaining "legacy" support even for PPC.
>> But I think you'll be surprised how fast this is deprecated:
>> 1) Bigger queues for block devices (guest-specified ringsize)
>> 2) Smaller rings for openbios (guest-specified alignment)
>> 3) All-mmio mode (powerpc)
>> 4) Whatever network features get numbers > 31.
>>
>>> I don't think we gain a lot by moving the ISR into a separate BAR.
>>> Splitting up registers like that seems weird to me too.
>>
>> Confused. I proposed the same split as you have, just ISR by itself.
>
> I believe Anthony objects to having the ISR by itself. What is the
> motivation for that?
Right, BARs are a precious resource not to be spent lightly. Having an
entire BAR dedicated to a 1-byte register seems like a waste to me.
Regards,
Anthony Liguori
>
>
> --
> error compiling committee.c: too many arguments to function
> --
> 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: Proposal for virtio standardization.
From: Cornelia Huck @ 2012-10-09 14:02 UTC (permalink / raw)
To: Rusty Russell
Cc: Anthony Liguori, Avishay Traeger, kvm, Pawel Moll,
Michael S. Tsirkin, qemu-devel, LKML, virtualization, Avi Kivity,
Amit Shah, Paolo Bonzini, Sasha Levin
In-Reply-To: <87zk4c2tqq.fsf@rustcorp.com.au>
On Thu, 27 Sep 2012 09:59:33 +0930
Rusty Russell <rusty@rustcorp.com.au> wrote:
> 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)
"Infinite" only applies to virtio-pci, no?
>
> 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.
>
Sounds like a good idea. I'll be happy to review the spec with an eye
to virtio-ccw.
Cornelia
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Anthony Liguori @ 2012-10-09 13:56 UTC (permalink / raw)
To: Rusty Russell, Gerd Hoffmann
Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <87haq48hds.fsf@rustcorp.com.au>
Rusty Russell <rusty@rustcorp.com.au> writes:
> Anthony Liguori <aliguori@us.ibm.com> writes:
>> We'll never remove legacy so we shouldn't plan on it. There are
>> literally hundreds of thousands of VMs out there with the current virtio
>> drivers installed in them. We'll be supporting them for a very, very
>> long time :-)
>
> You will be supporting this for qemu on x86, sure.
And PPC.
> As I think we're
> still in the growth phase for virtio, I prioritize future spec
> cleanliness pretty high.
>
> But I think you'll be surprised how fast this is deprecated:
> 1) Bigger queues for block devices (guest-specified ringsize)
> 2) Smaller rings for openbios (guest-specified alignment)
> 3) All-mmio mode (powerpc)
> 4) Whatever network features get numbers > 31.
We can do all of these things with incremental change to the existing
layout. That's the only way what I'm suggesting is different.
You want to reorder all of the fields and have a driver flag day. But I
strongly suspect we'll decide we need to do the same exercise again in 4
years when we now need to figure out how to take advantage of
transactional memory or some other whiz-bang hardware feature.
There are a finite number of BARs but each BAR has an almost infinite
size. So extending BARs instead of introducing new one seems like the
conservative approach moving forward.
>> I don't think we gain a lot by moving the ISR into a separate BAR.
>> Splitting up registers like that seems weird to me too.
>
> Confused. I proposed the same split as you have, just ISR by itself.
I disagree with moving the ISR into a separate BAR. That's what seems
weird to me.
>> It's very normal to have a mirrored set of registers that are PIO in one
>> bar and MMIO in a different BAR.
>>
>> If we added an additional constraints that BAR1 was mirrored except for
>> the config space and the MSI section was always there, I think the end
>> result would be nice. IOW:
>
> But it won't be the same, because we want all that extra stuff, like
> more feature bits and queue size alignment. (Admittedly queues past
> 16TB aren't a killer feature).
>
> To make it concrete:
>
> Current:
> struct {
> __le32 host_features; /* read-only */
> __le32 guest_features; /* read/write */
> __le32 queue_pfn; /* read/write */
> __le16 queue_size; /* read-only */
> __le16 queue_sel; /* read/write */
> __le16 queue_notify; /* read/write */
> u8 status; /* read/write */
> u8 isr; /* read-only, clear on read */
> /* Optional */
> __le16 msi_config_vector; /* read/write */
> __le16 msi_queue_vector; /* read/write */
> /* ... device features */
> };
>
> Proposed:
> struct virtio_pci_cfg {
> /* About the whole device. */
> __le32 device_feature_select; /* read-write */
> __le32 device_feature; /* read-only */
> __le32 guest_feature_select; /* read-write */
> __le32 guest_feature; /* read-only */
> __le16 msix_config; /* read-write */
> __u8 device_status; /* read-write */
> __u8 unused;
>
> /* About a specific virtqueue. */
> __le16 queue_select; /* read-write */
> __le16 queue_align; /* read-write, power of 2. */
> __le16 queue_size; /* read-write, power of 2. */
> __le16 queue_msix_vector;/* read-write */
> __le64 queue_address; /* read-write: 0xFFFFFFFFFFFFFFFF == DNE. */
> };
>
> struct virtio_pci_isr {
> __u8 isr; /* read-only, clear on read */
> };
What I'm suggesting is:
> struct {
> __le32 host_features; /* read-only */
> __le32 guest_features; /* read/write */
> __le32 queue_pfn; /* read/write */
> __le16 queue_size; /* read-only */
> __le16 queue_sel; /* read/write */
> __le16 queue_notify; /* read/write */
> u8 status; /* read/write */
> u8 isr; /* read-only, clear on read */
> __le16 msi_config_vector; /* read/write */
> __le16 msi_queue_vector; /* read/write */
> __le32 host_feature_select; /* read/write */
> __le32 guest_feature_select; /* read/write */
> __le32 queue_pfn_hi; /* read/write */
> };
>
With the additional semantic that the virtio-config space is overlayed
on top of the register set in BAR0 unless the
VIRTIO_PCI_F_SEPARATE_CONFIG feature is acknowledged. This feature
acts as a latch and when set, removes the config space overlay.
If the config space overlays the registers, the offset in BAR0 of the
overlay depends on whether MSI is enabled or not in the PCI device.
BAR1 is an MMIO mirror of BAR0 except that the config space is never
overlayed in BAR1 regardless of VIRTIO_PCI_F_SEPARATE_CONFIG.
BAR2 contains the config space.
A guest can look at BAR1 and BAR2 to determine whether they exist.
> We could also enforce LE in the per-device config space in this case,
> another nice cleanup for PCI people.
>
>> BAR0[pio]: virtio-pci registers + optional MSI section + virtio-config
>> BAR1[mmio]: virtio-pci registers + MSI section + future extensions
>> BAR2[mmio]: virtio-config
>>
>> We can continue to do ISR access via BAR0 for performance reasons.
>
> But powerpc explicitly *doesnt* want a pio bar. So let it be its own
> bar, which can be either.
Does it really care? It all ends up being MMIO for PPC anyway so I
don't think there's any real pressing need for it not to be PIO.
>>> As to MMIO vs PIO, the BARs are self-describing, so we should explicitly
>>> endorse that and leave it to the devices.
>>>
>>> The detection is simple: if BAR1 has non-zero length, it's new-style,
>>> otherwise legacy.
>>
>> I agree that this is the best way to extend, but I think we should still
>> use a transport feature bit. We want to be able to detect within QEMU
>> whether a guest is using these new features because we need to adjust
>> migration state accordingly.
>>
>> Otherwise we would have to detect reads/writes to the new BARs to
>> maintain whether the extended register state needs to be saved. This
>> gets nasty dealing with things like reset.
>
> I don't think it'll be that bad; reset clears the device to unknown,
> bar0 moves it from unknown->legacy mode, bar1/2/3 changes it from
> unknown->modern mode, and anything else is bad (I prefer being strict so
> we catch bad implementations from the beginning).
You mean, "accesses to bar1/2/3" change it to modern mode. That's a
pretty big side effect of a read.
> But I'm happy to implement it and see what it's like.
>
>> A feature bit simplifies this all pretty well.
>
> I suspect it will be quite ugly, actually. The guest has to use BAR0 to
> get the features to see if they can use BAR1. Do they ack the feature
> (via BAR0) before accessing BAR1? If not, qemu can't rely on the
> feature bit.
See above. A guest could happily just use BAR1/BAR2 and completely
ignore BAR0 provided that BAR1/BAR2 are present.
It also means the guest drivers don't need separate code paths for
"legacy" vs. "modern". They can switch between the two by just changing
a single pointer.
The implementation ends up being pretty trivial too. We can use the
same MemoryRegion in QEMU for both PIO and MMIO. The kernel code stays
the same. It just takes a couple if()s to handle whether there's a
config overlay or not.
Regards,
Anthony Liguori
>
> Cheers,
> Rusty.
> --
> 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: Avi Kivity @ 2012-10-09 10:17 UTC (permalink / raw)
To: Rusty Russell
Cc: Anthony Liguori, kvm, Michael S. Tsirkin, qemu-devel,
virtualization
In-Reply-To: <87haq48hds.fsf@rustcorp.com.au>
On 10/09/2012 05:16 AM, Rusty Russell wrote:
> Anthony Liguori <aliguori@us.ibm.com> writes:
>> We'll never remove legacy so we shouldn't plan on it. There are
>> literally hundreds of thousands of VMs out there with the current virtio
>> drivers installed in them. We'll be supporting them for a very, very
>> long time :-)
>
> You will be supporting this for qemu on x86, sure. As I think we're
> still in the growth phase for virtio, I prioritize future spec
> cleanliness pretty high.
If a pure ppc hypervisor was on the table, this might have been
worthwhile. As it is the codebase is shared, and the Linux drivers are
shared, so cleaning up the spec doesn't help the code.
>
> But I think you'll be surprised how fast this is deprecated:
> 1) Bigger queues for block devices (guest-specified ringsize)
> 2) Smaller rings for openbios (guest-specified alignment)
> 3) All-mmio mode (powerpc)
> 4) Whatever network features get numbers > 31.
>
>> I don't think we gain a lot by moving the ISR into a separate BAR.
>> Splitting up registers like that seems weird to me too.
>
> Confused. I proposed the same split as you have, just ISR by itself.
I believe Anthony objects to having the ISR by itself. What is the
motivation for that?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* [PATCH] vhost-blk: Add vhost-blk support v2
From: Asias He @ 2012-10-09 8:05 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization
vhost-blk is an in-kernel virito-blk device accelerator.
Due to lack of proper in-kernel AIO interface, this version converts
guest's I/O request to bio and use submit_bio() to submit I/O directly.
So this version any supports raw block device as guest's disk image,
e.g. /dev/sda, /dev/ram0. We can add file based image support to
vhost-blk once we have in-kernel AIO interface. There are some work in
progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown:
http://marc.info/?l=linux-fsdevel&m=133312234313122
Performance evaluation:
-----------------------------
1) LKVM
Fio with libaio ioengine on Fusion IO device using kvm tool
IOPS Before After Improvement
seq-read 107 121 +13.0%
seq-write 130 179 +37.6%
rnd-read 102 122 +19.6%
rnd-write 125 159 +27.0%
2) QEMU
Fio with libaio ioengine on Fusion IO device using QEMU
IOPS Before After Improvement
seq-read 76 123 +61.8%
seq-write 139 173 +24.4%
rnd-read 73 120 +64.3%
rnd-write 75 156 +108.0%
Userspace bits:
-----------------------------
1) LKVM
The latest vhost-blk userspace bits for kvm tool can be found here:
git@github.com:asias/linux-kvm.git blk.vhost-blk
2) QEMU
The latest vhost-blk userspace prototype for QEMU can be found here:
git@github.com:asias/qemu.git blk.vhost-blk
Signed-off-by: Asias He <asias@redhat.com>
---
drivers/vhost/Kconfig | 1 +
drivers/vhost/Kconfig.blk | 10 +
drivers/vhost/Makefile | 2 +
drivers/vhost/blk.c | 641 ++++++++++++++++++++++++++++++++++++++++++++++
drivers/vhost/blk.h | 8 +
5 files changed, 662 insertions(+)
create mode 100644 drivers/vhost/Kconfig.blk
create mode 100644 drivers/vhost/blk.c
create mode 100644 drivers/vhost/blk.h
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 202bba6..acd8038 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -11,4 +11,5 @@ config VHOST_NET
if STAGING
source "drivers/vhost/Kconfig.tcm"
+source "drivers/vhost/Kconfig.blk"
endif
diff --git a/drivers/vhost/Kconfig.blk b/drivers/vhost/Kconfig.blk
new file mode 100644
index 0000000..ff8ab76
--- /dev/null
+++ b/drivers/vhost/Kconfig.blk
@@ -0,0 +1,10 @@
+config VHOST_BLK
+ tristate "Host kernel accelerator for virtio blk (EXPERIMENTAL)"
+ depends on BLOCK && EXPERIMENTAL && m
+ ---help---
+ This kernel module can be loaded in host kernel to accelerate
+ guest block with virtio_blk. Not to be confused with virtio_blk
+ module itself which needs to be loaded in guest kernel.
+
+ To compile this driver as a module, choose M here: the module will
+ be called vhost_blk.
diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
index a27b053..1a8a4a5 100644
--- a/drivers/vhost/Makefile
+++ b/drivers/vhost/Makefile
@@ -2,3 +2,5 @@ obj-$(CONFIG_VHOST_NET) += vhost_net.o
vhost_net-y := vhost.o net.o
obj-$(CONFIG_TCM_VHOST) += tcm_vhost.o
+obj-$(CONFIG_VHOST_BLK) += vhost_blk.o
+vhost_blk-y := blk.o
diff --git a/drivers/vhost/blk.c b/drivers/vhost/blk.c
new file mode 100644
index 0000000..6b2445a
--- /dev/null
+++ b/drivers/vhost/blk.c
@@ -0,0 +1,641 @@
+/*
+ * Copyright (C) 2011 Taobao, Inc.
+ * Author: Liu Yuan <tailai.ly@taobao.com>
+ *
+ * Copyright (C) 2012 Red Hat, Inc.
+ * Author: Asias He <asias@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ *
+ * virtio-blk server in host kernel.
+ */
+
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/vhost.h>
+#include <linux/virtio_blk.h>
+#include <linux/mutex.h>
+#include <linux/file.h>
+#include <linux/kthread.h>
+#include <linux/blkdev.h>
+
+#include "vhost.c"
+#include "vhost.h"
+#include "blk.h"
+
+#define BLK_HDR 0
+
+static DEFINE_IDA(vhost_blk_index_ida);
+
+enum {
+ VHOST_BLK_VQ_REQ = 0,
+ VHOST_BLK_VQ_MAX = 1,
+};
+
+struct req_page_list {
+ struct page **pages;
+ int pages_nr;
+};
+
+struct vhost_blk_req {
+ struct llist_node llnode;
+ struct req_page_list *pl;
+ struct vhost_blk *blk;
+
+ struct iovec *iov;
+ int iov_nr;
+
+ struct bio **bio;
+ atomic_t bio_nr;
+
+ sector_t sector;
+ int write;
+ u16 head;
+ long len;
+
+ u8 *status;
+};
+
+struct vhost_blk {
+ struct task_struct *host_kick;
+ struct vhost_blk_req *reqs;
+ struct vhost_virtqueue vq;
+ struct llist_head llhead;
+ struct vhost_dev dev;
+ u16 reqs_nr;
+ int index;
+};
+
+static inline int iov_num_pages(struct iovec *iov)
+{
+ return (PAGE_ALIGN((unsigned long)iov->iov_base + iov->iov_len) -
+ ((unsigned long)iov->iov_base & PAGE_MASK)) >> PAGE_SHIFT;
+}
+
+static int vhost_blk_setup(struct vhost_blk *blk)
+{
+ blk->reqs_nr = blk->vq.num;
+
+ blk->reqs = kmalloc(sizeof(struct vhost_blk_req) * blk->reqs_nr,
+ GFP_KERNEL);
+ if (!blk->reqs)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static inline int vhost_blk_set_status(struct vhost_blk_req *req, u8 status)
+{
+ struct vhost_blk *blk = req->blk;
+
+ if (copy_to_user(req->status, &status, sizeof(status))) {
+ vq_err(&blk->vq, "Failed to write status\n");
+ vhost_discard_vq_desc(&blk->vq, 1);
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+static void vhost_blk_enable_vq(struct vhost_blk *blk,
+ struct vhost_virtqueue *vq)
+{
+ wake_up_process(blk->host_kick);
+}
+
+static void vhost_blk_req_done(struct bio *bio, int err)
+{
+ struct vhost_blk_req *req = bio->bi_private;
+ struct vhost_blk *blk = req->blk;
+
+ if (err)
+ req->len = err;
+
+ if (atomic_dec_and_test(&req->bio_nr)) {
+ llist_add(&req->llnode, &blk->llhead);
+ wake_up_process(blk->host_kick);
+ }
+
+ bio_put(bio);
+}
+
+static void vhost_blk_req_umap(struct vhost_blk_req *req)
+{
+ struct req_page_list *pl;
+ int i, j;
+
+ if (!req->pl)
+ return;
+
+ for (i = 0; i < req->iov_nr; i++) {
+ pl = &req->pl[i];
+ for (j = 0; j < pl->pages_nr; j++) {
+ if (!req->write)
+ set_page_dirty_lock(pl->pages[j]);
+ page_cache_release(pl->pages[j]);
+ }
+ }
+
+ kfree(req->pl);
+}
+
+static int vhost_blk_bio_make(struct vhost_blk_req *req,
+ struct block_device *bdev)
+{
+ int pages_nr_total, i, j, ret;
+ struct iovec *iov = req->iov;
+ int iov_nr = req->iov_nr;
+ struct page **pages, *page;
+ struct bio *bio = NULL;
+ int bio_nr = 0;
+
+ req->len = 0;
+ pages_nr_total = 0;
+ for (i = 0; i < iov_nr; i++) {
+ req->len += iov[i].iov_len;
+ pages_nr_total += iov_num_pages(&iov[i]);
+ }
+
+ req->pl = kmalloc((iov_nr * sizeof(struct req_page_list)) +
+ (pages_nr_total * sizeof(struct page *)) +
+ (pages_nr_total * sizeof(struct bio *)),
+ GFP_KERNEL);
+ if (!req->pl)
+ return -ENOMEM;
+ pages = (struct page **)&req->pl[iov_nr];
+ req->bio = (struct bio **)&pages[pages_nr_total];
+
+ req->iov_nr = 0;
+ for (i = 0; i < iov_nr; i++) {
+ int pages_nr = iov_num_pages(&iov[i]);
+ unsigned long iov_base, iov_len;
+ struct req_page_list *pl;
+
+ iov_base = (unsigned long)iov[i].iov_base;
+ iov_len = (unsigned long)iov[i].iov_len;
+
+ ret = get_user_pages_fast(iov_base, pages_nr,
+ !req->write, pages);
+ if (ret != pages_nr)
+ goto fail;
+
+ req->iov_nr++;
+ pl = &req->pl[i];
+ pl->pages_nr = pages_nr;
+ pl->pages = pages;
+
+ for (j = 0; j < pages_nr; j++) {
+ unsigned int off, len;
+ page = pages[j];
+ off = iov_base & ~PAGE_MASK;
+ len = PAGE_SIZE - off;
+ if (len > iov_len)
+ len = iov_len;
+
+ while (!bio || bio_add_page(bio, page, len, off) <= 0) {
+ bio = bio_alloc(GFP_KERNEL, pages_nr);
+ if (!bio)
+ goto fail;
+ bio->bi_sector = req->sector;
+ bio->bi_bdev = bdev;
+ bio->bi_private = req;
+ bio->bi_end_io = vhost_blk_req_done;
+ req->bio[bio_nr++] = bio;
+ }
+ req->sector += len >> 9;
+ iov_base += len;
+ iov_len -= len;
+ }
+
+ pages += pages_nr;
+ }
+ atomic_set(&req->bio_nr, bio_nr);
+
+ return 0;
+
+fail:
+ for (i = 0; i < bio_nr; i++)
+ bio_put(req->bio[i]);
+ vhost_blk_req_umap(req);
+ return -ENOMEM;
+}
+
+static inline void vhost_blk_bio_send(struct vhost_blk_req *req)
+{
+ struct blk_plug plug;
+ int i, bio_nr;
+
+ bio_nr = atomic_read(&req->bio_nr);
+ blk_start_plug(&plug);
+ for (i = 0; i < bio_nr; i++)
+ submit_bio(req->write, req->bio[i]);
+ blk_finish_plug(&plug);
+}
+
+static int vhost_blk_req_submit(struct vhost_blk_req *req, struct file *file)
+{
+
+ struct inode *inode = file->f_mapping->host;
+ struct block_device *bdev = inode->i_bdev;
+ int ret;
+
+ ret = vhost_blk_bio_make(req, bdev);
+ if (ret < 0)
+ return ret;
+
+ vhost_blk_bio_send(req);
+
+ return ret;
+}
+
+static int vhost_blk_req_done_thread(void *data)
+{
+ mm_segment_t oldfs = get_fs();
+ struct vhost_blk *blk = data;
+ struct vhost_virtqueue *vq;
+ struct llist_node *llnode;
+ struct vhost_blk_req *req;
+ bool added;
+ u8 status;
+ int ret;
+
+ vq = &blk->vq;
+ set_fs(USER_DS);
+ use_mm(blk->dev.mm);
+ for (;;) {
+ llnode = llist_del_all(&blk->llhead);
+ if (!llnode) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule();
+ if (unlikely(kthread_should_stop()))
+ break;
+ continue;
+ }
+ added = false;
+ while (llnode) {
+ req = llist_entry(llnode, struct vhost_blk_req, llnode);
+ llnode = llist_next(llnode);
+
+ vhost_blk_req_umap(req);
+
+ status = req->len > 0 ?
+ VIRTIO_BLK_S_OK : VIRTIO_BLK_S_IOERR;
+ ret = copy_to_user(req->status, &status,
+ sizeof(status));
+ if (unlikely(ret)) {
+ vq_err(&blk->vq, "Failed to write status\n");
+ return -1;
+ }
+ vhost_add_used(&blk->vq, req->head, req->len);
+ added = true;
+ }
+ if (likely(added))
+ vhost_signal(&blk->dev, &blk->vq);
+
+ }
+ unuse_mm(blk->dev.mm);
+ set_fs(oldfs);
+ return 0;
+}
+
+static void vhost_blk_flush(struct vhost_blk *blk)
+{
+ vhost_poll_flush(&blk->vq.poll);
+}
+
+static struct file *vhost_blk_stop_vq(struct vhost_blk *blk,
+ struct vhost_virtqueue *vq)
+{
+ struct file *file;
+
+ mutex_lock(&vq->mutex);
+ file = rcu_dereference_protected(vq->private_data,
+ lockdep_is_held(&vq->mutex));
+ rcu_assign_pointer(vq->private_data, NULL);
+ mutex_unlock(&vq->mutex);
+
+ return file;
+
+}
+
+static inline void vhost_blk_stop(struct vhost_blk *blk, struct file **file)
+{
+
+ *file = vhost_blk_stop_vq(blk, &blk->vq);
+}
+
+/* Handle guest request */
+static int vhost_blk_req_handle(struct vhost_virtqueue *vq,
+ struct virtio_blk_outhdr *hdr,
+ u16 head, u16 out, u16 in,
+ struct file *file)
+{
+ struct vhost_blk *blk = container_of(vq->dev, struct vhost_blk, dev);
+ struct vhost_blk_req *req;
+ int iov_nr, ret;
+ u8 status;
+
+ if (hdr->type == VIRTIO_BLK_T_IN || hdr->type == VIRTIO_BLK_T_GET_ID)
+ iov_nr = in - 1;
+ else
+ iov_nr = out - 1;
+
+ req = &blk->reqs[head];
+ req->head = head;
+ req->status = blk->vq.iov[iov_nr + 1].iov_base;
+ req->blk = blk;
+ req->iov = &vq->iov[BLK_HDR + 1];
+ req->iov_nr = iov_nr;
+ req->sector = hdr->sector;
+
+ switch (hdr->type) {
+ case VIRTIO_BLK_T_OUT:
+ req->write = WRITE;
+ ret = vhost_blk_req_submit(req, file);
+ break;
+ case VIRTIO_BLK_T_IN:
+ req->write = READ;
+ ret = vhost_blk_req_submit(req, file);
+ break;
+ case VIRTIO_BLK_T_FLUSH:
+ ret = vfs_fsync(file, 1);
+ status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
+ if (!vhost_blk_set_status(req, status))
+ vhost_add_used_and_signal(&blk->dev, vq, head, ret);
+ break;
+ case VIRTIO_BLK_T_GET_ID:
+ ret = snprintf(vq->iov[BLK_HDR + 1].iov_base,
+ VIRTIO_BLK_ID_BYTES, "vhost-blk%d", blk->index);
+ status = ret < 0 ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
+ if (!vhost_blk_set_status(req, status))
+ vhost_add_used_and_signal(&blk->dev, vq, head, ret);
+ break;
+ default:
+ pr_warn("Unsupported request type %d\n", hdr->type);
+ vhost_discard_vq_desc(vq, 1);
+ ret = -EFAULT;
+ break;
+ }
+
+ return ret;
+}
+
+/* Guest kick us for IO request */
+static void vhost_blk_handle_guest_kick(struct vhost_work *work)
+{
+ struct virtio_blk_outhdr hdr;
+ struct vhost_virtqueue *vq;
+ struct vhost_blk *blk;
+ struct blk_plug plug;
+ struct file *f;
+ int in, out;
+ u16 head;
+
+ vq = container_of(work, struct vhost_virtqueue, poll.work);
+ blk = container_of(vq->dev, struct vhost_blk, dev);
+
+ /* TODO: check that we are running from vhost_worker? */
+ f = rcu_dereference_check(vq->private_data, 1);
+ if (!f)
+ return;
+
+ vhost_disable_notify(&blk->dev, vq);
+ blk_start_plug(&plug);
+ for (;;) {
+ head = vhost_get_vq_desc(&blk->dev, vq, vq->iov,
+ ARRAY_SIZE(vq->iov),
+ &out, &in, NULL, NULL);
+ if (unlikely(head < 0))
+ break;
+
+ if (unlikely(head == vq->num)) {
+ if (unlikely(vhost_enable_notify(&blk->dev, vq))) {
+ vhost_disable_notify(&blk->dev, vq);
+ continue;
+ }
+ break;
+ }
+
+ if (unlikely(copy_from_user(&hdr, vq->iov[BLK_HDR].iov_base,
+ sizeof(hdr)))) {
+ vq_err(vq, "Failed to get block header!\n");
+ vhost_discard_vq_desc(vq, 1);
+ break;
+ }
+
+ if (vhost_blk_req_handle(vq, &hdr, head, out, in, f) < 0)
+ break;
+ }
+ blk_finish_plug(&plug);
+}
+
+static int vhost_blk_open(struct inode *inode, struct file *file)
+{
+ struct vhost_blk *blk;
+ int ret;
+
+ blk = kzalloc(sizeof(*blk), GFP_KERNEL);
+ if (!blk) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ret = ida_simple_get(&vhost_blk_index_ida, 0, 0, GFP_KERNEL);
+ if (ret < 0)
+ goto out_dev;
+ blk->index = ret;
+
+ blk->vq.handle_kick = vhost_blk_handle_guest_kick;
+
+ ret = vhost_dev_init(&blk->dev, &blk->vq, VHOST_BLK_VQ_MAX);
+ if (ret < 0)
+ goto out_dev;
+ file->private_data = blk;
+
+ blk->host_kick = kthread_create(vhost_blk_req_done_thread,
+ blk, "vhost-blk-%d", current->pid);
+ if (IS_ERR(blk->host_kick)) {
+ ret = PTR_ERR(blk->host_kick);
+ goto out_dev;
+ }
+
+ return ret;
+out_dev:
+ kfree(blk);
+out:
+ return ret;
+}
+
+static int vhost_blk_release(struct inode *inode, struct file *f)
+{
+ struct vhost_blk *blk = f->private_data;
+ struct file *file;
+
+ ida_simple_remove(&vhost_blk_index_ida, blk->index);
+ vhost_blk_stop(blk, &file);
+ vhost_blk_flush(blk);
+ vhost_dev_cleanup(&blk->dev, false);
+ if (file)
+ fput(file);
+ kthread_stop(blk->host_kick);
+ kfree(blk->reqs);
+ kfree(blk);
+
+ return 0;
+}
+
+static int vhost_blk_set_features(struct vhost_blk *blk, u64 features)
+{
+ mutex_lock(&blk->dev.mutex);
+ blk->dev.acked_features = features;
+ mutex_unlock(&blk->dev.mutex);
+
+ return 0;
+}
+
+static long vhost_blk_set_backend(struct vhost_blk *blk, unsigned index, int fd)
+{
+ struct vhost_virtqueue *vq = &blk->vq;
+ struct file *file, *oldfile;
+ int ret;
+
+ mutex_lock(&blk->dev.mutex);
+ ret = vhost_dev_check_owner(&blk->dev);
+ if (ret)
+ goto out_dev;
+
+ if (index >= VHOST_BLK_VQ_MAX) {
+ ret = -ENOBUFS;
+ goto out_dev;
+ }
+
+ mutex_lock(&vq->mutex);
+
+ if (!vhost_vq_access_ok(vq)) {
+ ret = -EFAULT;
+ goto out_vq;
+ }
+
+ file = fget(fd);
+ if (IS_ERR(file)) {
+ ret = PTR_ERR(file);
+ goto out_vq;
+ }
+
+ oldfile = rcu_dereference_protected(vq->private_data,
+ lockdep_is_held(&vq->mutex));
+ if (file != oldfile) {
+ rcu_assign_pointer(vq->private_data, file);
+ vhost_blk_enable_vq(blk, vq);
+
+ ret = vhost_init_used(vq);
+ if (ret)
+ goto out_vq;
+ }
+
+ mutex_unlock(&vq->mutex);
+
+ if (oldfile) {
+ vhost_blk_flush(blk);
+ fput(oldfile);
+ }
+
+ mutex_unlock(&blk->dev.mutex);
+ return 0;
+
+out_vq:
+ mutex_unlock(&vq->mutex);
+out_dev:
+ mutex_unlock(&blk->dev.mutex);
+ return ret;
+}
+
+static long vhost_blk_reset_owner(struct vhost_blk *blk)
+{
+ struct file *file = NULL;
+ int err;
+
+ mutex_lock(&blk->dev.mutex);
+ err = vhost_dev_check_owner(&blk->dev);
+ if (err)
+ goto done;
+ vhost_blk_stop(blk, &file);
+ vhost_blk_flush(blk);
+ err = vhost_dev_reset_owner(&blk->dev);
+done:
+ mutex_unlock(&blk->dev.mutex);
+ if (file)
+ fput(file);
+ return err;
+}
+
+static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
+ unsigned long arg)
+{
+ struct vhost_blk *blk = f->private_data;
+ void __user *argp = (void __user *)arg;
+ struct vhost_vring_file backend;
+ u64 __user *featurep = argp;
+ u64 features;
+ int ret;
+
+ switch (ioctl) {
+ case VHOST_BLK_SET_BACKEND:
+ if (copy_from_user(&backend, argp, sizeof(backend)))
+ return -EFAULT;
+ return vhost_blk_set_backend(blk, backend.index, backend.fd);
+ case VHOST_GET_FEATURES:
+ features = VHOST_BLK_FEATURES;
+ if (copy_to_user(featurep, &features, sizeof(features)))
+ return -EFAULT;
+ return 0;
+ case VHOST_SET_FEATURES:
+ if (copy_from_user(&features, featurep, sizeof(features)))
+ return -EFAULT;
+ if (features & ~VHOST_BLK_FEATURES)
+ return -EOPNOTSUPP;
+ return vhost_blk_set_features(blk, features);
+ case VHOST_RESET_OWNER:
+ return vhost_blk_reset_owner(blk);
+ default:
+ mutex_lock(&blk->dev.mutex);
+ ret = vhost_dev_ioctl(&blk->dev, ioctl, arg);
+ if (!ret && ioctl == VHOST_SET_VRING_NUM)
+ ret = vhost_blk_setup(blk);
+ vhost_blk_flush(blk);
+ mutex_unlock(&blk->dev.mutex);
+ return ret;
+ }
+}
+
+static const struct file_operations vhost_blk_fops = {
+ .owner = THIS_MODULE,
+ .open = vhost_blk_open,
+ .release = vhost_blk_release,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = vhost_blk_ioctl,
+};
+
+static struct miscdevice vhost_blk_misc = {
+ MISC_DYNAMIC_MINOR,
+ "vhost-blk",
+ &vhost_blk_fops,
+};
+
+int vhost_blk_init(void)
+{
+ return misc_register(&vhost_blk_misc);
+}
+
+void vhost_blk_exit(void)
+{
+ misc_deregister(&vhost_blk_misc);
+}
+
+module_init(vhost_blk_init);
+module_exit(vhost_blk_exit);
+
+MODULE_VERSION("0.0.3");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Asias He");
+MODULE_DESCRIPTION("Host kernel accelerator for virtio_blk");
diff --git a/drivers/vhost/blk.h b/drivers/vhost/blk.h
new file mode 100644
index 0000000..2f674f0
--- /dev/null
+++ b/drivers/vhost/blk.h
@@ -0,0 +1,8 @@
+#include <linux/vhost.h>
+
+enum {
+ VHOST_BLK_FEATURES = (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
+ (1ULL << VIRTIO_RING_F_EVENT_IDX),
+};
+/* VHOST_BLK specific defines */
+#define VHOST_BLK_SET_BACKEND _IOW(VHOST_VIRTIO, 0x50, struct vhost_vring_file)
--
1.7.11.4
^ permalink raw reply related
* Re: [PATCH 0/3] virtio-net: inline header support
From: Paolo Bonzini @ 2012-10-09 7:27 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Sasha Levin, avi, Thomas Lendacky
In-Reply-To: <878vbg8cma.fsf@rustcorp.com.au>
Il 09/10/2012 06:59, Rusty Russell ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> Il 05/10/2012 07:43, Rusty Russell ha scritto:
>>> 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.
>
> Could we do that? It's the cmd length I'm concerned about; is it always
> 32 in practice for some reason?
It is always 32 or less except in very obscure cases that are pretty
much confined to iSCSI. We don't care about the obscure cases, and the
extra bytes don't hurt.
BTW, 32 is the default cdb_size used by virtio-scsi.
> Currently qemu does:
>
> struct sg_io_hdr hdr;
> memset(&hdr, 0, sizeof(struct sg_io_hdr));
> hdr.interface_id = 'S';
> hdr.cmd_len = req->elem.out_sg[1].iov_len;
> hdr.cmdp = req->elem.out_sg[1].iov_base;
> hdr.dxfer_len = 0;
>
> If it's a command which expects more output data, there's no way to
> guess where the boundary is between that command and the data.
Yep, so I understood the problem right.
Paolo
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Gerd Hoffmann @ 2012-10-09 6:33 UTC (permalink / raw)
To: Anthony Liguori; +Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <87sj9oh0pm.fsf@codemonkey.ws>
Hi,
>> Well, we also want to clean up the registers, so how about:
>>
>> BAR0: legacy, as is. If you access this, don't use the others.
Ok.
>> BAR1: new format virtio-pci layout. If you use this, don't use BAR0.
>> BAR2: virtio-cfg. If you use this, don't use BAR0.
Why use two bars for this? You can put them into one mmio bar, together
with the msi-x vector table and PBA. Of course a pci capability
describing the location is helpful for that ;)
>> BAR3: ISR. If you use this, don't use BAR0.
Again, I wouldn't hardcode that but use a capability.
>> I prefer the cases exclusive (ie. use one or the other) as a clear path
>> to remove the legacy layout; and leaving the ISR in BAR0 leaves us with
>> an ugly corner case in future (ISR is BAR0 + 19? WTF?).
Ok, so we have four register sets:
(1) legacy layout
(2) new virtio-pci
(3) new virtio-config
(4) new virtio-isr
We can have a vendor pci capability, with a dword for each register set:
bit 31 -- present bit
bits 26-24 -- bar
bits 23-0 -- offset
So current drivers which must support legacy can use this:
legacy layout -- present, bar 0, offset 0
new virtio-pci -- present, bar 1, offset 0
new virtio-config -- present, bar 1, offset 256
new virtio-isr -- present, bar 0, offset 19
[ For completeness: msi-x capability could add this: ]
msi-x vector table bar 1, offset 512
msi-x pba bar 1, offset 768
> We'll never remove legacy so we shouldn't plan on it. There are
> literally hundreds of thousands of VMs out there with the current virtio
> drivers installed in them. We'll be supporting them for a very, very
> long time :-)
But new devices (virtio-qxl being a candidate) don't have old guests and
don't need to worry.
They could use this if they care about fast isr:
legacy layout -- not present
new virtio-pci -- present, bar 1, offset 0
new virtio-config -- present, bar 1, offset 256
new virtio-isr -- present, bar 0, offset 0
Or this if they don't worry about isr performance:
legacy layout -- not present
new virtio-pci -- present, bar 0, offset 0
new virtio-config -- present, bar 0, offset 256
new virtio-isr -- not present
> I don't think we gain a lot by moving the ISR into a separate BAR.
> Splitting up registers like that seems weird to me too.
Main advantage of defining a register set with just isr is that it
reduces pio address space consumtion for new virtio devices which don't
have to worry about the legacy layout (8 bytes which is minimum size for
io bars instead of 64 bytes).
> If we added an additional constraints that BAR1 was mirrored except for
Why add constraints? We want something future-proof, don't we?
>> The detection is simple: if BAR1 has non-zero length, it's new-style,
>> otherwise legacy.
Doesn't fly. BAR1 is in use today for MSI-X support.
> I agree that this is the best way to extend, but I think we should still
> use a transport feature bit. We want to be able to detect within QEMU
> whether a guest is using these new features because we need to adjust
> migration state accordingly.
Why does migration need adjustments?
[ Not that I want veto a feature bit, but I don't see the need yet ]
cheers,
Gerd
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Rusty Russell @ 2012-10-09 4:59 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kvm, Michael S. Tsirkin, netdev, linux-kernel, virtualization,
Sasha Levin, avi, Thomas Lendacky
In-Reply-To: <507029EB.2050405@redhat.com>
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 05/10/2012 07:43, Rusty Russell ha scritto:
>> 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.
Could we do that? It's the cmd length I'm concerned about; is it always
32 in practice for some reason?
Currently qemu does:
struct sg_io_hdr hdr;
memset(&hdr, 0, sizeof(struct sg_io_hdr));
hdr.interface_id = 'S';
hdr.cmd_len = req->elem.out_sg[1].iov_len;
hdr.cmdp = req->elem.out_sg[1].iov_base;
hdr.dxfer_len = 0;
If it's a command which expects more output data, there's no way to
guess where the boundary is between that command and the data.
Cheers,
Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Rusty Russell @ 2012-10-09 3:16 UTC (permalink / raw)
To: Anthony Liguori, Gerd Hoffmann
Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <87sj9oh0pm.fsf@codemonkey.ws>
Anthony Liguori <aliguori@us.ibm.com> writes:
> We'll never remove legacy so we shouldn't plan on it. There are
> literally hundreds of thousands of VMs out there with the current virtio
> drivers installed in them. We'll be supporting them for a very, very
> long time :-)
You will be supporting this for qemu on x86, sure. As I think we're
still in the growth phase for virtio, I prioritize future spec
cleanliness pretty high.
But I think you'll be surprised how fast this is deprecated:
1) Bigger queues for block devices (guest-specified ringsize)
2) Smaller rings for openbios (guest-specified alignment)
3) All-mmio mode (powerpc)
4) Whatever network features get numbers > 31.
> I don't think we gain a lot by moving the ISR into a separate BAR.
> Splitting up registers like that seems weird to me too.
Confused. I proposed the same split as you have, just ISR by itself.
> It's very normal to have a mirrored set of registers that are PIO in one
> bar and MMIO in a different BAR.
>
> If we added an additional constraints that BAR1 was mirrored except for
> the config space and the MSI section was always there, I think the end
> result would be nice. IOW:
But it won't be the same, because we want all that extra stuff, like
more feature bits and queue size alignment. (Admittedly queues past
16TB aren't a killer feature).
To make it concrete:
Current:
struct {
__le32 host_features; /* read-only */
__le32 guest_features; /* read/write */
__le32 queue_pfn; /* read/write */
__le16 queue_size; /* read-only */
__le16 queue_sel; /* read/write */
__le16 queue_notify; /* read/write */
u8 status; /* read/write */
u8 isr; /* read-only, clear on read */
/* Optional */
__le16 msi_config_vector; /* read/write */
__le16 msi_queue_vector; /* read/write */
/* ... device features */
};
Proposed:
struct virtio_pci_cfg {
/* About the whole device. */
__le32 device_feature_select; /* read-write */
__le32 device_feature; /* read-only */
__le32 guest_feature_select; /* read-write */
__le32 guest_feature; /* read-only */
__le16 msix_config; /* read-write */
__u8 device_status; /* read-write */
__u8 unused;
/* About a specific virtqueue. */
__le16 queue_select; /* read-write */
__le16 queue_align; /* read-write, power of 2. */
__le16 queue_size; /* read-write, power of 2. */
__le16 queue_msix_vector;/* read-write */
__le64 queue_address; /* read-write: 0xFFFFFFFFFFFFFFFF == DNE. */
};
struct virtio_pci_isr {
__u8 isr; /* read-only, clear on read */
};
We could also enforce LE in the per-device config space in this case,
another nice cleanup for PCI people.
> BAR0[pio]: virtio-pci registers + optional MSI section + virtio-config
> BAR1[mmio]: virtio-pci registers + MSI section + future extensions
> BAR2[mmio]: virtio-config
>
> We can continue to do ISR access via BAR0 for performance reasons.
But powerpc explicitly *doesnt* want a pio bar. So let it be its own
bar, which can be either.
>> As to MMIO vs PIO, the BARs are self-describing, so we should explicitly
>> endorse that and leave it to the devices.
>>
>> The detection is simple: if BAR1 has non-zero length, it's new-style,
>> otherwise legacy.
>
> I agree that this is the best way to extend, but I think we should still
> use a transport feature bit. We want to be able to detect within QEMU
> whether a guest is using these new features because we need to adjust
> migration state accordingly.
>
> Otherwise we would have to detect reads/writes to the new BARs to
> maintain whether the extended register state needs to be saved. This
> gets nasty dealing with things like reset.
I don't think it'll be that bad; reset clears the device to unknown,
bar0 moves it from unknown->legacy mode, bar1/2/3 changes it from
unknown->modern mode, and anything else is bad (I prefer being strict so
we catch bad implementations from the beginning).
But I'm happy to implement it and see what it's like.
> A feature bit simplifies this all pretty well.
I suspect it will be quite ugly, actually. The guest has to use BAR0 to
get the features to see if they can use BAR1. Do they ack the feature
(via BAR0) before accessing BAR1? If not, qemu can't rely on the
feature bit.
Cheers,
Rusty.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Anthony Liguori @ 2012-10-09 1:51 UTC (permalink / raw)
To: Rusty Russell, Gerd Hoffmann
Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <87sj9o8qn7.fsf@rustcorp.com.au>
Rusty Russell <rusty@rustcorp.com.au> writes:
> Anthony Liguori <aliguori@us.ibm.com> writes:
>> Gerd Hoffmann <kraxel@redhat.com> writes:
>>
>>> 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.
>>
>> I think having BAR1 be an MMIO mirror of the registers + a BAR2 for
>> virtio configuration space is probably not that bad of a solution.
>
> Well, we also want to clean up the registers, so how about:
>
> BAR0: legacy, as is. If you access this, don't use the others.
> BAR1: new format virtio-pci layout. If you use this, don't use BAR0.
> BAR2: virtio-cfg. If you use this, don't use BAR0.
> BAR3: ISR. If you use this, don't use BAR0.
>
> I prefer the cases exclusive (ie. use one or the other) as a clear path
> to remove the legacy layout; and leaving the ISR in BAR0 leaves us with
> an ugly corner case in future (ISR is BAR0 + 19? WTF?).
We'll never remove legacy so we shouldn't plan on it. There are
literally hundreds of thousands of VMs out there with the current virtio
drivers installed in them. We'll be supporting them for a very, very
long time :-)
I don't think we gain a lot by moving the ISR into a separate BAR.
Splitting up registers like that seems weird to me too.
It's very normal to have a mirrored set of registers that are PIO in one
bar and MMIO in a different BAR.
If we added an additional constraints that BAR1 was mirrored except for
the config space and the MSI section was always there, I think the end
result would be nice. IOW:
BAR0[pio]: virtio-pci registers + optional MSI section + virtio-config
BAR1[mmio]: virtio-pci registers + MSI section + future extensions
BAR2[mmio]: virtio-config
We can continue to do ISR access via BAR0 for performance reasons.
> As to MMIO vs PIO, the BARs are self-describing, so we should explicitly
> endorse that and leave it to the devices.
>
> The detection is simple: if BAR1 has non-zero length, it's new-style,
> otherwise legacy.
I agree that this is the best way to extend, but I think we should still
use a transport feature bit. We want to be able to detect within QEMU
whether a guest is using these new features because we need to adjust
migration state accordingly.
Otherwise we would have to detect reads/writes to the new BARs to
maintain whether the extended register state needs to be saved. This
gets nasty dealing with things like reset.
A feature bit simplifies this all pretty well.
Regards,
Anthony Liguori
>
> Thoughts?
> Rusty.
> --
> 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: Rusty Russell @ 2012-10-08 23:56 UTC (permalink / raw)
To: Anthony Liguori, Gerd Hoffmann
Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <874nm4u1in.fsf@codemonkey.ws>
Anthony Liguori <aliguori@us.ibm.com> writes:
> Gerd Hoffmann <kraxel@redhat.com> writes:
>
>> 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.
>
> I think having BAR1 be an MMIO mirror of the registers + a BAR2 for
> virtio configuration space is probably not that bad of a solution.
Well, we also want to clean up the registers, so how about:
BAR0: legacy, as is. If you access this, don't use the others.
BAR1: new format virtio-pci layout. If you use this, don't use BAR0.
BAR2: virtio-cfg. If you use this, don't use BAR0.
BAR3: ISR. If you use this, don't use BAR0.
I prefer the cases exclusive (ie. use one or the other) as a clear path
to remove the legacy layout; and leaving the ISR in BAR0 leaves us with
an ugly corner case in future (ISR is BAR0 + 19? WTF?).
As to MMIO vs PIO, the BARs are self-describing, so we should explicitly
endorse that and leave it to the devices.
The detection is simple: if BAR1 has non-zero length, it's new-style,
otherwise legacy.
Thoughts?
Rusty.
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: inline header support
From: Michael S. Tsirkin @ 2012-10-08 21:31 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, netdev, linux-kernel, Sasha Levin, virtualization, avi,
Anthony Liguori, Thomas Lendacky
In-Reply-To: <87391u3o67.fsf@rustcorp.com.au>
On Thu, Oct 04, 2012 at 01:04:56PM +0930, Rusty Russell wrote:
> Anthony Liguori <anthony@codemonkey.ws> writes:
> > Rusty Russell <rusty@rustcorp.com.au> writes:
> >
> >> "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.
> >
> > This is a bug in the specification.
> >
> > The QEMU implementation pre-dates the specification. All of the actual
> > implementations of virtio relied on the semantics of s/g elements and
> > still do.
>
> lguest fix is pending in my queue. lkvm and qemu are broken; lkvm isn't
> ever going to be merged, so I'm not sure what its status is? But I'm
> determined to fix qemu, and hence my torture patch to make sure this
> doesn't creep in again.
If you look at my patch you'll notice there's also a
comment in virtio_net.h that seems to be broken in this respect:
/* This is the first element of the scatter-gather list. If you don't
* specify GSO or CSUM features, you can simply ignore the header. */
There is a similar comment in virtio-blk.
^ permalink raw reply
* Re: [Qemu-devel] Using PCI config space to indicate config location
From: Anthony Liguori @ 2012-10-08 20:55 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: virtualization, qemu-devel, kvm, Michael S. Tsirkin
In-Reply-To: <507333F1.1060000@redhat.com>
Gerd Hoffmann <kraxel@redhat.com> writes:
> 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.
I think having BAR1 be an MMIO mirror of the registers + a BAR2 for
virtio configuration space is probably not that bad of a solution.
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: [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