* [PATCH 1/3] virtio: add API to check that ring is setup [not found] <1364531592-8368-1-git-send-email-nab@linux-iscsi.org> @ 2013-03-29 4:33 ` Nicholas A. Bellinger 2013-03-29 4:33 ` [PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num Nicholas A. Bellinger ` (4 subsequent siblings) 5 siblings, 0 replies; 9+ messages in thread From: Nicholas A. Bellinger @ 2013-03-29 4:33 UTC (permalink / raw) To: target-devel Cc: kvm-devel, Michael S. Tsirkin, qemu-devel, lf-virt, Anthony Liguori, Stefan Hajnoczi, Paolo Bonzini 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num [not found] <1364531592-8368-1-git-send-email-nab@linux-iscsi.org> 2013-03-29 4:33 ` [PATCH 1/3] virtio: add API to check that ring is setup Nicholas A. Bellinger @ 2013-03-29 4:33 ` Nicholas A. Bellinger 2013-03-31 7:37 ` Michael S. Tsirkin [not found] ` <20130331073727.GB23484@redhat.com> 2013-03-29 4:33 ` [PATCH 3/3] vhost: Check+skip uninitialized VQs in vhost_verify_ring_mappings Nicholas A. Bellinger ` (3 subsequent siblings) 5 siblings, 2 replies; 9+ messages in thread From: Nicholas A. Bellinger @ 2013-03-29 4:33 UTC (permalink / raw) To: target-devel Cc: kvm-devel, Michael S. Tsirkin, qemu-devel, lf-virt, Anthony Liguori, Stefan Hajnoczi, Paolo Bonzini 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. 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 | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-) diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 0d67b84..231ca0c 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -211,6 +211,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy) } for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) { + if (!virtio_queue_valid(proxy->vdev, n)) { + continue; + } if (!virtio_queue_get_num(proxy->vdev, n)) { continue; } @@ -225,6 +228,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy) assign_error: while (--n >= 0) { + if (!virtio_queue_valid(proxy->vdev, n)) { + continue; + } if (!virtio_queue_get_num(proxy->vdev, n)) { continue; } @@ -246,6 +252,9 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy) } for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) { + if (!virtio_queue_valid(proxy->vdev, n)) { + continue; + } if (!virtio_queue_get_num(proxy->vdev, n)) { continue; } @@ -546,6 +555,9 @@ 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_valid(vdev, queue_no)) { + continue; + } if (!virtio_queue_get_num(vdev, queue_no)) { break; } @@ -593,6 +605,9 @@ 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_valid(vdev, queue_no)) { + continue; + } if (!virtio_queue_get_num(vdev, queue_no)) { break; } @@ -665,6 +680,9 @@ 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_valid(vdev, queue_no)) { + continue; + } if (!virtio_queue_get_num(vdev, queue_no)) { break; } @@ -695,6 +713,9 @@ 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_valid(vdev, queue_no)) { + continue; + } if (!virtio_queue_get_num(vdev, queue_no)) { break; } @@ -717,6 +738,9 @@ 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_valid(vdev, queue_no)) { + continue; + } if (!virtio_queue_get_num(vdev, queue_no)) { break; } @@ -790,6 +814,9 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign) } for (n = 0; n < nvqs; n++) { + if (!virtio_queue_valid(vdev, n)) { + continue; + } if (!virtio_queue_get_num(vdev, n)) { break; } -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num 2013-03-29 4:33 ` [PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num Nicholas A. Bellinger @ 2013-03-31 7:37 ` Michael S. Tsirkin [not found] ` <20130331073727.GB23484@redhat.com> 1 sibling, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2013-03-31 7:37 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: kvm-devel, qemu-devel, lf-virt, Anthony Liguori, target-devel, Stefan Hajnoczi, Paolo Bonzini On Fri, Mar 29, 2013 at 04:33:11AM +0000, Nicholas A. Bellinger wrote: > 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. > > 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> Makes sense. Minor nit: virtio_queue_valid calls virtio_queue_get_num internally, so we can drop it everywhere we know queue is valid. > --- > hw/virtio-pci.c | 27 +++++++++++++++++++++++++++ > 1 files changed, 27 insertions(+), 0 deletions(-) > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 0d67b84..231ca0c 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -211,6 +211,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy) > } > > for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) { > + if (!virtio_queue_valid(proxy->vdev, n)) { > + continue; > + } > if (!virtio_queue_get_num(proxy->vdev, n)) { > continue; > } > @@ -225,6 +228,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy) > > assign_error: > while (--n >= 0) { > + if (!virtio_queue_valid(proxy->vdev, n)) { > + continue; > + } > if (!virtio_queue_get_num(proxy->vdev, n)) { > continue; > } > @@ -246,6 +252,9 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy) > } > > for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) { > + if (!virtio_queue_valid(proxy->vdev, n)) { > + continue; > + } > if (!virtio_queue_get_num(proxy->vdev, n)) { > continue; > } > @@ -546,6 +555,9 @@ 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_valid(vdev, queue_no)) { > + continue; > + } > if (!virtio_queue_get_num(vdev, queue_no)) { > break; > } > @@ -593,6 +605,9 @@ 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_valid(vdev, queue_no)) { > + continue; > + } > if (!virtio_queue_get_num(vdev, queue_no)) { > break; > } > @@ -665,6 +680,9 @@ 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_valid(vdev, queue_no)) { > + continue; > + } > if (!virtio_queue_get_num(vdev, queue_no)) { > break; > } > @@ -695,6 +713,9 @@ 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_valid(vdev, queue_no)) { > + continue; > + } > if (!virtio_queue_get_num(vdev, queue_no)) { > break; > } > @@ -717,6 +738,9 @@ 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_valid(vdev, queue_no)) { > + continue; > + } > if (!virtio_queue_get_num(vdev, queue_no)) { > break; > } > @@ -790,6 +814,9 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign) > } > > for (n = 0; n < nvqs; n++) { > + if (!virtio_queue_valid(vdev, n)) { > + continue; > + } > if (!virtio_queue_get_num(vdev, n)) { > break; > } > -- > 1.7.2.5 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <20130331073727.GB23484@redhat.com>]
* Re: [PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num [not found] ` <20130331073727.GB23484@redhat.com> @ 2013-04-01 23:16 ` Nicholas A. Bellinger 0 siblings, 0 replies; 9+ messages in thread From: Nicholas A. Bellinger @ 2013-04-01 23:16 UTC (permalink / raw) To: Michael S. Tsirkin Cc: kvm-devel, qemu-devel, lf-virt, Anthony Liguori, target-devel, Stefan Hajnoczi, Paolo Bonzini On Sun, 2013-03-31 at 10:37 +0300, Michael S. Tsirkin wrote: > On Fri, Mar 29, 2013 at 04:33:11AM +0000, Nicholas A. Bellinger wrote: > > 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. > > > > 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> > > Makes sense. Minor nit: virtio_queue_valid calls virtio_queue_get_num > internally, so we can drop it everywhere we know queue is valid. > Yes, of course. This includes every location in virtio-pci.c below.. Including for patch-v2. > > --- > > hw/virtio-pci.c | 27 +++++++++++++++++++++++++++ > > 1 files changed, 27 insertions(+), 0 deletions(-) > > > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > > index 0d67b84..231ca0c 100644 > > --- a/hw/virtio-pci.c > > +++ b/hw/virtio-pci.c > > @@ -211,6 +211,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy) > > } > > > > for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) { > > + if (!virtio_queue_valid(proxy->vdev, n)) { > > + continue; > > + } > > if (!virtio_queue_get_num(proxy->vdev, n)) { > > continue; > > } > > @@ -225,6 +228,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy) > > > > assign_error: > > while (--n >= 0) { > > + if (!virtio_queue_valid(proxy->vdev, n)) { > > + continue; > > + } > > if (!virtio_queue_get_num(proxy->vdev, n)) { > > continue; > > } > > @@ -246,6 +252,9 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy) > > } > > > > for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) { > > + if (!virtio_queue_valid(proxy->vdev, n)) { > > + continue; > > + } > > if (!virtio_queue_get_num(proxy->vdev, n)) { > > continue; > > } > > @@ -546,6 +555,9 @@ 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_valid(vdev, queue_no)) { > > + continue; > > + } > > if (!virtio_queue_get_num(vdev, queue_no)) { > > break; > > } > > @@ -593,6 +605,9 @@ 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_valid(vdev, queue_no)) { > > + continue; > > + } > > if (!virtio_queue_get_num(vdev, queue_no)) { > > break; > > } > > @@ -665,6 +680,9 @@ 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_valid(vdev, queue_no)) { > > + continue; > > + } > > if (!virtio_queue_get_num(vdev, queue_no)) { > > break; > > } > > @@ -695,6 +713,9 @@ 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_valid(vdev, queue_no)) { > > + continue; > > + } > > if (!virtio_queue_get_num(vdev, queue_no)) { > > break; > > } > > @@ -717,6 +738,9 @@ 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_valid(vdev, queue_no)) { > > + continue; > > + } > > if (!virtio_queue_get_num(vdev, queue_no)) { > > break; > > } > > @@ -790,6 +814,9 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign) > > } > > > > for (n = 0; n < nvqs; n++) { > > + if (!virtio_queue_valid(vdev, n)) { > > + continue; > > + } > > if (!virtio_queue_get_num(vdev, n)) { > > break; > > } > > -- > > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] vhost: Check+skip uninitialized VQs in vhost_verify_ring_mappings [not found] <1364531592-8368-1-git-send-email-nab@linux-iscsi.org> 2013-03-29 4:33 ` [PATCH 1/3] virtio: add API to check that ring is setup Nicholas A. Bellinger 2013-03-29 4:33 ` [PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num Nicholas A. Bellinger @ 2013-03-29 4:33 ` Nicholas A. Bellinger [not found] ` <1364531592-8368-4-git-send-email-nab@linux-iscsi.org> ` (2 subsequent siblings) 5 siblings, 0 replies; 9+ messages in thread From: Nicholas A. Bellinger @ 2013-03-29 4:33 UTC (permalink / raw) To: target-devel Cc: kvm-devel, Michael S. Tsirkin, qemu-devel, lf-virt, Anthony Liguori, Stefan Hajnoczi, Paolo Bonzini From: Nicholas Bellinger <nab@linux-iscsi.org> With the virtio_queue_valid() checks in place to skip uninitialized VQs within virtio-pci code, go ahead and skip the same uninitialized VQs during vhost_verify_ring_mappings(). Note this patch does not prevent vhost_virtqueue_start() from executing by checking virtio_queue_valid(), as other logic during seabios -> virtio-scsi LLD guest hand-off appears to depend upon this execution. 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 | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/hw/vhost.c b/hw/vhost.c index 4d6aee3..3a71aee 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; } -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
[parent not found: <1364531592-8368-4-git-send-email-nab@linux-iscsi.org>]
* Re: [PATCH 3/3] vhost: Check+skip uninitialized VQs in vhost_verify_ring_mappings [not found] ` <1364531592-8368-4-git-send-email-nab@linux-iscsi.org> @ 2013-03-31 7:45 ` Michael S. Tsirkin 2013-04-01 23:49 ` Nicholas A. Bellinger 0 siblings, 1 reply; 9+ messages in thread From: Michael S. Tsirkin @ 2013-03-31 7:45 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: kvm-devel, qemu-devel, lf-virt, Anthony Liguori, target-devel, Stefan Hajnoczi, Paolo Bonzini On Fri, Mar 29, 2013 at 04:33:12AM +0000, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger <nab@linux-iscsi.org> > > With the virtio_queue_valid() checks in place to skip uninitialized VQs > within virtio-pci code, go ahead and skip the same uninitialized VQs > during vhost_verify_ring_mappings(). > > Note this patch does not prevent vhost_virtqueue_start() from executing > by checking virtio_queue_valid(), as other logic during seabios -> > virtio-scsi LLD guest hand-off appears to depend upon this execution. Weird. cpu_physical_memory_map only succeeds for PA==0 by chance, we really should not depend on this. So the right thing really should be to skip vhost_virtqueue_start IMHO, maybe add an explicit valid flag in vhost_virtqueue so vhost_verify_ring_mappings can check it. What exactly does it do that is needed? > > 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 | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/hw/vhost.c b/hw/vhost.c > index 4d6aee3..3a71aee 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; > } > -- > 1.7.2.5 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] vhost: Check+skip uninitialized VQs in vhost_verify_ring_mappings 2013-03-31 7:45 ` Michael S. Tsirkin @ 2013-04-01 23:49 ` Nicholas A. Bellinger 0 siblings, 0 replies; 9+ messages in thread From: Nicholas A. Bellinger @ 2013-04-01 23:49 UTC (permalink / raw) To: Michael S. Tsirkin Cc: kvm-devel, qemu-devel, lf-virt, Anthony Liguori, target-devel, Stefan Hajnoczi, Paolo Bonzini On Sun, 2013-03-31 at 10:45 +0300, Michael S. Tsirkin wrote: > On Fri, Mar 29, 2013 at 04:33:12AM +0000, Nicholas A. Bellinger wrote: > > From: Nicholas Bellinger <nab@linux-iscsi.org> > > > > With the virtio_queue_valid() checks in place to skip uninitialized VQs > > within virtio-pci code, go ahead and skip the same uninitialized VQs > > during vhost_verify_ring_mappings(). > > > > Note this patch does not prevent vhost_virtqueue_start() from executing > > by checking virtio_queue_valid(), as other logic during seabios -> > > virtio-scsi LLD guest hand-off appears to depend upon this execution. > > Weird. > cpu_physical_memory_map only succeeds for PA==0 by chance, > we really should not depend on this. > So the right thing really should be to skip vhost_virtqueue_start IMHO, > maybe add an explicit valid flag in vhost_virtqueue > so vhost_verify_ring_mappings can check it. > What exactly does it do that is needed? > So the issue with virtio_queue_valid() preventing vhost_virtqueue_start() execution in the original patch was that vhost_virtqueue_stop() was missing a matching virtio_queue_valid() call, which ended up triggering a bad ram pointer during subsequent cpu_physical_memory_unmap() calls to non-existent virtio queue memory.. With the matching virtio_queue_valid() call in place preventing vhost_virtqueue_stop() when vhost_virtqueue_start() is skipped for an uninitialized VQ, a explicit valid flag should not be necessary. --nab > > > > 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 | 3 +++ > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > diff --git a/hw/vhost.c b/hw/vhost.c > > index 4d6aee3..3a71aee 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; > > } > > -- > > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] virtio/vhost: Add checks for uninitialized VQs [not found] <1364531592-8368-1-git-send-email-nab@linux-iscsi.org> ` (3 preceding siblings ...) [not found] ` <1364531592-8368-4-git-send-email-nab@linux-iscsi.org> @ 2013-03-31 7:46 ` Michael S. Tsirkin [not found] ` <20130331074559.GD23484@redhat.com> 5 siblings, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2013-03-31 7:46 UTC (permalink / raw) To: Nicholas A. Bellinger Cc: kvm-devel, qemu-devel, lf-virt, Anthony Liguori, target-devel, Stefan Hajnoczi, Paolo Bonzini On Fri, Mar 29, 2013 at 04:33:09AM +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, that is currently > expected to occur during seabios setup of virtio-scsi. > > This also includes a vhost specific check for uninitialized VQs in > vhost_verify_ring_mappings() to avoid this same case. > > Please review. > > --nab Okay, and does this fix the failures in vhost_verify_ring_mappings that you've observed? > 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: Check+skip uninitialized VQs in vhost_verify_ring_mappings > > hw/vhost.c | 3 +++ > hw/virtio-pci.c | 27 +++++++++++++++++++++++++++ > hw/virtio.c | 5 +++++ > hw/virtio.h | 1 + > 4 files changed, 36 insertions(+), 0 deletions(-) > > -- > 1.7.2.5 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <20130331074559.GD23484@redhat.com>]
* Re: [PATCH 0/3] virtio/vhost: Add checks for uninitialized VQs [not found] ` <20130331074559.GD23484@redhat.com> @ 2013-04-01 23:51 ` Nicholas A. Bellinger 0 siblings, 0 replies; 9+ messages in thread From: Nicholas A. Bellinger @ 2013-04-01 23:51 UTC (permalink / raw) To: Michael S. Tsirkin Cc: kvm-devel, qemu-devel, lf-virt, Anthony Liguori, target-devel, Stefan Hajnoczi, Paolo Bonzini On Sun, 2013-03-31 at 10:46 +0300, Michael S. Tsirkin wrote: > On Fri, Mar 29, 2013 at 04:33:09AM +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, that is currently > > expected to occur during seabios setup of virtio-scsi. > > > > This also includes a vhost specific check for uninitialized VQs in > > vhost_verify_ring_mappings() to avoid this same case. > > > > Please review. > > > > --nab > > Okay, and does this fix the failures in vhost_verify_ring_mappings > that you've observed? > Unfortunately, no. I've done some more digging and will follow up with additional details on the original thread shortly.. --nab ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-04-01 23:51 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1364531592-8368-1-git-send-email-nab@linux-iscsi.org>
2013-03-29 4:33 ` [PATCH 1/3] virtio: add API to check that ring is setup Nicholas A. Bellinger
2013-03-29 4:33 ` [PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num Nicholas A. Bellinger
2013-03-31 7:37 ` Michael S. Tsirkin
[not found] ` <20130331073727.GB23484@redhat.com>
2013-04-01 23:16 ` Nicholas A. Bellinger
2013-03-29 4:33 ` [PATCH 3/3] vhost: Check+skip uninitialized VQs in vhost_verify_ring_mappings Nicholas A. Bellinger
[not found] ` <1364531592-8368-4-git-send-email-nab@linux-iscsi.org>
2013-03-31 7:45 ` Michael S. Tsirkin
2013-04-01 23:49 ` Nicholas A. Bellinger
2013-03-31 7:46 ` [PATCH 0/3] virtio/vhost: Add checks for uninitialized VQs Michael S. Tsirkin
[not found] ` <20130331074559.GD23484@redhat.com>
2013-04-01 23:51 ` Nicholas A. Bellinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).