[Qemu-devel] [PATCH-v2 0/3] virtio/vhost: Add checks for uninitialized VQs

[Qemu-devel] [PATCH-v2 0/3] virtio/vhost: Add checks for uninitialized VQs

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi folks,

This series adds a virtio_queue_valid() for use by virtio-pci code in
order to prevent opreations upon uninitialized VQs, which is currently
expected to occur during seabios setup of virtio-scsi with in-flight
vhost-scsi-pci device code.

On the vhost side, it also adds virtio_queue_valid() sanity checks in
vhost_virtqueue_[start,stop]() and vhost_verify_ring_mappings() in order
to skip the same uninitialized VQs.

Changes from v1:
  - Remove now unnecessary virtio_queue_get_num() calls in virtio-pci.c
  - Add virtio_queue_valid() calls in vhost_virtqueue_[start,stop]()

Please review.

--nab

Michael S. Tsirkin (1):
  virtio: add API to check that ring is setup

Nicholas Bellinger (2):
  virtio-pci: Add virtio_queue_valid checks ahead of
    virtio_queue_get_num
  vhost: Skip uninitialized VQs in vhost_virtqueue_[start,stop]

 hw/vhost.c      |   12 ++++++++++++
 hw/virtio-pci.c |   34 +++++++++++++++-------------------
 hw/virtio.c     |    5 +++++
 hw/virtio.h     |    1 +
 4 files changed, 33 insertions(+), 19 deletions(-)

-- 
1.7.2.5

[Qemu-devel] [PATCH-v2 1/3] virtio: add API to check that ring is setup

[Nicholas A. Bellinger]

From: Michael S. Tsirkin <mst@redhat.com>

virtio scsi makes it legal to only setup a subset of rings.  The only
way to detect the ring is setup seems to be to check whether PA was
written to.  Add API to do this, and teach code to use it instead of
checking hardware queue size.

(nab: use .vring.desc instead of .vring.pa)

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Asias He <asias@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 hw/virtio.c |    5 +++++
 hw/virtio.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/hw/virtio.c b/hw/virtio.c
index 26fbc79..65ba253 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -651,6 +651,11 @@ int virtio_queue_get_num(VirtIODevice *vdev, int n)
     return vdev->vq[n].vring.num;
 }
 
+bool virtio_queue_valid(VirtIODevice *vdev, int n)
+{
+    return vdev->vq[n].vring.num && vdev->vq[n].vring.desc;
+}
+
 int virtio_queue_get_id(VirtQueue *vq)
 {
     VirtIODevice *vdev = vq->vdev;
diff --git a/hw/virtio.h b/hw/virtio.h
index fdbe931..3086798 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -227,6 +227,7 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data);
 void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
 hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
 int virtio_queue_get_num(VirtIODevice *vdev, int n);
+bool virtio_queue_valid(VirtIODevice *vdev, int n);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
-- 
1.7.2.5

[Qemu-devel] [PATCH-v2 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a number of virtio_queue_valid() checks to virtio-pci
ahead of virtio_queue_get_num() usage in order to skip operation upon
the detection of an uninitialized VQ.

There is one exception in virtio_ioport_read():VIRTIO_PCI_QUEUE_NUM,
where virtio_queue_get_num() may still be called without a valid
vdev->vq[n].vring.desc physical address.

v2: Drop now unnecessary virtio_queue_get_num calls (mst)

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Asias He <asias@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 hw/virtio-pci.c |   34 +++++++++++++++-------------------
 1 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 0d67b84..1369d9a 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -211,10 +211,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy)
     }
 
     for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
