| Message ID | 1446047243-3221-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 28 Oct 2015 17:48:02 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > Use address_space_read to make sure we handle the case of an indirect > descriptor crossing DIMM boundary correctly. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Warning: compile-tested only. > > hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c > index 68f1994..0b92fcf 100644 > --- a/hw/virtio/dataplane/vring.c > +++ b/hw/virtio/dataplane/vring.c > @@ -257,6 +257,21 @@ static void copy_in_vring_desc(VirtIODevice > *vdev, host->next = virtio_lduw_p(vdev, &guest->next); > } > > +static bool read_vring_desc(VirtIODevice *vdev, > + hwaddr guest, > + struct vring_desc *host) > +{ > + if (address_space_read(&address_space_memory, guest, > MEMTXATTRS_UNSPECIFIED, > + (uint8_t *)host, sizeof *host)) { > + return false; > + } > + host->addr = virtio_tswap64(vdev, host->addr); > + host->len = virtio_tswap32(vdev, host->len); > + host->flags = virtio_tswap16(vdev, host->flags); > + host->next = virtio_tswap16(vdev, host->next); > + return true; > +} > + > /* This is stolen from linux/drivers/vhost/vhost.c. */ > static int get_indirect(VirtIODevice *vdev, Vring *vring, > VirtQueueElement *elem, struct vring_desc > *indirect) @@ -284,23 +299,16 @@ static int get_indirect(VirtIODevice > *vdev, Vring *vring, } > > do { > - struct vring_desc *desc_ptr; > - MemoryRegion *mr; > - > /* Translate indirect descriptor */ > - desc_ptr = vring_map(&mr, > - indirect->addr + found * sizeof(desc), Is it correct to use 'found' as iterator here vs using desc.next as is done when translating direct descriptors? > - sizeof(desc), false); > - if (!desc_ptr) { > - error_report("Failed to map indirect descriptor " > + if (!read_vring_desc(vdev, indirect->addr + found * > sizeof(desc), > + &desc)) { > + error_report("Failed to read indirect descriptor " > "addr %#" PRIx64 " len %zu", > (uint64_t)indirect->addr + found * > sizeof(desc), sizeof(desc)); > vring->broken = true; > return -EFAULT; > } > - copy_in_vring_desc(vdev, desc_ptr, &desc); > - memory_region_unref(mr); > > /* Ensure descriptor has been loaded before accessing fields > */ barrier(); /* read_barrier_depends(); */
On Wed, 28 Oct 2015 17:48:02 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > Use address_space_read to make sure we handle the case of an indirect > descriptor crossing DIMM boundary correctly. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Tested-by: Igor Mammedov <imammedo@redhat.com> > --- > > Warning: compile-tested only. > > hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c > index 68f1994..0b92fcf 100644 > --- a/hw/virtio/dataplane/vring.c > +++ b/hw/virtio/dataplane/vring.c > @@ -257,6 +257,21 @@ static void copy_in_vring_desc(VirtIODevice > *vdev, host->next = virtio_lduw_p(vdev, &guest->next); > } > > +static bool read_vring_desc(VirtIODevice *vdev, > + hwaddr guest, > + struct vring_desc *host) > +{ > + if (address_space_read(&address_space_memory, guest, > MEMTXATTRS_UNSPECIFIED, > + (uint8_t *)host, sizeof *host)) { > + return false; > + } > + host->addr = virtio_tswap64(vdev, host->addr); > + host->len = virtio_tswap32(vdev, host->len); > + host->flags = virtio_tswap16(vdev, host->flags); > + host->next = virtio_tswap16(vdev, host->next); > + return true; > +} > + > /* This is stolen from linux/drivers/vhost/vhost.c. */ > static int get_indirect(VirtIODevice *vdev, Vring *vring, > VirtQueueElement *elem, struct vring_desc > *indirect) @@ -284,23 +299,16 @@ static int get_indirect(VirtIODevice > *vdev, Vring *vring, } > > do { > - struct vring_desc *desc_ptr; > - MemoryRegion *mr; > - > /* Translate indirect descriptor */ > - desc_ptr = vring_map(&mr, > - indirect->addr + found * sizeof(desc), > - sizeof(desc), false); > - if (!desc_ptr) { > - error_report("Failed to map indirect descriptor " > + if (!read_vring_desc(vdev, indirect->addr + found * > sizeof(desc), > + &desc)) { > + error_report("Failed to read indirect descriptor " > "addr %#" PRIx64 " len %zu", > (uint64_t)indirect->addr + found * > sizeof(desc), sizeof(desc)); > vring->broken = true; > return -EFAULT; > } > - copy_in_vring_desc(vdev, desc_ptr, &desc); > - memory_region_unref(mr); > > /* Ensure descriptor has been loaded before accessing fields > */ barrier(); /* read_barrier_depends(); */
On Thu, Oct 29, 2015 at 11:28:05AM +0100, Igor Mammedov wrote: > On Wed, 28 Oct 2015 17:48:02 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > - struct vring_desc *desc_ptr; > > - MemoryRegion *mr; > > - > > /* Translate indirect descriptor */ > > - desc_ptr = vring_map(&mr, > > - indirect->addr + found * sizeof(desc), > Is it correct to use 'found' as iterator here vs using desc.next > as is done when translating direct descriptors? That is how Linux drivers/vhost/vhost.c:get_indirect() does it. QEMU hw/virtio/virtio.c honors desc.next. The VIRTIO 1.0 specification says: The first indirect descriptor is located at start of the indirect descriptor table (index 0), additional indirect descriptors are chained by next field. I think vhost and dataplane do not follow the specification here. Fixing it is for a separate patch series. Stefan
On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote: > Use address_space_read to make sure we handle the case of an indirect > descriptor crossing DIMM boundary correctly. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Warning: compile-tested only. > > hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> I will test and merge (together with your fixup patch squashed in) on Monday. Stefan
On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote: > Use address_space_read to make sure we handle the case of an indirect > descriptor crossing DIMM boundary correctly. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Warning: compile-tested only. > > hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote: > Use address_space_read to make sure we handle the case of an indirect > descriptor crossing DIMM boundary correctly. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Warning: compile-tested only. Test (with your follow-up patch squashed in) with 6 4KB seqeuential read processes on running successfully. I didn't test the DIMM boundary case. Igor: what is the easiest way to reproduce the DIMM boundary crossing?
On Mon, 2 Nov 2015 17:43:16 +0000 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, Oct 28, 2015 at 05:48:02PM +0200, Michael S. Tsirkin wrote: > > Use address_space_read to make sure we handle the case of an indirect > > descriptor crossing DIMM boundary correctly. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Warning: compile-tested only. > > Test (with your follow-up patch squashed in) with 6 4KB seqeuential read > processes on running successfully. > > I didn't test the DIMM boundary case. Igor: what is the easiest way to > reproduce the DIMM boundary crossing? I've tested it, here is QEMU CLI: qemu-system-x86_64 -enable-kvm -enable-kvm -m 128M,slots=250,maxmem=32G -drive if=none,id=hd,file=rhel72,cache=none,aio=native,format=raw -device virtio-blk,drive=hd,scsi=off,config-wce=off,x-data-plane=on `for i in $(seq 0 15); do echo -n "-object memory-backend-ram,id=m$i,size=10M -device pc-dimm,id=dimm$i,memdev=m$i "; done` boot and then do: dd if=/dev/vda of=/dev/null bs=16M without fix it hangs either at boot time or when doing 'dd'
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c index 68f1994..0b92fcf 100644 --- a/hw/virtio/dataplane/vring.c +++ b/hw/virtio/dataplane/vring.c @@ -257,6 +257,21 @@ static void copy_in_vring_desc(VirtIODevice *vdev, host->next = virtio_lduw_p(vdev, &guest->next); } +static bool read_vring_desc(VirtIODevice *vdev, + hwaddr guest, + struct vring_desc *host) +{ + if (address_space_read(&address_space_memory, guest, MEMTXATTRS_UNSPECIFIED, + (uint8_t *)host, sizeof *host)) { + return false; + } + host->addr = virtio_tswap64(vdev, host->addr); + host->len = virtio_tswap32(vdev, host->len); + host->flags = virtio_tswap16(vdev, host->flags); + host->next = virtio_tswap16(vdev, host->next); + return true; +} + /* This is stolen from linux/drivers/vhost/vhost.c. */ static int get_indirect(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem, struct vring_desc *indirect) @@ -284,23 +299,16 @@ static int get_indirect(VirtIODevice *vdev, Vring *vring, } do { - struct vring_desc *desc_ptr; - MemoryRegion *mr; - /* Translate indirect descriptor */ - desc_ptr = vring_map(&mr, - indirect->addr + found * sizeof(desc), - sizeof(desc), false); - if (!desc_ptr) { - error_report("Failed to map indirect descriptor " + if (!read_vring_desc(vdev, indirect->addr + found * sizeof(desc), + &desc)) { + error_report("Failed to read indirect descriptor " "addr %#" PRIx64 " len %zu", (uint64_t)indirect->addr + found * sizeof(desc), sizeof(desc)); vring->broken = true; return -EFAULT; } - copy_in_vring_desc(vdev, desc_ptr, &desc); - memory_region_unref(mr); /* Ensure descriptor has been loaded before accessing fields */ barrier(); /* read_barrier_depends(); */
Use address_space_read to make sure we handle the case of an indirect descriptor crossing DIMM boundary correctly. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Warning: compile-tested only. hw/virtio/dataplane/vring.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)