Linux virtualization list
 help / color / mirror / Atom feed
* [PATCH net-next RFC V3 3/3] vhost_net: basic polling support
From: Jason Wang @ 2015-11-12 10:16 UTC (permalink / raw)
  To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1447323395-28052-1-git-send-email-jasowang@redhat.com>

This patch tries to poll for new added tx buffer or socket receive
queue for a while at the end of tx/rx processing. The maximum time
spent on polling were specified through a new kind of vring ioctl.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/net.c        | 77 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/vhost/vhost.c      | 15 +++++++++
 drivers/vhost/vhost.h      |  1 +
 include/uapi/linux/vhost.h | 11 +++++++
 4 files changed, 99 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 9eda69e..a38fa32 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -287,6 +287,45 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
 	rcu_read_unlock_bh();
 }
 
+static inline unsigned long busy_clock(void)
+{
+	return local_clock() >> 10;
+}
+
+static bool vhost_can_busy_poll(struct vhost_dev *dev,
+				unsigned long endtime)
+{
+	return likely(!need_resched()) &&
+	       likely(!time_after(busy_clock(), endtime)) &&
+	       likely(!signal_pending(current)) &&
+	       !vhost_has_work(dev) &&
+	       single_task_running();
+}
+
+static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
+				    struct vhost_virtqueue *vq,
+				    struct iovec iov[], unsigned int iov_size,
+				    unsigned int *out_num, unsigned int *in_num)
+{
+	unsigned long uninitialized_var(endtime);
+
+	if (vq->busyloop_timeout) {
+		preempt_disable();
+		endtime = busy_clock() + vq->busyloop_timeout;
+	}
+
+	while (vq->busyloop_timeout &&
+	       vhost_can_busy_poll(vq->dev, endtime) &&
+	       !vhost_vq_more_avail(vq->dev, vq))
+		cpu_relax();
+
+	if (vq->busyloop_timeout)
+		preempt_enable();
+
+	return vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
+				 out_num, in_num, NULL, NULL);
+}
+
 /* Expects to be always run from workqueue - which acts as
  * read-size critical section for our kind of RCU. */
 static void handle_tx(struct vhost_net *net)
@@ -331,10 +370,9 @@ static void handle_tx(struct vhost_net *net)
 			      % UIO_MAXIOV == nvq->done_idx))
 			break;
 
-		head = vhost_get_vq_desc(vq, vq->iov,
-					 ARRAY_SIZE(vq->iov),
-					 &out, &in,
-					 NULL, NULL);
+		head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
+						ARRAY_SIZE(vq->iov),
+						&out, &in);
 		/* On error, stop handling until the next kick. */
 		if (unlikely(head < 0))
 			break;
@@ -435,6 +473,35 @@ static int peek_head_len(struct sock *sk)
 	return len;
 }
 
+static int vhost_net_peek_head_len(struct vhost_net *net, struct sock *sk)
+{
+	struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
+	struct vhost_virtqueue *vq = &nvq->vq;
+	unsigned long uninitialized_var(endtime);
+
+	if (vq->busyloop_timeout) {
+		mutex_lock(&vq->mutex);
+		vhost_disable_notify(&net->dev, vq);
+		preempt_disable();
+		endtime = busy_clock() + vq->busyloop_timeout;
+	}
+
+	while (vq->busyloop_timeout &&
+	       vhost_can_busy_poll(&net->dev, endtime) &&
+	       skb_queue_empty(&sk->sk_receive_queue) &&
+	       !vhost_vq_more_avail(&net->dev, vq))
+		cpu_relax();
+
+	if (vq->busyloop_timeout) {
+		preempt_enable();
+		if (vhost_enable_notify(&net->dev, vq))
+			vhost_poll_queue(&vq->poll);
+		mutex_unlock(&vq->mutex);
+	}
+
+	return peek_head_len(sk);
+}
+
 /* This is a multi-buffer version of vhost_get_desc, that works if
  *	vq has read descriptors only.
  * @vq		- the relevant virtqueue
@@ -553,7 +620,7 @@ static void handle_rx(struct vhost_net *net)
 		vq->log : NULL;
 	mergeable = vhost_has_feature(vq, VIRTIO_NET_F_MRG_RXBUF);
 
-	while ((sock_len = peek_head_len(sock->sk))) {
+	while ((sock_len = vhost_net_peek_head_len(net, sock->sk))) {
 		sock_len += sock_hlen;
 		vhost_len = sock_len + vhost_hlen;
 		headcount = get_rx_bufs(vq, vq->heads, vhost_len,
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b86c5aa..8f9a64c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -285,6 +285,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	vq->memory = NULL;
 	vq->is_le = virtio_legacy_is_little_endian();
 	vhost_vq_reset_user_be(vq);
+	vq->busyloop_timeout = 0;
 }
 
 static int vhost_worker(void *data)
@@ -747,6 +748,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
 	struct vhost_vring_state s;
 	struct vhost_vring_file f;
 	struct vhost_vring_addr a;
+	struct vhost_vring_busyloop_timeout t;
 	u32 idx;
 	long r;
 
@@ -919,6 +921,19 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
 	case VHOST_GET_VRING_ENDIAN:
 		r = vhost_get_vring_endian(vq, idx, argp);
 		break;
+	case VHOST_SET_VRING_BUSYLOOP_TIMEOUT:
+		if (copy_from_user(&t, argp, sizeof t)) {
+			r = -EFAULT;
+			break;
+		}
+		vq->busyloop_timeout = t.timeout;
+		break;
+	case VHOST_GET_VRING_BUSYLOOP_TIMEOUT:
+		t.index = idx;
+		t.timeout = vq->busyloop_timeout;
+		if (copy_to_user(argp, &t, sizeof t))
+			r = -EFAULT;
+		break;
 	default:
 		r = -ENOIOCTLCMD;
 	}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 5983a13..90453f0 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -115,6 +115,7 @@ struct vhost_virtqueue {
 	/* Ring endianness requested by userspace for cross-endian support. */
 	bool user_be;
 #endif
+	u32 busyloop_timeout;
 };
 
 struct vhost_dev {
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index ab373191..f013656 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -27,6 +27,11 @@ struct vhost_vring_file {
 
 };
 
+struct vhost_vring_busyloop_timeout {
+	unsigned int index;
+	unsigned int timeout;
+};
+
 struct vhost_vring_addr {
 	unsigned int index;
 	/* Option flags. */
@@ -126,6 +131,12 @@ struct vhost_memory {
 #define VHOST_SET_VRING_CALL _IOW(VHOST_VIRTIO, 0x21, struct vhost_vring_file)
 /* Set eventfd to signal an error */
 #define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
+/* Set busy loop timeout */
+#define VHOST_SET_VRING_BUSYLOOP_TIMEOUT _IOW(VHOST_VIRTIO, 0x23,	\
+		                     struct vhost_vring_busyloop_timeout)
+/* Get busy loop timeout */
+#define VHOST_GET_VRING_BUSYLOOP_TIMEOUT _IOW(VHOST_VIRTIO, 0x24,	\
+		                     struct vhost_vring_busyloop_timeout)
 
 /* VHOST_NET specific defines */
 
-- 
2.1.4

^ permalink raw reply related

* [PATCH net-next RFC V3 2/3] vhost: introduce vhost_vq_more_avail()
From: Jason Wang @ 2015-11-12 10:16 UTC (permalink / raw)
  To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1447323395-28052-1-git-send-email-jasowang@redhat.com>

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/vhost.c | 26 +++++++++++++++++---------
 drivers/vhost/vhost.h |  1 +
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 163b365..b86c5aa 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1633,10 +1633,25 @@ void vhost_add_used_and_signal_n(struct vhost_dev *dev,
 }
 EXPORT_SYMBOL_GPL(vhost_add_used_and_signal_n);
 
+bool vhost_vq_more_avail(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+{
+	__virtio16 avail_idx;
+	int r;
+
+	r = __get_user(avail_idx, &vq->avail->idx);
+	if (r) {
+		vq_err(vq, "Failed to check avail idx at %p: %d\n",
+		       &vq->avail->idx, r);
+		return false;
+	}
+
+	return vhost16_to_cpu(vq, avail_idx) != vq->avail_idx;
+}
+EXPORT_SYMBOL_GPL(vhost_vq_more_avail);
+
 /* OK, now we need to know about added descriptors. */
 bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 {
-	__virtio16 avail_idx;
 	int r;
 
 	if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY))
@@ -1660,14 +1675,7 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
 	/* They could have slipped one in as we were doing that: make
 	 * sure it's written, then check again. */
 	smp_mb();
-	r = __get_user(avail_idx, &vq->avail->idx);
-	if (r) {
-		vq_err(vq, "Failed to check avail idx at %p: %d\n",
-		       &vq->avail->idx, r);
-		return false;
-	}
-
-	return vhost16_to_cpu(vq, avail_idx) != vq->avail_idx;
+	return vhost_vq_more_avail(dev, vq);
 }
 EXPORT_SYMBOL_GPL(vhost_enable_notify);
 
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index ea0327d..5983a13 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -159,6 +159,7 @@ void vhost_add_used_and_signal_n(struct vhost_dev *, struct vhost_virtqueue *,
 			       struct vring_used_elem *heads, unsigned count);
 void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
 void vhost_disable_notify(struct vhost_dev *, struct vhost_virtqueue *);
+bool vhost_vq_more_avail(struct vhost_dev *, struct vhost_virtqueue *);
 bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
-- 
2.1.4

^ permalink raw reply related

* [PATCH net-next RFC V3 1/3] vhost: introduce vhost_has_work()
From: Jason Wang @ 2015-11-12 10:16 UTC (permalink / raw)
  To: mst, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1447323395-28052-1-git-send-email-jasowang@redhat.com>

This path introduces a helper which can give a hint for whether or not
there's a work queued in the work list.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vhost/vhost.c | 7 +++++++
 drivers/vhost/vhost.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index eec2f11..163b365 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -245,6 +245,13 @@ void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work)
 }
 EXPORT_SYMBOL_GPL(vhost_work_queue);
 
+/* A lockless hint for busy polling code to exit the loop */
+bool vhost_has_work(struct vhost_dev *dev)
+{
+	return !list_empty(&dev->work_list);
+}
+EXPORT_SYMBOL_GPL(vhost_has_work);
+
 void vhost_poll_queue(struct vhost_poll *poll)
 {
 	vhost_work_queue(poll->dev, &poll->work);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 4772862..ea0327d 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -37,6 +37,7 @@ struct vhost_poll {
 
 void vhost_work_init(struct vhost_work *work, vhost_work_fn_t fn);
 void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work);
+bool vhost_has_work(struct vhost_dev *dev);
 
 void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
 		     unsigned long mask, struct vhost_dev *dev);
