Nicholas A. Bellinger
2013-Apr-01 23:58 UTC
[PATCH-v2 0/3] virtio/vhost: Add checks for uninitialized VQs
From: Nicholas Bellinger <nab at 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
Nicholas A. Bellinger
2013-Apr-01 23:58 UTC
[PATCH-v2 1/3] virtio: add API to check that ring is setup
From: Michael S. Tsirkin <mst at 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 at redhat.com> Cc: Asias He <asias at redhat.com> Cc: Paolo Bonzini <pbonzini at redhat.com> Signed-off-by: Nicholas Bellinger <nab at 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
Nicholas A. Bellinger
2013-Apr-01 23:58 UTC
[PATCH-v2 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num
From: Nicholas Bellinger <nab at 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 at redhat.com> Cc: Asias He <asias at redhat.com> Cc: Paolo Bonzini <pbonzini at redhat.com> Signed-off-by: Nicholas Bellinger <nab at 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
Nicholas A. Bellinger
2013-Apr-01 23:58 UTC
[PATCH-v2 3/3] vhost: Skip uninitialized VQs in vhost_virtqueue_[start, stop]
From: Nicholas Bellinger <nab at 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 at redhat.com> Cc: Asias He <asias at redhat.com> Cc: Paolo Bonzini <pbonzini at redhat.com> Signed-off-by: Nicholas Bellinger <nab at 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
Michael S. Tsirkin
2013-Apr-02 12:01 UTC
[PATCH-v2 0/3] virtio/vhost: Add checks for uninitialized VQs
On Mon, Apr 01, 2013 at 11:58:21PM +0000, Nicholas A. Bellinger wrote:> From: Nicholas Bellinger <nab at 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. > > --nabLooks reasonable. Acked-by: Michael S. Tsirkin <mst at 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
Nicholas A. Bellinger
2013-Apr-02 23:16 UTC
[PATCH-v2 0/3] virtio/vhost: Add checks for uninitialized VQs
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 at 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 at 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 at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html