-        if (!virtio_queue_get_num(proxy->vdev, n)) {
+        if (!virtio_queue_valid(proxy->vdev, n)) {
             continue;
         }
-
         r = virtio_pci_set_host_notifier_internal(proxy, n, true, true);
         if (r < 0) {
             goto assign_error;
@@ -225,10 +224,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy)
 
 assign_error:
     while (--n >= 0) {
-        if (!virtio_queue_get_num(proxy->vdev, n)) {
+        if (!virtio_queue_valid(proxy->vdev, n)) {
             continue;
         }
-
         r = virtio_pci_set_host_notifier_internal(proxy, n, false, false);
         assert(r >= 0);
     }
@@ -246,10 +244,9 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
     }
 
     for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
-        if (!virtio_queue_get_num(proxy->vdev, n)) {
+        if (!virtio_queue_valid(proxy->vdev, n)) {
             continue;
         }
-
         r = virtio_pci_set_host_notifier_internal(proxy, n, false, false);
         assert(r >= 0);
     }
@@ -546,8 +543,8 @@ static int kvm_virtio_pci_vector_use(VirtIOPCIProxy *proxy, int nvqs)
     MSIMessage msg;
 
     for (queue_no = 0; queue_no < nvqs; queue_no++) {
-        if (!virtio_queue_get_num(vdev, queue_no)) {
-            break;
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
         }
         vector = virtio_queue_vector(vdev, queue_no);
         if (vector >= msix_nr_vectors_allocated(dev)) {
@@ -593,8 +590,8 @@ static void kvm_virtio_pci_vector_release(VirtIOPCIProxy *proxy, int nvqs)
     int queue_no;
 
     for (queue_no = 0; queue_no < nvqs; queue_no++) {
-        if (!virtio_queue_get_num(vdev, queue_no)) {
-            break;
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
         }
         vector = virtio_queue_vector(vdev, queue_no);
         if (vector >= msix_nr_vectors_allocated(dev)) {
@@ -665,8 +662,8 @@ static int kvm_virtio_pci_vector_unmask(PCIDevice *dev, unsigned vector,
     int ret, queue_no;
 
     for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers; queue_no++) {
-        if (!virtio_queue_get_num(vdev, queue_no)) {
-            break;
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
         }
         if (virtio_queue_vector(vdev, queue_no) != vector) {
             continue;
@@ -695,8 +692,8 @@ static void kvm_virtio_pci_vector_mask(PCIDevice *dev, unsigned vector)
     int queue_no;
 
     for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers; queue_no++) {
-        if (!virtio_queue_get_num(vdev, queue_no)) {
-            break;
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
         }
         if (virtio_queue_vector(vdev, queue_no) != vector) {
             continue;
@@ -717,8 +714,8 @@ static void kvm_virtio_pci_vector_poll(PCIDevice *dev,
     VirtQueue *vq;
 
     for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers; queue_no++) {
-        if (!virtio_queue_get_num(vdev, queue_no)) {
-            break;
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
         }
         vector = virtio_queue_vector(vdev, queue_no);
         if (vector < vector_start || vector >= vector_end ||
@@ -790,10 +787,9 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
     }
 
     for (n = 0; n < nvqs; n++) {
-        if (!virtio_queue_get_num(vdev, n)) {
-            break;
+        if (!virtio_queue_valid(vdev, n)) {
+            continue;
         }
-
         r = virtio_pci_set_guest_notifier(d, n, assign,
                                           kvm_msi_via_irqfd_enabled());
         if (r < 0) {
-- 
1.7.2.5

[Qemu-devel] [PATCH-v2 3/3] vhost: Skip uninitialized VQs in vhost_virtqueue_[start, stop]

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds virtio_queue_valid() checks in vhost_virtqueue_start()
and vhost_virtqueue_stop() to avoid uninitialized VQs during vhost-scsi-pci
seabios operation, where we currently expect only the request VQ to have
been initialized before virtio-scsi LLD guest hand-off.

Also, go ahead and skip the same uninitialized VQs during sanity checks
within vhost_verify_ring_mappings() by checking vq->ring_[phys,size]
directly.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Asias He <asias@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 hw/vhost.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/hw/vhost.c b/hw/vhost.c
index 4d6aee3..832cc89 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -314,6 +314,9 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
         hwaddr l;
         void *p;
 
+        if (!vq->ring_phys || !vq->ring_size) {
+            continue;
+        }
         if (!ranges_overlap(start_addr, size, vq->ring_phys, vq->ring_size)) {
             continue;
         }
@@ -645,6 +648,10 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
 
     assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
 
+    if (!virtio_queue_valid(vdev, idx)) {
+        return 0;
+    }
+
     vq->num = state.num = virtio_queue_get_num(vdev, idx);
     r = ioctl(dev->control, VHOST_SET_VRING_NUM, &state);
     if (r) {
@@ -732,6 +739,11 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
     };
     int r;
     assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
+
+    if (!virtio_queue_valid(vdev, idx)) {
+        return;
+    }
+
     r = ioctl(dev->control, VHOST_GET_VRING_BASE, &state);
     if (r < 0) {
         fprintf(stderr, "vhost VQ %d ring restore failed: %d\n", idx, r);
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH-v2 0/3] virtio/vhost: Add checks for uninitialized VQs

[Michael S. Tsirkin]

On Mon, Apr 01, 2013 at 11:58:21PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi folks,
> 
> This series adds a virtio_queue_valid() for use by virtio-pci code in
> order to prevent opreations upon uninitialized VQs, which is currently
> expected to occur during seabios setup of virtio-scsi with in-flight
> vhost-scsi-pci device code.
> 
> On the vhost side, it also adds virtio_queue_valid() sanity checks in
> vhost_virtqueue_[start,stop]() and vhost_verify_ring_mappings() in order
> to skip the same uninitialized VQs.
> 
> Changes from v1:
>   - Remove now unnecessary virtio_queue_get_num() calls in virtio-pci.c
>   - Add virtio_queue_valid() calls in vhost_virtqueue_[start,stop]()
> 
> Please review.
> 
> --nab

Looks reasonable.
Acked-by: Michael S. Tsirkin <mst@redhat.com>

So - does this fix the issues you saw with vhost-scsi?

> Michael S. Tsirkin (1):
>   virtio: add API to check that ring is setup
> 
> Nicholas Bellinger (2):
>   virtio-pci: Add virtio_queue_valid checks ahead of
>     virtio_queue_get_num
>   vhost: Skip uninitialized VQs in vhost_virtqueue_[start,stop]
> 
>  hw/vhost.c      |   12 ++++++++++++
>  hw/virtio-pci.c |   34 +++++++++++++++-------------------
>  hw/virtio.c     |    5 +++++
>  hw/virtio.h     |    1 +
>  4 files changed, 33 insertions(+), 19 deletions(-)
> 
> -- 
> 1.7.2.5

Re: [Qemu-devel] [PATCH-v2 0/3] virtio/vhost: Add checks for uninitialized VQs

[Nicholas A. Bellinger]

On Tue, 2013-04-02 at 15:01 +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 01, 2013 at 11:58:21PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > Hi folks,
> > 
> > This series adds a virtio_queue_valid() for use by virtio-pci code in
> > order to prevent opreations upon uninitialized VQs, which is currently
> > expected to occur during seabios setup of virtio-scsi with in-flight
> > vhost-scsi-pci device code.
> > 
> > On the vhost side, it also adds virtio_queue_valid() sanity checks in
> > vhost_virtqueue_[start,stop]() and vhost_verify_ring_mappings() in order
> > to skip the same uninitialized VQs.
> > 
> > Changes from v1:
> >   - Remove now unnecessary virtio_queue_get_num() calls in virtio-pci.c
> >   - Add virtio_queue_valid() calls in vhost_virtqueue_[start,stop]()
> > 
> > Please review.
> > 
> > --nab
> 
> Looks reasonable.
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 

Thanks MST!

Anthony, do you want to pick these up now..?  Or shall I include in the
next vhost-scsi-pci PATCH-v3 series..?

--nab

> So - does this fix the issues you saw with vhost-scsi?
> 
> > Michael S. Tsirkin (1):
> >   virtio: add API to check that ring is setup
> > 
> > Nicholas Bellinger (2):
> >   virtio-pci: Add virtio_queue_valid checks ahead of
> >     virtio_queue_get_num
> >   vhost: Skip uninitialized VQs in vhost_virtqueue_[start,stop]
> > 
> >  hw/vhost.c      |   12 ++++++++++++
> >  hw/virtio-pci.c |   34 +++++++++++++++-------------------
> >  hw/virtio.c     |    5 +++++
> >  hw/virtio.h     |    1 +
> >  4 files changed, 33 insertions(+), 19 deletions(-)
> > 
> > -- 
> > 1.7.2.5
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html