-- 
2.1.4

^ permalink raw reply related

* [PATCH net-next RFC V3 0/3] basic busy polling support for vhost_net
From: Jason Wang @ 2015-11-12 10:16 UTC (permalink / raw)
  To: mst, kvm, virtualization, netdev, linux-kernel

Hi all:

This series tries to add basic busy polling for vhost net. The idea is
simple: at the end of tx/rx processing, busy polling for new tx added
descriptor and rx receive socket for a while. The maximum number of
time (in us) could be spent on busy polling was specified ioctl.

Test were done through:

- 50 us as busy loop timeout
- Netperf 2.6
- Two machines with back to back connected ixgbe
- Guest with 1 vcpu and 1 queue

Results:
- For stream workload, ioexits were reduced dramatically in medium
  size (1024-2048) of tx (at most -39%) and almost all rx (at most
  -79%) as a result of polling. This compensate for the possible
  wasted cpu cycles more or less. That porbably why we can still see
  some increasing in the normalized throughput in some cases.
- Throughput of tx were increased (at most 105%) expect for the huge
  write (16384). And we can send more packets in the case (+tpkts were
  increased).
- Very minor rx regression in some cases.
- Improvemnt on TCP_RR (at most 16%).

size/session/+thu%/+normalize%/+tpkts%/+rpkts%/+ioexits%/
   64/     1/   +9%/  -17%/   +5%/  +10%/   -2%
   64/     2/   +8%/  -18%/   +6%/  +10%/   -1%
   64/     4/   +4%/  -21%/   +6%/  +10%/   -1%
   64/     8/   +9%/  -17%/   +6%/   +9%/   -2%
  256/     1/  +20%/   -1%/  +15%/  +11%/   -9%
  256/     2/  +15%/   -6%/  +15%/   +8%/   -8%
  256/     4/  +17%/   -4%/  +16%/   +8%/   -8%
  256/     8/  -61%/  -69%/  +16%/  +10%/  -10%
  512/     1/  +15%/   -3%/  +19%/  +18%/  -11%
  512/     2/  +19%/    0%/  +19%/  +13%/  -10%
  512/     4/  +18%/   -2%/  +18%/  +15%/  -10%
  512/     8/  +17%/   -1%/  +18%/  +15%/  -11%
 1024/     1/  +25%/   +4%/  +27%/  +16%/  -21%
 1024/     2/  +28%/   +8%/  +25%/  +15%/  -22%
 1024/     4/  +25%/   +5%/  +25%/  +14%/  -21%
 1024/     8/  +27%/   +7%/  +25%/  +16%/  -21%
 2048/     1/  +32%/  +12%/  +31%/  +22%/  -38%
 2048/     2/  +33%/  +12%/  +30%/  +23%/  -36%
 2048/     4/  +31%/  +10%/  +31%/  +24%/  -37%
 2048/     8/ +105%/  +75%/  +33%/  +23%/  -39%
16384/     1/    0%/  -14%/   +2%/    0%/  +19%
16384/     2/    0%/  -13%/  +19%/  -13%/  +17%
16384/     4/    0%/  -12%/   +3%/    0%/   +2%
16384/     8/    0%/  -11%/   -2%/   +1%/   +1%
size/session/+thu%/+normalize%/+tpkts%/+rpkts%/+ioexits%/
   64/     1/   -7%/  -23%/   +4%/   +6%/  -74%
   64/     2/   -2%/  -12%/   +2%/   +2%/  -55%
   64/     4/   +2%/   -5%/  +10%/   -2%/  -43%
   64/     8/   -5%/   -5%/  +11%/  -34%/  -59%
  256/     1/   -6%/  -16%/   +9%/  +11%/  -60%
  256/     2/   +3%/   -4%/   +6%/   -3%/  -28%
  256/     4/    0%/   -5%/   -9%/   -9%/  -10%
  256/     8/   -3%/   -6%/  -12%/   -9%/  -40%
  512/     1/   -4%/  -17%/  -10%/  +21%/  -34%
  512/     2/    0%/   -9%/  -14%/   -3%/  -30%
  512/     4/    0%/   -4%/  -18%/  -12%/   -4%
  512/     8/   -1%/   -4%/   -1%/   -5%/   +4%
 1024/     1/    0%/  -16%/  +12%/  +11%/  -10%
 1024/     2/    0%/  -11%/    0%/   +5%/  -31%
 1024/     4/    0%/   -4%/   -7%/   +1%/  -22%
 1024/     8/   -5%/   -6%/  -17%/  -29%/  -79%
 2048/     1/    0%/  -16%/   +1%/   +9%/  -10%
 2048/     2/    0%/  -12%/   +7%/   +9%/  -26%
 2048/     4/    0%/   -7%/   -4%/   +3%/  -64%
 2048/     8/   -1%/   -5%/   -6%/   +4%/  -20%
16384/     1/    0%/  -12%/  +11%/   +7%/  -20%
16384/     2/    0%/   -7%/   +1%/   +5%/  -26%
16384/     4/    0%/   -5%/  +12%/  +22%/  -23%
16384/     8/    0%/   -1%/   -8%/   +5%/   -3%
size/session/+thu%/+normalize%/+tpkts%/+rpkts%/+ioexits%/
    1/     1/   +9%/  -29%/   +9%/   +9%/   +9%
    1/    25/   +6%/  -18%/   +6%/   +6%/   -1%
    1/    50/   +6%/  -19%/   +5%/   +5%/   -2%
    1/   100/   +5%/  -19%/   +4%/   +4%/   -3%
   64/     1/  +10%/  -28%/  +10%/  +10%/  +10%
   64/    25/   +8%/  -18%/   +7%/   +7%/   -2%
   64/    50/   +8%/  -17%/   +8%/   +8%/   -1%
   64/   100/   +8%/  -17%/   +8%/   +8%/   -1%
  256/     1/  +10%/  -28%/  +10%/  +10%/  +10%
  256/    25/  +15%/  -13%/  +15%/  +15%/    0%
  256/    50/  +16%/  -14%/  +18%/  +18%/   +2%
  256/   100/  +15%/  -13%/  +12%/  +12%/   -2%

Changes from V2:
- poll also at the end of rx handling
- factor out the polling logic and optimize the code a little bit
- add two ioctls to get and set the busy poll timeout
- test on ixgbe (which can give more stable and reproducable numbers)
  instead of mlx4.

Changes from V1:
- Add a comment for vhost_has_work() to explain why it could be
  lockless
- Add param description for busyloop_timeout
- Split out the busy polling logic into a new helper
- Check and exit the loop when there's a pending signal
- Disable preemption during busy looping to make sure lock_clock() was
  correctly used.

Jason Wang (3):
  vhost: introduce vhost_has_work()
  vhost: introduce vhost_vq_more_avail()
  vhost_net: basic polling support

 drivers/vhost/net.c        | 77 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/vhost/vhost.c      | 48 +++++++++++++++++++++++------
 drivers/vhost/vhost.h      |  3 ++
 include/uapi/linux/vhost.h | 11 +++++++
 4 files changed, 125 insertions(+), 14 deletions(-)

-- 
2.1.4

^ permalink raw reply

* [PATCH] VMCI: Use 32bit atomics for queue headers on X86_32
From: Jorgen Hansen @ 2015-11-12  9:29 UTC (permalink / raw)
  To: linux-kernel, virtualization; +Cc: pv-drivers, gregkh, Jorgen Hansen

This change restricts the reading and setting of the head and tail
pointers on 32bit X86 to 32bit for both correctness and
performance reasons. On uniprocessor X86_32, the atomic64_read
may be implemented as a non-locked cmpxchg8b. This may result in
updates to the pointers done by the VMCI device being overwritten.
On MP systems, there is no such correctness issue, but using 32bit
atomics avoids the overhead of the locked 64bit operation. All this
is safe because the queue size on 32bit systems will never exceed
a 32bit value.

Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
Signed-off-by: Jorgen Hansen <jhansen@vmware.com>
---
 drivers/misc/vmw_vmci/vmci_driver.c |    2 +-
 include/linux/vmw_vmci_defs.h       |   43 +++++++++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/vmw_vmci/vmci_driver.c b/drivers/misc/vmw_vmci/vmci_driver.c
index b823f9a..896be15 100644
--- a/drivers/misc/vmw_vmci/vmci_driver.c
+++ b/drivers/misc/vmw_vmci/vmci_driver.c
@@ -113,5 +113,5 @@ module_exit(vmci_drv_exit);
 
 MODULE_AUTHOR("VMware, Inc.");
 MODULE_DESCRIPTION("VMware Virtual Machine Communication Interface.");
-MODULE_VERSION("1.1.3.0-k");
+MODULE_VERSION("1.1.4.0-k");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
index 65ac54c..1bd31a3 100644
--- a/include/linux/vmw_vmci_defs.h
+++ b/include/linux/vmw_vmci_defs.h
@@ -734,6 +734,41 @@ static inline void *vmci_event_data_payload(struct vmci_event_data *ev_data)
 }
 
 /*
+ * Helper to read a value from a head or tail pointer. For X86_32, the
+ * pointer is treated as a 32bit value, since the pointer value
+ * never exceeds a 32bit value in this case. Also, doing an
+ * atomic64_read on X86_32 uniprocessor systems may be implemented
+ * as a non locked cmpxchg8b, that may end up overwriting updates done
+ * by the VMCI device to the memory location. On 32bit SMP, the lock
+ * prefix will be used, so correctness isn't an issue, but using a
+ * 64bit operation still adds unnecessary overhead.
+ */
+static inline u64 vmci_q_read_pointer(atomic64_t *var)
+{
+#if defined(CONFIG_X86_32)
+	return atomic_read((atomic_t *)var);
+#else
+	return atomic64_read(var);
+#endif
+}
+
+/*
+ * Helper to set the value of a head or tail pointer. For X86_32, the
+ * pointer is treated as a 32bit value, since the pointer value
+ * never exceeds a 32bit value in this case. On 32bit SMP, using a
+ * locked cmpxchg8b adds unnecessary overhead.
+ */
+static inline void vmci_q_set_pointer(atomic64_t *var,
+				      u64 new_val)
+{
+#if defined(CONFIG_X86_32)
+	return atomic_set((atomic_t *)var, (u32)new_val);
+#else
+	return atomic64_set(var, new_val);
+#endif
+}
+
+/*
  * Helper to add a given offset to a head or tail pointer. Wraps the
  * value of the pointer around the max size of the queue.
  */
@@ -741,14 +776,14 @@ static inline void vmci_qp_add_pointer(atomic64_t *var,
 				       size_t add,
 				       u64 size)
 {
-	u64 new_val = atomic64_read(var);
+	u64 new_val = vmci_q_read_pointer(var);
 
 	if (new_val >= size - add)
 		new_val -= size;
 
 	new_val += add;
 
-	atomic64_set(var, new_val);
+	vmci_q_set_pointer(var, new_val);
 }
 
 /*
@@ -758,7 +793,7 @@ static inline u64
 vmci_q_header_producer_tail(const struct vmci_queue_header *q_header)
 {
 	struct vmci_queue_header *qh = (struct vmci_queue_header *)q_header;
-	return atomic64_read(&qh->producer_tail);
+	return vmci_q_read_pointer(&qh->producer_tail);
 }
 
 /*
@@ -768,7 +803,7 @@ static inline u64
 vmci_q_header_consumer_head(const struct vmci_queue_header *q_header)
 {
 	struct vmci_queue_header *qh = (struct vmci_queue_header *)q_header;
-	return atomic64_read(&qh->consumer_head);
+	return vmci_q_read_pointer(&qh->consumer_head);
 }
 
 /*
-- 
1.7.0

^ permalink raw reply related

* Re: [PATCH] vhost: move is_le setup to the backend
From: Greg Kurz @ 2015-11-12  8:30 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20151030114235.28847.63465.stgit@bahia.huguette.org>

On Fri, 30 Oct 2015 12:42:35 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> The vq->is_le field is used to fix endianness when accessing the vring via
> the cpu_to_vhost16() and vhost16_to_cpu() helpers in the following cases:
> 
> 1) host is big endian and device is modern virtio
> 
> 2) host has cross-endian support and device is legacy virtio with a different
>    endianness than the host
> 
> Both cases rely on the VHOST_SET_FEATURES ioctl, but 2) also needs the
> VHOST_SET_VRING_ENDIAN ioctl to be called by userspace. Since vq->is_le
> is only needed when the backend is active, it was decided to set it at
> backend start.
> 
> This is currently done in vhost_init_used()->vhost_init_is_le() but it
> obfuscates the core vhost code. This patch moves the is_le setup to a
> dedicated function that is called from the backend code.
> 
> Note vhost_net is the only backend that can pass vq->private_data == NULL to
> vhost_init_used(), hence the "if (sock)" branch.
> 
> No behaviour change.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---

Ping ?

>  drivers/vhost/net.c   |    6 ++++++
>  drivers/vhost/scsi.c  |    3 +++
>  drivers/vhost/test.c  |    2 ++
>  drivers/vhost/vhost.c |   12 +++++++-----
>  drivers/vhost/vhost.h |    1 +
>  5 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 9eda69e40678..d6319cb2664c 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -917,6 +917,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> 
>  		vhost_net_disable_vq(n, vq);
>  		vq->private_data = sock;
> +
> +		if (sock)
> +			vhost_set_is_le(vq);
> +		else
> +			vq->is_le = virtio_legacy_is_little_endian();
> +
>  		r = vhost_init_used(vq);
>  		if (r)
>  			goto err_used;
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index e25a23692822..e2644a301fa5 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1276,6 +1276,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
>  			vq = &vs->vqs[i].vq;
>  			mutex_lock(&vq->mutex);
>  			vq->private_data = vs_tpg;
> +
> +			vhost_set_is_le(vq);
> +
>  			vhost_init_used(vq);
>  			mutex_unlock(&vq->mutex);
>  		}
> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> index f2882ac98726..b1c7df502211 100644
> --- a/drivers/vhost/test.c
> +++ b/drivers/vhost/test.c
> @@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
>  		oldpriv = vq->private_data;
>  		vq->private_data = priv;
> 
> +		vhost_set_is_le(vq);
> +
>  		r = vhost_init_used(&n->vqs[index]);
> 
>  		mutex_unlock(&vq->mutex);
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index eec2f11809ff..6be863dcbd13 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -113,6 +113,12 @@ static void vhost_init_is_le(struct vhost_virtqueue *vq)
>  }
>  #endif /* CONFIG_VHOST_CROSS_ENDIAN_LEGACY */
> 
> +void vhost_set_is_le(struct vhost_virtqueue *vq)
> +{
> +	vhost_init_is_le(vq);
> +}
> +EXPORT_SYMBOL_GPL(vhost_set_is_le);
> +
>  static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
>  			    poll_table *pt)
>  {
> @@ -1156,12 +1162,8 @@ int vhost_init_used(struct vhost_virtqueue *vq)
>  {
>  	__virtio16 last_used_idx;
>  	int r;
> -	if (!vq->private_data) {
> -		vq->is_le = virtio_legacy_is_little_endian();
> +	if (!vq->private_data)
>  		return 0;
> -	}
> -
> -	vhost_init_is_le(vq);
> 
>  	r = vhost_update_used_flags(vq);
>  	if (r)
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index 4772862b71a7..8a62041959fe 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
> 
>  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
>  		    unsigned int log_num, u64 len);
> +void vhost_set_is_le(struct vhost_virtqueue *vq);
> 
>  #define vq_err(vq, fmt, ...) do {                                  \
>  		pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \
> 
> --
> 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 v3 0/3] virtio DMA API core stuff
From: David Woodhouse @ 2015-11-11 22:30 UTC (permalink / raw)
  To: Andy Lutomirski, Michael S. Tsirkin
  Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
	linux-kernel@vger.kernel.org, Linux Virtualization,
	Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
	Paolo Bonzini, Christoph Hellwig
In-Reply-To: <CALCETrWmZaQxS3-r9jsUb3BPhdLRbRrdZWok2geHnYKaWC4YKA@mail.gmail.com>


[-- Attachment #1.1: Type: text/plain, Size: 3666 bytes --]

On Wed, 2015-11-11 at 07:56 -0800, Andy Lutomirski wrote:
> 
> Can you flesh out this trick?
> 
> On x86 IIUC the IOMMU more-or-less defaults to passthrough.  If the
> kernel wants, it can switch it to a non-passthrough mode.  My patches
> cause the virtio driver to do exactly this, except that the host
> implementation doesn't actually exist yet, so the patches will instead
> have no particular effect.

At some level, yes — we're compatible with a 1982 IBM PC and thus the
IOMMU is entirely disabled at boot until the kernel turns it on —
except in TXT mode where we abandon that compatibility.

But no, the virtio driver has *nothing* to do with switching the device
out of passthrough mode. It is either in passthrough mode, or it isn't.

If the VMM *doesn't* expose an IOMMU to the guest, obviously the
devices are in passthrough mode. If the guest kernel doesn't have IOMMU
support enabled, then obviously the devices are in passthrough mode.
And if the ACPI tables exposed to the guest kernel *tell* it that the
virtio devices are not actually behind the IOMMU (which qemu gets
wrong), then it'll be in passthrough mode.

If the IOMMU is exposed, and enabled, and telling the guest kernel that
it *does* cover the virtio devices, then those virtio devices will
*not* be in passthrough mode.

You choosing to use the DMA API in the virtio device drivers instead of
being buggy, has nothing to do with whether it's actually in
passthrough mode or not. Whether it's in passthrough mode or not, using
the DMA API is technically the right thing to do — because it should
either *do* the translation, or return a 1:1 mapped IOVA, as
appropriate.


> On powerpc and sparc, we *already* screwed up.  The host already tells
> the guest that there's an IOMMU and that it's *enabled* because those
> platforms don't have selective IOMMU coverage the way that x86 does.
> So we need to work around it.

No, we need it on x86 too because once we fix the virtio device driver
bug and make it start using the DMA API, then we start to trip up on
the qemu bug where it lies about which devices are covered by the
IOMMU.

Of course, we still have that same qemu bug w.r.t. assigned devices,
which it *also* claims are behind its IOMMU when they're not...

> I think that, if we want fancy virt-friendly IOMMU stuff like you're
> talking about, then the right thing to do is to create a virtio bus
> instead of pretending to be PCI.  That bus could have a virtio IOMMU
> and its own cross-platform enumeration mechanism for devices on the
> bus, and everything would be peachy.

That doesn't really help very much for the x86 case where the problem
is compatibility with *existing* (arguably broken) qemu
implementations.

Having said that, if this were real hardware I'd just be blacklisting
it and saying "Another BIOS with broken DMAR tables --> IOMMU
completely disabled". So perhaps we should just do that.


> I still don't understand what trick.  If we want virtio devices to be
> assignable, then they should be translated through the IOMMU, and the
> DMA API is the right interface for that.

The DMA API is the right interface *regardless* of whether there's
actual translation to be done. The device driver itself should not be
involved in any way with that decision.

When you want to access MMIO, you use ioremap() and writel() instead of
doing random crap for yourself. When you want DMA, you use the DMA API
to get a bus address for your device *even* if you expect there to be
no IOMMU and you expect it to precisely match the physical address. No
excuses.

-- 
dwmw2



[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* WorldCIST'2016 - Deadline extended: November 22, 2015
From: Maria Lemos @ 2015-11-11 16:59 UTC (permalink / raw)
  To: virtualization

[-- Attachment #1: Type: text/plain, Size: 7311 bytes --]

...
..... Apologize if you receive multiple copies of this email, or if its content is irrelevant for you.
....... Please forward for your contacts. Thank you so much!
.....
...

---------
WorldCIST'16 - 4th World Conference on Information Systems and Technologies 
Recife, PE, Brazil
22th-24th of March 2016
http://www.aisti.eu/worldcist16/
-------------------------------------------


SCOPE

The WorldCist'16 - 4th World Conference on Information Systems and Technologies ( http://www.aisti.eu/worldcist16/ ), to be held at Recife, PE, Brazil, 22 - 24 March 2016, is a global forum for researchers and practitioners to present and discuss the most recent innovations, trends, results, experiences and concerns in the several perspectives of Information Systems and Technologies.

We are pleased to invite you to submit your papers to WorldCist'16. All submissions will be reviewed on the basis of relevance, originality, importance and clarity.


THEMES

Submitted papers should be related with one or more of the main themes proposed for the Conference:

A) Information and Knowledge Management (IKM);
B) Organizational Models and Information Systems (OMIS);
C) Software and Systems Modeling (SSM);
D) Software Systems, Architectures, Applications and Tools (SSAAT);
E) Multimedia Systems and Applications (MSA);
F) Computer Networks, Mobility and Pervasive Systems (CNMPS);
G) Intelligent and Decision Support Systems (IDSS);
H) Big Data Analytics and Applications (BDAA);
I) Human-Computer Interaction (HCI);
J) Health Informatics (HIS);
K) Information Technologies in Education (ITE);
L) Information Technologies in Radiocommunications (ITR).


TYPES OF SUBMISSIONS AND DECISIONS

Four types of papers can be submitted:

- Full paper: Finished or consolidated R&D works, to be included in one of the Conference themes. These papers are assigned a 10-page limit.

- Short paper: Ongoing works with relevant preliminary results, open to discussion. These papers are assigned a 7-page limit.

-Poster paper: Initial work with relevant ideas, open to discussion. These papers are assigned to a 4-page limit.

- Company paper: Companies' papers that show practical experience, R & D, tools, etc., focused on some topics of the conference. These papers are assigned to a 4-page limit.

Submitted papers must comply with the format of Advances in Intelligent Systems and Computing Series (see Instructions for Authors at Springer Website or download a DOC example) be written in English, must not have been published before, not be under review for any other conference or publication and not include any information leading to the authors’ identification. Therefore, the authors’ names, affiliations and bibliographic references should not be included in the version for evaluation by the Program Committee. This information should only be included in the camera-ready version, saved in Word or Latex format and also in PDF format. These files must be accompanied by the Consent to Publication form filled out, in a ZIP file, and uploaded at the conference management system.

All papers will be subjected to a “double-blind review” by at least two members of the Program Committee.

Based on Program Committee evaluation, a paper can be rejected or accepted by the Conference Chairs. In the later case, it can be accepted as the type originally submitted or as another type. Thus, full papers can be accepted as short papers or poster papers only. Similarly, short papers can be accepted as poster papers only. In these cases, the authors will be allowed to maintain the original number of pages in the camera-ready version.

The authors of accepted poster papers must also build and print a poster to be exhibited during the Conference. This poster must follow an A1 or A2 vertical format. The Conference can includes Work Sessions where these posters are presented and orally discussed, with a 5 minute limit per poster.

The authors of accepted full papers will have 15 minutes to present their work in a Conference Work Session; approximately 5 minutes of discussion will follow each presentation. The authors of accepted short papers and company papers will have 11 minutes to present their work in a Conference Work Session; approximately 4 minutes of discussion will follow each presentation.


PUBLICATION AND INDEXING

To ensure that a full paper, short paper, poster paper or company paper is published in the Proceedings, at least one of the authors must be fully registered by the 27th of December 2015, and the paper must comply with the suggested layout and page-limit. Additionally, all recommended changes must be addressed by the authors before they submit the camera-ready version.

No more than one paper per registration will be published in the Conference Proceedings. An extra fee must be paid for publication of additional papers, with a maximum of one additional paper per registration.

Full and short papers will be published in Proceedings by Springer, in a book of Advances in Intelligent Systems and Computing series. Poster and company papers will be published by AISTI.

Published full and short papers will be submitted for indexation by ISI, EI-Compendex, SCOPUS and DBLP, among others, and will be available in the SpringerLink Digital Library.

The authors of the best selected papers will be invited to extend them for publication in international journals indexed by ISI/SCI, SCOPUS and DBLP, among others, such as:

- International Journal of Neural Systems (IF: 6.507)
- Integrated Computer-Aided Engineering (IF: 4.698)
- Computers in Human Behavior (IF: 2.694)
- Journal of Medical Systems (IF: 2.213)
- International Journal of Computer-Supported Collaborative Learning (IF: 1.841)
- Journal of Intelligent & Fuzzy Systems (IF: 1.812)
- Telemedicine and e-Health (IF: 1.668)
- International Journal of Information Management (IF: 1.550)
- Engineering Computations (IF: 1.495)
- Electronic Commerce Research and Applications (IF: 1.482)
- Telematics and Informatics (IF: 1.120)
- Journal of Evaluation in Clinical Practice (IF: 1.084)
- Ethics and Information Technology (IF: 1.021)
- Int. Journal of Computers Communications & Control (IF: 0.746)
- IET Software (IF: 0.595)
- Knowledge Management Research & Practice (IF: 0.554)
- AI Communications (IF: 0.547)
- Computing and Informatics (IF: 0.504)
- Universal Access in the Information Society (IF: 0.475)
- Journal of Global Information Management (IF: 0.424)
- Journal of Internet Services and Applications (SJR: 0.88)
- Journal of Hospitality and Tourism Technology (SJR: 0.41)
- VINE - The Journal of Information and Knowledge Management Systems (SJR: 0.24)
- International Journal of Online Engineering (SJR: 0.21)
- Int. Journal of Emerging Technologies in Learning (SJR: 0.12)
- Computer Methods in Biomechanics and Biomedical Engineering: Imaging & Visualization


IMPORTANT DATES

Paper Submission: November 22, 2015

Notification of Acceptance: December 13, 2015

Payment of Registration, to ensure the inclusion of an accepted paper in the conference proceedings: December 27, 2015.

Camera-ready Submission: December 31, 2015


-

WorldCIST'16
http://www.aisti.eu/worldcist16/





[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Andy Lutomirski @ 2015-11-11 15:56 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
	linux-kernel@vger.kernel.org, Linux Virtualization,
	Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
	Paolo Bonzini, David Woodhouse, Christoph Hellwig
In-Reply-To: <20151111111125-mutt-send-email-mst@redhat.com>

On Wed, Nov 11, 2015 at 2:05 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Nov 10, 2015 at 10:54:21AM -0800, Andy Lutomirski wrote:
>> On Nov 10, 2015 7:02 AM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >
>> > On Sun, Nov 08, 2015 at 12:49:46PM +0100, Joerg Roedel wrote:
>> > > On Sun, Nov 08, 2015 at 12:37:47PM +0200, Michael S. Tsirkin wrote:
>> > > > I have no problem with that. For example, can we teach
>> > > > the DMA API on intel x86 to use PT for virtio by default?
>> > > > That would allow merging Andy's patches with
>> > > > full compatibility with old guests and hosts.
>> > >
>> > > Well, the only incompatibility comes from an experimental qemu feature,
>> > > more explicitly from a bug in that features implementation. So why
>> > > should we work around that in the kernel? I think it is not too hard to
>> > > fix qemu to generate a correct DMAR table which excludes the virtio
>> > > devices from iommu translation.
>> > >
>> > >
>> > >       Joerg
>> >
>> > It's not that easy - you'd have to dedicate some buses
>> > for iommu bypass, and teach management tools to only put
>> > virtio there - but it's possible.
>> >
>> > This will absolutely address guests that don't need to set up IOMMU for
>> > virtio devices, and virtio that bypasses the IOMMU.
>> >
>> > But the problem is that we do want to *allow* guests
>> > to set up IOMMU for virtio devices.
>> > In that case, these are two other usecases:
>> >
>> > A- monolitic virtio within QEMU:
>> >         iommu only needed for VFIO ->
>> >         guest should always use iommu=pt
>> >         iommu=on works but is just useless overhead.
>> >
>> > B- modular out of process virtio outside QEMU:
>> >         iommu needed for VFIO or kernel driver ->
>> >         guest should use iommu=pt or iommu=on
>> >         depending on security/performance requirements
>> >
>> > Note that there could easily be a mix of these in the same system.
>> >
>> > So for these cases we do need QEMU to specify to guest that IOMMU covers
>> > the virtio devices.  Also, once one does this, the default on linux is
>> > iommu=on and not pt, which works but ATM is very slow.
>> >
>> > This poses three problems:
>> >
>> > 1. How do we address the different needs of A and B?
>> >    One way would be for virtio to pass the information to guest
>> >    using some virtio specific way, and have drivers
>> >    specify what kind of DMA access they want.
>> >
>> > 2. (Kind of a subset of 1) once we do allow IOMMU, how do we make sure most guests
>> >    use the more sensible iommu=pt.
>> >
>> > 3. Once we do allow IOMMU, how can we keep existing guests work in this configuration?
>> >    Creating different hypervisor configurations depending on guest is very nasty.
>> >    Again, one way would be some virtio specific interface.
>> >
>> > I'd rather we figured the answers to this before merging Andy's patches
>> > because I'm concerned that instead of 1 broken configuration
>> > (virtio always bypasses IOMMU) we'll get two bad configurations
>> > (in the second one, virtio uses the slow default with no
>> > gain in security).
>> >
>> > Suggestions wellcome.
>>
>> I think there's still no downside of using my patches, even on x86.
>>
>> Old kernels on new QEMU work unless IOMMU is enabled on the host.  I
>> think that's the best we can possibly do.
>> New kernels work at full speed on old QEMU.
>
> Only if IOMMU is disabled, right?
>
>> New kernels with new QEMU and iommu enabled work slower.  Even newer
>> kernels with default passthrough work at full speed, and there's no
>> obvious downside to the existence of kernels with just my patches.
>>
>> --Andy
>>
>
> I tried to explain the possible downside. Let me try again.  Imagine
> that guest kernel notifies hypervisor that it wants IOMMU to actually
> work.  This will make old kernel on new QEMU work even with IOMMU
> enabled on host - better than "the best we can do" that you described
> above.  Specifically, QEMU will assume that if it didn't get
> notification, it's an old kernel so it should ignore the IOMMU.

Can you flesh out this trick?

On x86 IIUC the IOMMU more-or-less defaults to passthrough.  If the
kernel wants, it can switch it to a non-passthrough mode.  My patches
cause the virtio driver to do exactly this, except that the host
implementation doesn't actually exist yet, so the patches will instead
have no particular effect.

On powerpc and sparc, we *already* screwed up.  The host already tells
the guest that there's an IOMMU and that it's *enabled* because those
platforms don't have selective IOMMU coverage the way that x86 does.
So we need to work around it.

I think that, if we want fancy virt-friendly IOMMU stuff like you're
talking about, then the right thing to do is to create a virtio bus
instead of pretending to be PCI.  That bus could have a virtio IOMMU
and its own cross-platform enumeration mechanism for devices on the
bus, and everything would be peachy.

In the mean time, there are existing mechanisms by which every PCI
driver is supposed to notify the host/platform of how it intends to
map DMA memory, and virtio gets it wrong.

>
> But if we apply your patches this trick won't work.
>

I still don't understand what trick.  If we want virtio devices to be
assignable, then they should be translated through the IOMMU, and the
DMA API is the right interface for that.

> Without implementing it all, I think the easiest incremental step would
> be to teach linux to make passthrough the default when running as a
> guest on top of QEMU, put your patches on top. If someone specifies
> non passthrough on command line it'll still be broken,
> but not too bad.

Can powerpc and sparc do exact 1:1 passthrough for a given device?  If
so, that might be a reasonable way forward.  After all, if a new
powerpc kernel asks for exact passthrough (dma addr = phys addr with
no offset at all), then old QEMU will just ignore it and therefore
accidentally get it right.  Ben?

--Andy

^ permalink raw reply

* Re: [PATCH] virtio_ring: Shadow available ring flags & index
From: Michael S. Tsirkin @ 2015-11-11 12:34 UTC (permalink / raw)
  To: Venkatesh Srinivas
  Cc: kvm, virtualization, vsrinivas, luto, dmatlack, pbonzini,
	huawei.xie
In-Reply-To: <1447201267-30024-1-git-send-email-venkateshs@google.com>

On Tue, Nov 10, 2015 at 04:21:07PM -0800, Venkatesh Srinivas wrote:
> Improves cacheline transfer flow of available ring header.
> 
> Virtqueues are implemented as a pair of rings, one producer->consumer
> avail ring and one consumer->producer used ring; preceding the
> avail ring in memory are two contiguous u16 fields -- avail->flags
> and avail->idx. A producer posts work by writing to avail->idx and
> a consumer reads avail->idx.
> 
> The flags and idx fields only need to be written by a producer CPU
> and only read by a consumer CPU; when the producer and consumer are
> running on different CPUs and the virtio_ring code is structured to
> only have source writes/sink reads, we can continuously transfer the
> avail header cacheline between 'M' states between cores. This flow
> optimizes core -> core bandwidth on certain CPUs.
> 
> (see: "Software Optimization Guide for AMD Family 15h Processors",
> Section 11.6; similar language appears in the 10h guide and should
> apply to CPUs w/ exclusive caches, using LLC as a transfer cache)
> 
> Unfortunately the existing virtio_ring code issued reads to the
> avail->idx and read-modify-writes to avail->flags on the producer.
> 
> This change shadows the flags and index fields in producer memory;
> the vring code now reads from the shadows and only ever writes to
> avail->flags and avail->idx, allowing the cacheline to transfer
> core -> core optimally.

Sounds logical, I'll apply this after a  bit of testing
of my own, thanks!

> In a concurrent version of vring_bench, the time required for
> 10,000,000 buffer checkout/returns was reduced by ~2% (average
> across many runs) on an AMD Piledriver (15h) CPU:
> 
> (w/o shadowing):
>  Performance counter stats for './vring_bench':
>      5,451,082,016      L1-dcache-loads
>      ...
>        2.221477739 seconds time elapsed
> 
> (w/ shadowing):
>  Performance counter stats for './vring_bench':
>      5,405,701,361      L1-dcache-loads
>      ...
>        2.168405376 seconds time elapsed

Could you supply the full command line you used
to test this?

> The further away (in a NUMA sense) virtio producers and consumers are
> from each other, the more we expect to benefit. Physical implementations
> of virtio devices and implementations of virtio where the consumer polls
> vring avail indexes (vhost) should also benefit.
> 
> Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>

Here's a similar patch for the ring itself:
https://lkml.org/lkml/2015/9/10/111

Does it help you as well?


> ---
>  drivers/virtio/virtio_ring.c | 46 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 096b857..6262015 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -80,6 +80,12 @@ struct vring_virtqueue {
>  	/* Last used index we've seen. */
>  	u16 last_used_idx;
>  
> +	/* Last written value to avail->flags */
> +	u16 avail_flags_shadow;
> +
> +	/* Last written value to avail->idx in guest byte order */
> +	u16 avail_idx_shadow;
> +
>  	/* How to notify other side. FIXME: commonalize hcalls! */
>  	bool (*notify)(struct virtqueue *vq);
>  
> @@ -235,13 +241,14 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>  
>  	/* Put entry in available array (but don't update avail->idx until they
>  	 * do sync). */
> -	avail = virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) & (vq->vring.num - 1);
> +	avail = vq->avail_idx_shadow & (vq->vring.num - 1);
>  	vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
>  
>  	/* Descriptors and available array need to be set before we expose the
>  	 * new available array entries. */
>  	virtio_wmb(vq->weak_barriers);
> -	vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) + 1);
> +	vq->avail_idx_shadow++;
> +	vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
>  	vq->num_added++;
>  
>  	pr_debug("Added buffer head %i to %p\n", head, vq);
> @@ -354,8 +361,8 @@ bool virtqueue_kick_prepare(struct virtqueue *_vq)
>  	 * event. */
>  	virtio_mb(vq->weak_barriers);
>  
> -	old = virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) - vq->num_added;
> -	new = virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx);
> +	old = vq->avail_idx_shadow - vq->num_added;
> +	new = vq->avail_idx_shadow;
>  	vq->num_added = 0;
>  
>  #ifdef DEBUG
> @@ -510,7 +517,7 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>  	/* If we expect an interrupt for the next entry, tell host
>  	 * by writing event index and flush out the write before
>  	 * the read in the next get_buf call. */
> -	if (!(vq->vring.avail->flags & cpu_to_virtio16(_vq->vdev, VRING_AVAIL_F_NO_INTERRUPT))) {
> +	if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
>  		vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, vq->last_used_idx);
>  		virtio_mb(vq->weak_barriers);
>  	}
> @@ -537,7 +544,11 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
>  
> -	vq->vring.avail->flags |= cpu_to_virtio16(_vq->vdev, VRING_AVAIL_F_NO_INTERRUPT);
> +	if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
> +		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> +		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +	}
> +
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
>  
> @@ -565,7 +576,10 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
>  	/* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
>  	 * either clear the flags bit or point the event index at the next
>  	 * entry. Always do both to keep code simple. */
> -	vq->vring.avail->flags &= cpu_to_virtio16(_vq->vdev, ~VRING_AVAIL_F_NO_INTERRUPT);
> +	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
> +		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
> +		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +	}
>  	vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx = vq->last_used_idx);
>  	END_USE(vq);
>  	return last_used_idx;
> @@ -633,9 +647,12 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
>  	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
>  	 * either clear the flags bit or point the event index at the next
>  	 * entry. Always do both to keep code simple. */
> -	vq->vring.avail->flags &= cpu_to_virtio16(_vq->vdev, ~VRING_AVAIL_F_NO_INTERRUPT);
> +	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
> +		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
> +		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +	}
>  	/* TODO: tune this threshold */
> -	bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) - vq->last_used_idx) * 3 / 4;
> +	bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
>  	vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, vq->last_used_idx + bufs);
>  	virtio_mb(vq->weak_barriers);
>  	if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) - vq->last_used_idx) > bufs)) {
> @@ -670,7 +687,8 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
>  		/* detach_buf clears data, so grab it now. */
>  		buf = vq->data[i];
>  		detach_buf(vq, i);
> -		vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) - 1);
> +		vq->avail_idx_shadow--;
> +		vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
>  		END_USE(vq);
>  		return buf;
>  	}
> @@ -735,6 +753,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>  	vq->weak_barriers = weak_barriers;
>  	vq->broken = false;
>  	vq->last_used_idx = 0;
> +	vq->avail_flags_shadow = 0;
> +	vq->avail_idx_shadow = 0;
>  	vq->num_added = 0;
>  	list_add_tail(&vq->vq.list, &vdev->vqs);
>  #ifdef DEBUG
> @@ -746,8 +766,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>  	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
>  
>  	/* No callback?  Tell other side not to bother us. */
> -	if (!callback)
> -		vq->vring.avail->flags |= cpu_to_virtio16(vdev, VRING_AVAIL_F_NO_INTERRUPT);
> +	if (!callback) {
> +		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> +		vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
> +	}
>  
>  	/* Put everything in free lists. */
>  	vq->free_head = 0;
> -- 
> 2.6.0.rc2.230.g3dd15c0

^ permalink raw reply

* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-11 10:05 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
	linux-kernel@vger.kernel.org, Linux Virtualization,
	Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
	Paolo Bonzini, David Woodhouse, Christoph Hellwig
In-Reply-To: <CALCETrUKnNajtHMGarpxdVEVzbJvDr6bU88j530DVJXWrGW9bQ@mail.gmail.com>

On Tue, Nov 10, 2015 at 10:54:21AM -0800, Andy Lutomirski wrote:
> On Nov 10, 2015 7:02 AM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > On Sun, Nov 08, 2015 at 12:49:46PM +0100, Joerg Roedel wrote:
> > > On Sun, Nov 08, 2015 at 12:37:47PM +0200, Michael S. Tsirkin wrote:
> > > > I have no problem with that. For example, can we teach
> > > > the DMA API on intel x86 to use PT for virtio by default?
> > > > That would allow merging Andy's patches with
> > > > full compatibility with old guests and hosts.
> > >
> > > Well, the only incompatibility comes from an experimental qemu feature,
> > > more explicitly from a bug in that features implementation. So why
> > > should we work around that in the kernel? I think it is not too hard to
> > > fix qemu to generate a correct DMAR table which excludes the virtio
> > > devices from iommu translation.
> > >
> > >
> > >       Joerg
> >
> > It's not that easy - you'd have to dedicate some buses
> > for iommu bypass, and teach management tools to only put
> > virtio there - but it's possible.
> >
> > This will absolutely address guests that don't need to set up IOMMU for
> > virtio devices, and virtio that bypasses the IOMMU.
> >
> > But the problem is that we do want to *allow* guests
> > to set up IOMMU for virtio devices.
> > In that case, these are two other usecases:
> >
> > A- monolitic virtio within QEMU:
> >         iommu only needed for VFIO ->
> >         guest should always use iommu=pt
> >         iommu=on works but is just useless overhead.
> >
> > B- modular out of process virtio outside QEMU:
> >         iommu needed for VFIO or kernel driver ->
> >         guest should use iommu=pt or iommu=on
> >         depending on security/performance requirements
> >
> > Note that there could easily be a mix of these in the same system.
> >
> > So for these cases we do need QEMU to specify to guest that IOMMU covers
> > the virtio devices.  Also, once one does this, the default on linux is
> > iommu=on and not pt, which works but ATM is very slow.
> >
> > This poses three problems:
> >
> > 1. How do we address the different needs of A and B?
> >    One way would be for virtio to pass the information to guest
> >    using some virtio specific way, and have drivers
> >    specify what kind of DMA access they want.
> >
> > 2. (Kind of a subset of 1) once we do allow IOMMU, how do we make sure most guests
> >    use the more sensible iommu=pt.
> >
> > 3. Once we do allow IOMMU, how can we keep existing guests work in this configuration?
> >    Creating different hypervisor configurations depending on guest is very nasty.
> >    Again, one way would be some virtio specific interface.
> >
> > I'd rather we figured the answers to this before merging Andy's patches
> > because I'm concerned that instead of 1 broken configuration
> > (virtio always bypasses IOMMU) we'll get two bad configurations
> > (in the second one, virtio uses the slow default with no
> > gain in security).
> >
> > Suggestions wellcome.
> 
> I think there's still no downside of using my patches, even on x86.
> 
> Old kernels on new QEMU work unless IOMMU is enabled on the host.  I
> think that's the best we can possibly do.
> New kernels work at full speed on old QEMU.

Only if IOMMU is disabled, right?

> New kernels with new QEMU and iommu enabled work slower.  Even newer
> kernels with default passthrough work at full speed, and there's no
> obvious downside to the existence of kernels with just my patches.
> 
> --Andy
> 

I tried to explain the possible downside. Let me try again.  Imagine
that guest kernel notifies hypervisor that it wants IOMMU to actually
work.  This will make old kernel on new QEMU work even with IOMMU
enabled on host - better than "the best we can do" that you described
above.  Specifically, QEMU will assume that if it didn't get
notification, it's an old kernel so it should ignore the IOMMU.

But if we apply your patches this trick won't work.

Without implementing it all, I think the easiest incremental step would
be to teach linux to make passthrough the default when running as a
guest on top of QEMU, put your patches on top. If someone specifies
non passthrough on command line it'll still be broken,
but not too bad.


> >
> > --
> > MST

^ permalink raw reply

* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-11  9:11 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
	linux-kernel@vger.kernel.org, Andy Lutomirski,
	Christian Borntraeger, Andy Lutomirski, Paolo Bonzini,
	Linux Virtualization, David Woodhouse, Christoph Hellwig,
	Martin Schwidefsky
In-Reply-To: <20151030151612.GB2704@suse.de>

On Sat, Oct 31, 2015 at 12:16:12AM +0900, Joerg Roedel wrote:
> On Thu, Oct 29, 2015 at 11:01:41AM +0200, Michael S. Tsirkin wrote:
> > Example: you have a mix of assigned devices and virtio devices. You
> > don't trust your assigned device vendor not to corrupt your memory so
> > you want to limit the damage your assigned device can do to your guest,
> > so you use an IOMMU for that.  Thus existing iommu=pt within guest is out.
> > 
> > But you trust your hypervisor (you have no choice anyway),
> > and you don't want the overhead of tweaking IOMMU
> > on data path for virtio. Thus iommu=on is out too.
> 
> IOMMUs on x86 usually come with an ACPI table that describes which
> IOMMUs are in the system and which devices they translate. So you can
> easily describe all devices there that are not behind an IOMMU.
> 
> The ACPI table is built by the BIOS, and the platform intialization code
> sets the device dma_ops accordingly. If the BIOS provides wrong
> information in the ACPI table this is a platform bug.

It doesn't look like I managed to put the point across.
My point is that IOMMU is required to do things like
userspace drivers, what we need is a way to express
"there is an IOMMU but it is part of device itself, use passthrough
 unless your driver is untrusted".

> > I'm not sure what ACPI has to do with it.  It's about a way for guest
> > users to specify whether they want to bypass an IOMMU for a given
> > device.
> 
> We have no way yet to request passthrough-mode per-device from the IOMMU
> drivers, but that can easily be added. But as I see it:
> 
> > By the way, a bunch of code is missing on the QEMU side
> > to make this useful:
> > 1. virtio ignores the iommu
> > 2. vhost user ignores the iommu
> > 3. dataplane ignores the iommu
> > 4. vhost-net ignores the iommu
> > 5. VFIO ignores the iommu
> 
> Qemu does not implement IOMMU translation for virtio devices anyway
> (which is fine), so it just should tell the guest so in the ACPI table
> built to describe the emulated IOMMU.
> 
> 
> 	Joerg

This is a short term limitation.

^ permalink raw reply

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Benjamin Herrenschmidt @ 2015-11-11  5:08 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: linux-s390, sparclinux, Joerg Roedel, KVM, Michael S. Tsirkin,
	linux-kernel@vger.kernel.org, Sebastian Ott, Christoph Hellwig,
	Christian Borntraeger, Martin Schwidefsky, Paolo Bonzini,
	Linux Virtualization, David Woodhouse, David S. Miller
In-Reply-To: <CALCETrWAVOATcCaUabunjYd+NYM1HRqJXG9K=DgrZCnBbWCexw@mail.gmail.com>

On Tue, 2015-11-10 at 20:46 -0800, Andy Lutomirski wrote:
> Me neither.  At least it wouldn't be a regression, but it's still
> crappy.
> 
> I think that arm is fine, at least.  I was unable to find an arm QEMU
> config that has any problems with my patches.

Ok, give me a few days for my headache & fever to subside see if I can
find something better. David, no idea from your side ? :-)

Cheers,
Ben.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Andy Lutomirski @ 2015-11-11  4:46 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linux-s390, sparclinux, Joerg Roedel, KVM, Michael S. Tsirkin,
	linux-kernel@vger.kernel.org, Sebastian Ott, Christoph Hellwig,
	Christian Borntraeger, Martin Schwidefsky, Paolo Bonzini,
	Linux Virtualization, David Woodhouse, David S. Miller
In-Reply-To: <1447202682.31884.123.camel@kernel.crashing.org>

On Nov 10, 2015 4:44 PM, "Benjamin Herrenschmidt"
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2015-11-10 at 15:44 -0800, Andy Lutomirski wrote:
> >
> > > What about partition <-> partition virtio such as what we could do on
> > > PAPR systems. That would have the weak barrier bit.
> > >
> >
> > Is it partition <-> partition, bypassing IOMMU?
>
> No.
>
> > I think I'd settle for just something that doesn't regress
> > non-experimental setups that actually work today and that allow new
> > setups (x86 with fixed QEMU and maybe something more complicated on
> > powerpc and/or sparc) to work in all cases.
> >
> > We could certainly just make powerpc and sparc continue bypassing the
> > IOMMU until someone comes up with a way to fix it.  I'll send out some
> > patches that do that, and maybe that'll help this make progress.
>
> But we haven't found a solution that works. All we have come up with is
> a quirk that will force bypass on virtio always and will not allow us
> to operate non-bypassing devices on either of those architectures in
> the future.
>
> I'm not too happy about this.

Me neither.  At least it wouldn't be a regression, but it's still crappy.

I think that arm is fine, at least.  I was unable to find an arm QEMU
config that has any problems with my patches.

--Andy

^ permalink raw reply

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Benjamin Herrenschmidt @ 2015-11-11  0:44 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: linux-s390, sparclinux, KVM, Michael S. Tsirkin, Sebastian Ott,
	linux-kernel@vger.kernel.org, Christoph Hellwig,
	Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
	Paolo Bonzini, Linux Virtualization, David Woodhouse,
	David S. Miller
In-Reply-To: <CALCETrWb9poeyxXhcbAbQkpcP-XYjWtz2w9iSPkKJH8rdzAj-A@mail.gmail.com>

On Tue, 2015-11-10 at 15:44 -0800, Andy Lutomirski wrote:
> 
> > What about partition <-> partition virtio such as what we could do on
> > PAPR systems. That would have the weak barrier bit.
> >
> 
> Is it partition <-> partition, bypassing IOMMU?

No.

> I think I'd settle for just something that doesn't regress
> non-experimental setups that actually work today and that allow new
> setups (x86 with fixed QEMU and maybe something more complicated on
> powerpc and/or sparc) to work in all cases.
> 
> We could certainly just make powerpc and sparc continue bypassing the
> IOMMU until someone comes up with a way to fix it.  I'll send out some
> patches that do that, and maybe that'll help this make progress.

But we haven't found a solution that works. All we have come up with is
a quirk that will force bypass on virtio always and will not allow us
to operate non-bypassing devices on either of those architectures in
the future.

I'm not too happy about this.

Ben.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* [PATCH] virtio_ring: Shadow available ring flags & index
From: Venkatesh Srinivas via Virtualization @ 2015-11-11  0:21 UTC (permalink / raw)
  To: virtualization; +Cc: kvm, mst, vsrinivas, luto, dmatlack, pbonzini, huawei.xie

Improves cacheline transfer flow of available ring header.

Virtqueues are implemented as a pair of rings, one producer->consumer
avail ring and one consumer->producer used ring; preceding the
avail ring in memory are two contiguous u16 fields -- avail->flags
and avail->idx. A producer posts work by writing to avail->idx and
a consumer reads avail->idx.

The flags and idx fields only need to be written by a producer CPU
and only read by a consumer CPU; when the producer and consumer are
running on different CPUs and the virtio_ring code is structured to
only have source writes/sink reads, we can continuously transfer the
avail header cacheline between 'M' states between cores. This flow
optimizes core -> core bandwidth on certain CPUs.

(see: "Software Optimization Guide for AMD Family 15h Processors",
Section 11.6; similar language appears in the 10h guide and should
apply to CPUs w/ exclusive caches, using LLC as a transfer cache)

Unfortunately the existing virtio_ring code issued reads to the
avail->idx and read-modify-writes to avail->flags on the producer.

This change shadows the flags and index fields in producer memory;
the vring code now reads from the shadows and only ever writes to
avail->flags and avail->idx, allowing the cacheline to transfer
core -> core optimally.

In a concurrent version of vring_bench, the time required for
10,000,000 buffer checkout/returns was reduced by ~2% (average
across many runs) on an AMD Piledriver (15h) CPU:

(w/o shadowing):
 Performance counter stats for './vring_bench':
     5,451,082,016      L1-dcache-loads
     ...
       2.221477739 seconds time elapsed

(w/ shadowing):
 Performance counter stats for './vring_bench':
     5,405,701,361      L1-dcache-loads
     ...
       2.168405376 seconds time elapsed

The further away (in a NUMA sense) virtio producers and consumers are
from each other, the more we expect to benefit. Physical implementations
of virtio devices and implementations of virtio where the consumer polls
vring avail indexes (vhost) should also benefit.

Signed-off-by: Venkatesh Srinivas <venkateshs@google.com>
---
 drivers/virtio/virtio_ring.c | 46 ++++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 096b857..6262015 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -80,6 +80,12 @@ struct vring_virtqueue {
 	/* Last used index we've seen. */
 	u16 last_used_idx;
 
+	/* Last written value to avail->flags */
+	u16 avail_flags_shadow;
+
+	/* Last written value to avail->idx in guest byte order */
+	u16 avail_idx_shadow;
+
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	bool (*notify)(struct virtqueue *vq);
 
@@ -235,13 +241,14 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 
 	/* Put entry in available array (but don't update avail->idx until they
 	 * do sync). */
-	avail = virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) & (vq->vring.num - 1);
+	avail = vq->avail_idx_shadow & (vq->vring.num - 1);
 	vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
 
 	/* Descriptors and available array need to be set before we expose the
 	 * new available array entries. */
 	virtio_wmb(vq->weak_barriers);
-	vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) + 1);
+	vq->avail_idx_shadow++;
+	vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
 	vq->num_added++;
 
 	pr_debug("Added buffer head %i to %p\n", head, vq);
@@ -354,8 +361,8 @@ bool virtqueue_kick_prepare(struct virtqueue *_vq)
 	 * event. */
 	virtio_mb(vq->weak_barriers);
 
-	old = virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) - vq->num_added;
-	new = virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx);
+	old = vq->avail_idx_shadow - vq->num_added;
+	new = vq->avail_idx_shadow;
 	vq->num_added = 0;
 
 #ifdef DEBUG
@@ -510,7 +517,7 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 	/* If we expect an interrupt for the next entry, tell host
 	 * by writing event index and flush out the write before
 	 * the read in the next get_buf call. */
-	if (!(vq->vring.avail->flags & cpu_to_virtio16(_vq->vdev, VRING_AVAIL_F_NO_INTERRUPT))) {
+	if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
 		vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, vq->last_used_idx);
 		virtio_mb(vq->weak_barriers);
 	}
@@ -537,7 +544,11 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
-	vq->vring.avail->flags |= cpu_to_virtio16(_vq->vdev, VRING_AVAIL_F_NO_INTERRUPT);
+	if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
+		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
+		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+	}
+
 }
 EXPORT_SYMBOL_GPL(virtqueue_disable_cb);
 
@@ -565,7 +576,10 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
 	/* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
 	 * either clear the flags bit or point the event index at the next
 	 * entry. Always do both to keep code simple. */
-	vq->vring.avail->flags &= cpu_to_virtio16(_vq->vdev, ~VRING_AVAIL_F_NO_INTERRUPT);
+	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
+		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
+		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+	}
 	vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx = vq->last_used_idx);
 	END_USE(vq);
 	return last_used_idx;
@@ -633,9 +647,12 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
 	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
 	 * either clear the flags bit or point the event index at the next
 	 * entry. Always do both to keep code simple. */
-	vq->vring.avail->flags &= cpu_to_virtio16(_vq->vdev, ~VRING_AVAIL_F_NO_INTERRUPT);
+	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
+		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
+		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
+	}
 	/* TODO: tune this threshold */
-	bufs = (u16)(virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) - vq->last_used_idx) * 3 / 4;
+	bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
 	vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, vq->last_used_idx + bufs);
 	virtio_mb(vq->weak_barriers);
 	if (unlikely((u16)(virtio16_to_cpu(_vq->vdev, vq->vring.used->idx) - vq->last_used_idx) > bufs)) {
@@ -670,7 +687,8 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
 		/* detach_buf clears data, so grab it now. */
 		buf = vq->data[i];
 		detach_buf(vq, i);
-		vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) - 1);
+		vq->avail_idx_shadow--;
+		vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
 		END_USE(vq);
 		return buf;
 	}
@@ -735,6 +753,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
 	vq->weak_barriers = weak_barriers;
 	vq->broken = false;
 	vq->last_used_idx = 0;
+	vq->avail_flags_shadow = 0;
+	vq->avail_idx_shadow = 0;
 	vq->num_added = 0;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 #ifdef DEBUG
@@ -746,8 +766,10 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
 	vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX);
 
 	/* No callback?  Tell other side not to bother us. */
-	if (!callback)
-		vq->vring.avail->flags |= cpu_to_virtio16(vdev, VRING_AVAIL_F_NO_INTERRUPT);
+	if (!callback) {
+		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
+		vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
+	}
 
 	/* Put everything in free lists. */
 	vq->free_head = 0;
-- 
2.6.0.rc2.230.g3dd15c0

^ permalink raw reply related

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Andy Lutomirski @ 2015-11-10 23:44 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linux-s390, sparclinux, KVM, Michael S. Tsirkin, Sebastian Ott,
	linux-kernel@vger.kernel.org, Christoph Hellwig,
	Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
	Paolo Bonzini, Linux Virtualization, David Woodhouse,
	David S. Miller
In-Reply-To: <1447194427.31884.100.camel@kernel.crashing.org>

On Tue, Nov 10, 2015 at 2:27 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2015-11-10 at 10:54 -0800, Andy Lutomirski wrote:
>>
>> Does that work on powerpc on existing kernels?
>>
>> Anyway, here's another crazy idea: make the quirk assume that the
>> IOMMU is bypasses if and only if the weak barriers bit is set on
>> systems that are missing the new DT binding.
>
> "New DT bindings" doesn't mean much ... how do we change DT bindings on
> existing machines with a FW in flash ?
>
> What about partition <-> partition virtio such as what we could do on
> PAPR systems. That would have the weak barrier bit.
>

Is it partition <-> partition, bypassing IOMMU?

I think I'd settle for just something that doesn't regress
non-experimental setups that actually work today and that allow new
setups (x86 with fixed QEMU and maybe something more complicated on
powerpc and/or sparc) to work in all cases.

We could certainly just make powerpc and sparc continue bypassing the
IOMMU until someone comes up with a way to fix it.  I'll send out some
patches that do that, and maybe that'll help this make progress.

--Andy

^ permalink raw reply

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Benjamin Herrenschmidt @ 2015-11-10 22:27 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: linux-s390, sparclinux, KVM, Michael S. Tsirkin, Sebastian Ott,
	linux-kernel@vger.kernel.org, Christoph Hellwig,
	Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
	Paolo Bonzini, Linux Virtualization, David Woodhouse,
	David S. Miller
In-Reply-To: <CALCETrVkE3=KCxK-_bf-P8UfTS96Vck+QPE8EsLvP7THxpLe1A@mail.gmail.com>

On Tue, 2015-11-10 at 10:54 -0800, Andy Lutomirski wrote:
> 
> Does that work on powerpc on existing kernels?
> 
> Anyway, here's another crazy idea: make the quirk assume that the
> IOMMU is bypasses if and only if the weak barriers bit is set on
> systems that are missing the new DT binding.

"New DT bindings" doesn't mean much ... how do we change DT bindings on
existing machines with a FW in flash ?

What about partition <-> partition virtio such as what we could do on
PAPR systems. That would have the weak barrier bit.

Ben.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Benjamin Herrenschmidt @ 2015-11-10 19:37 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Joerg Roedel, KVM, linux-s390, Sebastian Ott,
	linux-kernel@vger.kernel.org, Andy Lutomirski,
	Christian Borntraeger, Christoph Hellwig, Andy Lutomirski,
	sparclinux, Paolo Bonzini, Linux Virtualization, David Woodhouse,
	David S. Miller, Martin Schwidefsky
In-Reply-To: <20151110142633-mutt-send-email-mst@redhat.com>

On Tue, 2015-11-10 at 14:43 +0200, Michael S. Tsirkin wrote:
> But not virtio-pci I think - that's broken for that usecase since we use
> weaker barriers than required for real IO, as these have measureable
> overhead.  We could have a feature "is a real PCI device",
> that's completely reasonable.

Do we use weaker barriers on the Linux driver side ? I didn't think so
... 

Cheers,
Ben.

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Benjamin Herrenschmidt @ 2015-11-10 19:36 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: linux-s390, KVM, Michael S. Tsirkin, Sebastian Ott,
	linux-kernel@vger.kernel.org, Andy Lutomirski,
	Christian Borntraeger, Christoph Hellwig, Andy Lutomirski,
	sparclinux, Paolo Bonzini, Linux Virtualization, David Woodhouse,
	David S. Miller, Martin Schwidefsky
In-Reply-To: <20151110102733.GJ2255@suse.de>

On Tue, 2015-11-10 at 11:27 +0100, Joerg Roedel wrote:
> 
> You have the same problem when real PCIe devices appear that speak
> virtio. I think the only real (still not very nice) solution is to add a
> quirk to powerpc platform code that sets noop dma-ops for the existing
> virtio vendor/device-ids and add a DT property to opt-out of that quirk.
>
> New vendor/device-ids (as for real devices) would just not be covered by
> the quirk and existing emulated devices continue to work.

Why woud real devices use new vendor/device IDs ? Also there are other
cases such as using virtio between 2 partitions, which we could do
under PowerVM ... that would require proper iommu usage with existing
IDs.

> The absence of the property just means that the quirk is in place and
> the system assumes no translation for virtio devices.

The only way that works forward for me (and possibly sparc & others,
what about ARM ?) is if we *change* something in virtio qemu at the
same time as we add some kind of property. For example the ProgIf field
or revision ID field.

That way I can key on that change.

It's still tricky because I would have to somewhat tell my various firmwares
(SLOF, OpenBIOS, OPAL, ...) so they can create the appropriate property, it's
still hacky, but it would be workable.

Ben.

^ permalink raw reply

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Andy Lutomirski @ 2015-11-10 18:54 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linux-s390, sparclinux, KVM, Michael S. Tsirkin, Sebastian Ott,
	linux-kernel@vger.kernel.org, Christoph Hellwig,
	Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
	Paolo Bonzini, Linux Virtualization, David Woodhouse,
	David S. Miller
In-Reply-To: <1447151874.31884.82.camel@kernel.crashing.org>

On Nov 10, 2015 2:38 AM, "Benjamin Herrenschmidt"
<benh@kernel.crashing.org> wrote:
>
> On Mon, 2015-11-09 at 21:35 -0800, Andy Lutomirski wrote:
> >
> > We could do it the other way around: on powerpc, if a PCI device is in
> > that range and doesn't have the "bypass" property at all, then it's
> > assumed to bypass the IOMMU.  This means that everything that
> > currently works continues working.  If someone builds a physical
> > virtio device or uses another system in PCIe target mode speaking
> > virtio, then it won't work until they upgrade their firmware to set
> > bypass=0.  Meanwhile everyone using hypothetical new QEMU also gets
> > bypass=0 and no ambiguity.
> >
> > vfio will presumably notice the bypass and correctly refuse to map any
> > current virtio devices.
> >
> > Would that work?
>
> That would be extremely strange from a platform perspective. Any device
> in that vendor/device range would bypass the iommu unless some new
> property "actually-works-like-a-real-pci-device" happens to exist in
> the device-tree, which we would then need to define somewhere and
> handle accross at least 3 different platforms who get their device-tree
> from widly different places.
>
> Also if tomorrow I create a PCI device that implements virtio-net and
> put it in a machine running IBM proprietary firmware (or Apple's or
> Sun's), it won't have that property...
>
> This is not hypothetical. People are using virtio to do point-to-point
> communication between machines via PCIe today.

Does that work on powerpc on existing kernels?

Anyway, here's another crazy idea: make the quirk assume that the
IOMMU is bypasses if and only if the weak barriers bit is set on
systems that are missing the new DT binding.

--Andy

>
> Cheers,
> Ben.
>
>

^ permalink raw reply

* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Andy Lutomirski @ 2015-11-10 18:54 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
	linux-kernel@vger.kernel.org, Linux Virtualization,
	Christian Borntraeger, Joerg Roedel, Martin Schwidefsky,
	Paolo Bonzini, David Woodhouse, Christoph Hellwig
In-Reply-To: <20151109224720-mutt-send-email-mst@redhat.com>

On Nov 10, 2015 7:02 AM, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> On Sun, Nov 08, 2015 at 12:49:46PM +0100, Joerg Roedel wrote:
> > On Sun, Nov 08, 2015 at 12:37:47PM +0200, Michael S. Tsirkin wrote:
> > > I have no problem with that. For example, can we teach
> > > the DMA API on intel x86 to use PT for virtio by default?
> > > That would allow merging Andy's patches with
> > > full compatibility with old guests and hosts.
> >
> > Well, the only incompatibility comes from an experimental qemu feature,
> > more explicitly from a bug in that features implementation. So why
> > should we work around that in the kernel? I think it is not too hard to
> > fix qemu to generate a correct DMAR table which excludes the virtio
> > devices from iommu translation.
> >
> >
> >       Joerg
>
> It's not that easy - you'd have to dedicate some buses
> for iommu bypass, and teach management tools to only put
> virtio there - but it's possible.
>
> This will absolutely address guests that don't need to set up IOMMU for
> virtio devices, and virtio that bypasses the IOMMU.
>
> But the problem is that we do want to *allow* guests
> to set up IOMMU for virtio devices.
> In that case, these are two other usecases:
>
> A- monolitic virtio within QEMU:
>         iommu only needed for VFIO ->
>         guest should always use iommu=pt
>         iommu=on works but is just useless overhead.
>
> B- modular out of process virtio outside QEMU:
>         iommu needed for VFIO or kernel driver ->
>         guest should use iommu=pt or iommu=on
>         depending on security/performance requirements
>
> Note that there could easily be a mix of these in the same system.
>
> So for these cases we do need QEMU to specify to guest that IOMMU covers
> the virtio devices.  Also, once one does this, the default on linux is
> iommu=on and not pt, which works but ATM is very slow.
>
> This poses three problems:
>
> 1. How do we address the different needs of A and B?
>    One way would be for virtio to pass the information to guest
>    using some virtio specific way, and have drivers
>    specify what kind of DMA access they want.
>
> 2. (Kind of a subset of 1) once we do allow IOMMU, how do we make sure most guests
>    use the more sensible iommu=pt.
>
> 3. Once we do allow IOMMU, how can we keep existing guests work in this configuration?
>    Creating different hypervisor configurations depending on guest is very nasty.
>    Again, one way would be some virtio specific interface.
>
> I'd rather we figured the answers to this before merging Andy's patches
> because I'm concerned that instead of 1 broken configuration
> (virtio always bypasses IOMMU) we'll get two bad configurations
> (in the second one, virtio uses the slow default with no
> gain in security).
>
> Suggestions wellcome.

I think there's still no downside of using my patches, even on x86.

Old kernels on new QEMU work unless IOMMU is enabled on the host.  I
think that's the best we can possibly do.

New kernels work at full speed on old QEMU.

New kernels with new QEMU and iommu enabled work slower.  Even newer
kernels with default passthrough work at full speed, and there's no
obvious downside to the existence of kernels with just my patches.

--Andy

>
> --
> MST

^ permalink raw reply

* Re: [PATCH v3 0/3] virtio DMA API core stuff
From: Michael S. Tsirkin @ 2015-11-10 15:02 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: linux-s390, KVM, Benjamin Herrenschmidt, Sebastian Ott,
	linux-kernel@vger.kernel.org, Andy Lutomirski,
	Christian Borntraeger, Andy Lutomirski, Paolo Bonzini,
	Linux Virtualization, David Woodhouse, Christoph Hellwig,
	Martin Schwidefsky
In-Reply-To: <20151108114946.GG2255@suse.de>

On Sun, Nov 08, 2015 at 12:49:46PM +0100, Joerg Roedel wrote:
> On Sun, Nov 08, 2015 at 12:37:47PM +0200, Michael S. Tsirkin wrote:
> > I have no problem with that. For example, can we teach
> > the DMA API on intel x86 to use PT for virtio by default?
> > That would allow merging Andy's patches with
> > full compatibility with old guests and hosts.
> 
> Well, the only incompatibility comes from an experimental qemu feature,
> more explicitly from a bug in that features implementation. So why
> should we work around that in the kernel? I think it is not too hard to
> fix qemu to generate a correct DMAR table which excludes the virtio
> devices from iommu translation.
> 
> 
> 	Joerg

It's not that easy - you'd have to dedicate some buses
for iommu bypass, and teach management tools to only put
virtio there - but it's possible.

This will absolutely address guests that don't need to set up IOMMU for
virtio devices, and virtio that bypasses the IOMMU.

But the problem is that we do want to *allow* guests
to set up IOMMU for virtio devices.
In that case, these are two other usecases:

A- monolitic virtio within QEMU:
	iommu only needed for VFIO ->
	guest should always use iommu=pt
        iommu=on works but is just useless overhead.

B- modular out of process virtio outside QEMU:
	iommu needed for VFIO or kernel driver ->
	guest should use iommu=pt or iommu=on
	depending on security/performance requirements

Note that there could easily be a mix of these in the same system.

So for these cases we do need QEMU to specify to guest that IOMMU covers
the virtio devices.  Also, once one does this, the default on linux is
iommu=on and not pt, which works but ATM is very slow.

This poses three problems:

1. How do we address the different needs of A and B?
   One way would be for virtio to pass the information to guest
   using some virtio specific way, and have drivers
   specify what kind of DMA access they want.

2. (Kind of a subset of 1) once we do allow IOMMU, how do we make sure most guests
   use the more sensible iommu=pt.

3. Once we do allow IOMMU, how can we keep existing guests work in this configuration?
   Creating different hypervisor configurations depending on guest is very nasty.
   Again, one way would be some virtio specific interface.

I'd rather we figured the answers to this before merging Andy's patches
because I'm concerned that instead of 1 broken configuration
(virtio always bypasses IOMMU) we'll get two bad configurations
(in the second one, virtio uses the slow default with no
gain in security).

Suggestions wellcome.

-- 
MST

^ permalink raw reply

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Michael S. Tsirkin @ 2015-11-10 12:43 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Joerg Roedel, KVM, linux-s390, Sebastian Ott,
	linux-kernel@vger.kernel.org, Andy Lutomirski,
	Christian Borntraeger, Christoph Hellwig, Andy Lutomirski,
	sparclinux, Paolo Bonzini, Linux Virtualization, David Woodhouse,
	David S. Miller, Martin Schwidefsky
In-Reply-To: <1447151874.31884.82.camel@kernel.crashing.org>

On Tue, Nov 10, 2015 at 09:37:54PM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2015-11-09 at 21:35 -0800, Andy Lutomirski wrote:
> > 
> > We could do it the other way around: on powerpc, if a PCI device is in
> > that range and doesn't have the "bypass" property at all, then it's
> > assumed to bypass the IOMMU.  This means that everything that
> > currently works continues working.  If someone builds a physical
> > virtio device or uses another system in PCIe target mode speaking
> > virtio, then it won't work until they upgrade their firmware to set
> > bypass=0.  Meanwhile everyone using hypothetical new QEMU also gets
> > bypass=0 and no ambiguity.
> >
> > vfio will presumably notice the bypass and correctly refuse to map any
> > current virtio devices.
> > 
> > Would that work?
> 
> That would be extremely strange from a platform perspective. Any device
> in that vendor/device range would bypass the iommu unless some new
> property "actually-works-like-a-real-pci-device" happens to exist in
> the device-tree, which we would then need to define somewhere and
> handle accross at least 3 different platforms who get their device-tree 
> from widly different places.

Then we are back to virtio driver telling DMA core
whether it wants a 1:1 mapping in the iommu?
If that's acceptable to others, I don't think that's too bad.


> Also if tomorrow I create a PCI device that implements virtio-net and
> put it in a machine running IBM proprietary firmware (or Apple's or
> Sun's), it won't have that property...
> 
> This is not hypothetical. People are using virtio to do point-to-point
> communication between machines via PCIe today.
> 
> Cheers,
> Ben.

But not virtio-pci I think - that's broken for that usecase since we use
weaker barriers than required for real IO, as these have measureable
overhead.  We could have a feature "is a real PCI device",
that's completely reasonable.

-- 
MST

^ permalink raw reply

* Re: [PATCH v4 0/6] virtio core DMA API conversion
From: Benjamin Herrenschmidt @ 2015-11-10 10:37 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Joerg Roedel, KVM, linux-s390, Michael S. Tsirkin, Sebastian Ott,
	linux-kernel@vger.kernel.org, Christoph Hellwig,
	Christian Borntraeger, Andy Lutomirski, sparclinux, Paolo Bonzini,
	Linux Virtualization, David Woodhouse, David S. Miller,
	Martin Schwidefsky
In-Reply-To: <CALCETrVPQc04Ah7FXcRMb4RhpUJ1WPCoC_4dbacB8a+u5XpmwA@mail.gmail.com>

On Mon, 2015-11-09 at 21:35 -0800, Andy Lutomirski wrote:
> 
> We could do it the other way around: on powerpc, if a PCI device is in
> that range and doesn't have the "bypass" property at all, then it's
> assumed to bypass the IOMMU.  This means that everything that
> currently works continues working.  If someone builds a physical
> virtio device or uses another system in PCIe target mode speaking
> virtio, then it won't work until they upgrade their firmware to set
> bypass=0.  Meanwhile everyone using hypothetical new QEMU also gets
> bypass=0 and no ambiguity.
>
> vfio will presumably notice the bypass and correctly refuse to map any
> current virtio devices.
> 
> Would that work?

That would be extremely strange from a platform perspective. Any device
in that vendor/device range would bypass the iommu unless some new
property "actually-works-like-a-real-pci-device" happens to exist in
the device-tree, which we would then need to define somewhere and
handle accross at least 3 different platforms who get their device-tree 
from widly different places.

Also if tomorrow I create a PCI device that implements virtio-net and
put it in a machine running IBM proprietary firmware (or Apple's or
Sun's), it won't have that property...

This is not hypothetical. People are using virtio to do point-to-point
communication between machines via PCIe today.

Cheers,
Ben.


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox