diff mbox

[06/18] virtio: Return error from virtqueue_pop

Message ID 1429257573-7359-7-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng April 17, 2015, 7:59 a.m. UTC
When getting invalid data from vring, virtqueue_pop used to print an
error and exit.

Add an errp parameter so it can return the error to callers.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/9pfs/virtio-9p.c         |  2 +-
 hw/block/virtio-blk.c       |  2 +-
 hw/char/virtio-serial-bus.c | 10 +++----
 hw/net/virtio-net.c         |  6 ++--
 hw/scsi/virtio-scsi.c       |  2 +-
 hw/virtio/virtio-balloon.c  |  4 +--
 hw/virtio/virtio-rng.c      |  2 +-
 hw/virtio/virtio.c          | 70 ++++++++++++++++++++++++++++++++++-----------
 include/hw/virtio/virtio.h  |  2 +-
 9 files changed, 69 insertions(+), 31 deletions(-)

Comments

Michael S. Tsirkin April 21, 2015, 6:49 a.m. UTC | #1
On Fri, Apr 17, 2015 at 03:59:21PM +0800, Fam Zheng wrote:
> When getting invalid data from vring, virtqueue_pop used to print an
> error and exit.
> 
> Add an errp parameter so it can return the error to callers.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/9pfs/virtio-9p.c         |  2 +-
>  hw/block/virtio-blk.c       |  2 +-
>  hw/char/virtio-serial-bus.c | 10 +++----
>  hw/net/virtio-net.c         |  6 ++--
>  hw/scsi/virtio-scsi.c       |  2 +-
>  hw/virtio/virtio-balloon.c  |  4 +--
>  hw/virtio/virtio-rng.c      |  2 +-
>  hw/virtio/virtio.c          | 70 ++++++++++++++++++++++++++++++++++-----------
>  include/hw/virtio/virtio.h  |  2 +-
>  9 files changed, 69 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index 4964da0..17d0c4a 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -3259,7 +3259,7 @@ void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
>      ssize_t len;
>  
>      while ((pdu = alloc_pdu(s)) &&
> -            (len = virtqueue_pop(vq, &pdu->elem)) != 0) {
> +            (len = virtqueue_pop(vq, &pdu->elem, &error_abort)) != 0) {
>          uint8_t *ptr;
>          pdu->s = s;
>          BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f7d8528..0b66ee1 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -191,7 +191,7 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
>  {
>      VirtIOBlockReq *req = virtio_blk_alloc_request(s);
>  
> -    if (!virtqueue_pop(s->vq, &req->elem)) {
> +    if (!virtqueue_pop(s->vq, &req->elem, &error_abort)) {
>          virtio_blk_free_request(req);
>          return NULL;
>      }
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index a56dafc..76a934b 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -94,7 +94,7 @@ static size_t write_to_port(VirtIOSerialPort *port,
>      while (offset < size) {
>          size_t len;
>  
> -        if (!virtqueue_pop(vq, &elem)) {
> +        if (!virtqueue_pop(vq, &elem, &error_abort)) {
>              break;
>          }
>  
> @@ -116,7 +116,7 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
>      if (!virtio_queue_ready(vq)) {
>          return;
>      }
> -    while (virtqueue_pop(vq, &elem)) {
> +    while (virtqueue_pop(vq, &elem, &error_abort)) {
>          virtqueue_push(vq, &elem, 0);
>      }
>      virtio_notify(vdev, vq);
> @@ -137,7 +137,7 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
>  
>          /* Pop an elem only if we haven't left off a previous one mid-way */
>          if (!port->elem.out_num) {
> -            if (!virtqueue_pop(vq, &port->elem)) {
> +            if (!virtqueue_pop(vq, &port->elem, &error_abort)) {
>                  break;
>              }
>              port->iov_idx = 0;
> @@ -190,7 +190,7 @@ static size_t send_control_msg(VirtIOSerial *vser, void *buf, size_t len)
>      if (!virtio_queue_ready(vq)) {
>          return 0;
>      }
> -    if (!virtqueue_pop(vq, &elem)) {
> +    if (!virtqueue_pop(vq, &elem, &error_abort)) {
>          return 0;
>      }
>  
> @@ -420,7 +420,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
>  
>      len = 0;
>      buf = NULL;
> -    while (virtqueue_pop(vq, &elem)) {
> +    while (virtqueue_pop(vq, &elem, &error_abort)) {
>          size_t cur_len;
>  
>          cur_len = iov_size(elem.out_sg, elem.out_num);
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 59f76bc..bbcb51f 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -804,7 +804,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>      struct iovec *iov, *iov2;
>      unsigned int iov_cnt;
>  
> -    while (virtqueue_pop(vq, &elem)) {
> +    while (virtqueue_pop(vq, &elem, &error_abort)) {
>          if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
>              iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
>              error_report("virtio-net ctrl missing headers");
> @@ -1031,7 +1031,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
>  
>          total = 0;
>  
> -        if (virtqueue_pop(q->rx_vq, &elem) == 0) {
> +        if (virtqueue_pop(q->rx_vq, &elem, &error_abort) == 0) {
>              if (i == 0)
>                  return -1;
>              error_report("virtio-net unexpected empty queue: "
> @@ -1134,7 +1134,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>          return num_packets;
>      }
>  
> -    while (virtqueue_pop(q->tx_vq, &elem)) {
> +    while (virtqueue_pop(q->tx_vq, &elem, &error_abort)) {
>          ssize_t ret, len;
>          unsigned int out_num = elem.out_num;
>          struct iovec *out_sg = &elem.out_sg[0];
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index c9bea06..40ba03d 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -177,7 +177,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
>  static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq)
>  {
>      VirtIOSCSIReq *req = virtio_scsi_init_req(s, vq);
> -    if (!virtqueue_pop(vq, &req->elem)) {
> +    if (!virtqueue_pop(vq, &req->elem, &error_abort)) {
>          virtio_scsi_free_req(req);
>          return NULL;
>      }
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 95b0643..e26c0a7 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -206,7 +206,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>      VirtQueueElement elem;
>      MemoryRegionSection section;
>  
> -    while (virtqueue_pop(vq, &elem)) {
> +    while (virtqueue_pop(vq, &elem, &error_abort)) {
>          size_t offset = 0;
>          uint32_t pfn;
>  
> @@ -246,7 +246,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
>      size_t offset = 0;
>      qemu_timeval tv;
>  
> -    if (!virtqueue_pop(vq, elem)) {
> +    if (!virtqueue_pop(vq, elem, &error_abort)) {
>          goto out;
>      }
>  
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 9e1bd75..c3cbdc3 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -56,7 +56,7 @@ static void chr_read(void *opaque, const void *buf, size_t size)
>  
>      offset = 0;
>      while (offset < size) {
> -        if (!virtqueue_pop(vrng->vq, &elem)) {
> +        if (!virtqueue_pop(vrng->vq, &elem, &error_abort)) {
>              break;
>          }
>          len = iov_from_buf(elem.in_sg, elem.in_num,
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 1cd454b..e6f9f6b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -481,29 +481,49 @@ fail:
>      error_setg(errp, "virtio: error trying to map MMIO memory");
>  }
>  
> -int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> +static void virtqueue_undo_map_sg(struct iovec *sg, hwaddr *addr,
> +                                  size_t num_sg, int is_write)
> +{
> +    int i;
> +
> +    for (i = 0; i < num_sg; i++) {
> +        cpu_physical_memory_unmap(sg[i].iov_base, sg[i].iov_len,
> +                                  is_write, 0);
> +    }
> +}
> +
> +int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem, Error **errp)
>  {
>      unsigned int i, head, max;
> +    int ret;
>      hwaddr desc_pa = vq->vring.desc;
>      VirtIODevice *vdev = vq->vdev;
> +    Error *local_err = NULL;
>  
> -    if (!virtqueue_num_heads(vq, vq->last_avail_idx, &error_abort))
> -        return 0;
> +    ret = virtqueue_num_heads(vq, vq->last_avail_idx, &local_err);
> +    if (ret <= 0) {
> +        goto err;
> +    }
>  

Strange.
Doesn't this make pop print an error if ring is empty?



>      /* When we start there are none of either input nor output. */
>      elem->out_num = elem->in_num = 0;
>  
>      max = vq->vring.num;
>  
> -    i = head = virtqueue_get_head(vq, vq->last_avail_idx++, &error_abort);
> +    ret = virtqueue_get_head(vq, vq->last_avail_idx++, &local_err);
> +    if (ret < 0) {
> +        goto err;
> +    }
> +    head = i = ret;
> +
>      if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
>          vring_set_avail_event(vq, vq->last_avail_idx);
>      }
>  
>      if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
>          if (vring_desc_len(vdev, desc_pa, i) % sizeof(VRingDesc)) {
> -            error_report("Invalid size for indirect buffer table");
> -            exit(1);
> +            error_setg(errp, "Invalid size for indirect buffer table");
> +            return -EINVAL;

Again, you have both error_setg and a return code.
There's no need for this.
Just return ring empty on error.


>          }
>  
>          /* loop over the indirect descriptor table */
> @@ -518,15 +538,17 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>  
>          if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_WRITE) {
>              if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
> -                error_report("Too many write descriptors in indirect table");
> -                exit(1);
> +                error_setg(errp,
> +                           "Too many write descriptors in indirect table");
> +                return -EINVAL;
>              }
>              elem->in_addr[elem->in_num] = vring_desc_addr(vdev, desc_pa, i);
>              sg = &elem->in_sg[elem->in_num++];
>          } else {
>              if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
> -                error_report("Too many read descriptors in indirect table");
> -                exit(1);
> +                error_setg(errp,
> +                           "Too many read descriptors in indirect table");
> +                return -EINVAL;
>              }
>              elem->out_addr[elem->out_num] = vring_desc_addr(vdev, desc_pa, i);
>              sg = &elem->out_sg[elem->out_num++];
> @@ -536,20 +558,31 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>  
>          /* If we've got too many, that implies a descriptor loop. */
>          if ((elem->in_num + elem->out_num) > max) {
> -            error_report("Looped descriptor");
> -            exit(1);
> +            error_setg(errp, "Looped descriptor");
> +            return -EINVAL;
>          }
> -        i = virtqueue_next_desc(vdev, desc_pa, i, max, &error_abort);
> -        if (i == max) {
> +        ret = virtqueue_next_desc(vdev, desc_pa, i, max, &local_err);
> +        if (ret < 0) {
> +            goto err;
> +        } else if (ret == max) {
>              break;
>          }
> +        i = ret;
>      }
>  
>      /* Now map what we have collected */
>      virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1,
> -                     &error_abort);
> +                     &local_err);
> +    if (local_err) {
> +        ret = -EINVAL;
> +        goto err;
> +    }
>      virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0,
> -                     &error_abort);
> +                     &local_err);
> +    if (local_err) {
> +        ret = -EINVAL;
> +        goto err_unmap;
> +    }
>  
>      elem->index = head;
>  
> @@ -557,6 +590,11 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
>  
>      trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
>      return elem->in_num + elem->out_num;
> +err_unmap:
> +    virtqueue_undo_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
> +err:
> +    error_propagate(errp, local_err);
> +    return ret;
>  }
>  
>  /* virtio device */
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index a37ee64..c478f48 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -142,7 +142,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
>  void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
>                        size_t num_sg, int is_write,
>                        Error **errp);
> -int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
> +int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem, Error **errp);
>  int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
>                            unsigned int out_bytes);
>  void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
> -- 
> 1.9.3
Fam Zheng April 21, 2015, 7:24 a.m. UTC | #2
On Tue, 04/21 08:49, Michael S. Tsirkin wrote:
> On Fri, Apr 17, 2015 at 03:59:21PM +0800, Fam Zheng wrote:
> > When getting invalid data from vring, virtqueue_pop used to print an
> > error and exit.
> > 
> > Add an errp parameter so it can return the error to callers.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/9pfs/virtio-9p.c         |  2 +-
> >  hw/block/virtio-blk.c       |  2 +-
> >  hw/char/virtio-serial-bus.c | 10 +++----
> >  hw/net/virtio-net.c         |  6 ++--
> >  hw/scsi/virtio-scsi.c       |  2 +-
> >  hw/virtio/virtio-balloon.c  |  4 +--
> >  hw/virtio/virtio-rng.c      |  2 +-
> >  hw/virtio/virtio.c          | 70 ++++++++++++++++++++++++++++++++++-----------
> >  include/hw/virtio/virtio.h  |  2 +-
> >  9 files changed, 69 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> > index 4964da0..17d0c4a 100644
> > --- a/hw/9pfs/virtio-9p.c
> > +++ b/hw/9pfs/virtio-9p.c
> > @@ -3259,7 +3259,7 @@ void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
> >      ssize_t len;
> >  
> >      while ((pdu = alloc_pdu(s)) &&
> > -            (len = virtqueue_pop(vq, &pdu->elem)) != 0) {
> > +            (len = virtqueue_pop(vq, &pdu->elem, &error_abort)) != 0) {
> >          uint8_t *ptr;
> >          pdu->s = s;
> >          BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index f7d8528..0b66ee1 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -191,7 +191,7 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
> >  {
> >      VirtIOBlockReq *req = virtio_blk_alloc_request(s);
> >  
> > -    if (!virtqueue_pop(s->vq, &req->elem)) {
> > +    if (!virtqueue_pop(s->vq, &req->elem, &error_abort)) {
> >          virtio_blk_free_request(req);
> >          return NULL;
> >      }
> > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > index a56dafc..76a934b 100644
> > --- a/hw/char/virtio-serial-bus.c
> > +++ b/hw/char/virtio-serial-bus.c
> > @@ -94,7 +94,7 @@ static size_t write_to_port(VirtIOSerialPort *port,
> >      while (offset < size) {
> >          size_t len;
> >  
> > -        if (!virtqueue_pop(vq, &elem)) {
> > +        if (!virtqueue_pop(vq, &elem, &error_abort)) {
> >              break;
> >          }
> >  
> > @@ -116,7 +116,7 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
> >      if (!virtio_queue_ready(vq)) {
> >          return;
> >      }
> > -    while (virtqueue_pop(vq, &elem)) {
> > +    while (virtqueue_pop(vq, &elem, &error_abort)) {
> >          virtqueue_push(vq, &elem, 0);
> >      }
> >      virtio_notify(vdev, vq);
> > @@ -137,7 +137,7 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
> >  
> >          /* Pop an elem only if we haven't left off a previous one mid-way */
> >          if (!port->elem.out_num) {
> > -            if (!virtqueue_pop(vq, &port->elem)) {
> > +            if (!virtqueue_pop(vq, &port->elem, &error_abort)) {
> >                  break;
> >              }
> >              port->iov_idx = 0;
> > @@ -190,7 +190,7 @@ static size_t send_control_msg(VirtIOSerial *vser, void *buf, size_t len)
> >      if (!virtio_queue_ready(vq)) {
> >          return 0;
> >      }
> > -    if (!virtqueue_pop(vq, &elem)) {
> > +    if (!virtqueue_pop(vq, &elem, &error_abort)) {
> >          return 0;
> >      }
> >  
> > @@ -420,7 +420,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
> >  
> >      len = 0;
> >      buf = NULL;
> > -    while (virtqueue_pop(vq, &elem)) {
> > +    while (virtqueue_pop(vq, &elem, &error_abort)) {
> >          size_t cur_len;
> >  
> >          cur_len = iov_size(elem.out_sg, elem.out_num);
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 59f76bc..bbcb51f 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -804,7 +804,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> >      struct iovec *iov, *iov2;
> >      unsigned int iov_cnt;
> >  
> > -    while (virtqueue_pop(vq, &elem)) {
> > +    while (virtqueue_pop(vq, &elem, &error_abort)) {
> >          if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
> >              iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
> >              error_report("virtio-net ctrl missing headers");
> > @@ -1031,7 +1031,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
> >  
> >          total = 0;
> >  
> > -        if (virtqueue_pop(q->rx_vq, &elem) == 0) {
> > +        if (virtqueue_pop(q->rx_vq, &elem, &error_abort) == 0) {
> >              if (i == 0)
> >                  return -1;
> >              error_report("virtio-net unexpected empty queue: "
> > @@ -1134,7 +1134,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> >          return num_packets;
> >      }
> >  
> > -    while (virtqueue_pop(q->tx_vq, &elem)) {
> > +    while (virtqueue_pop(q->tx_vq, &elem, &error_abort)) {
> >          ssize_t ret, len;
> >          unsigned int out_num = elem.out_num;
> >          struct iovec *out_sg = &elem.out_sg[0];
> > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > index c9bea06..40ba03d 100644
> > --- a/hw/scsi/virtio-scsi.c
> > +++ b/hw/scsi/virtio-scsi.c
> > @@ -177,7 +177,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
> >  static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq)
> >  {
> >      VirtIOSCSIReq *req = virtio_scsi_init_req(s, vq);
> > -    if (!virtqueue_pop(vq, &req->elem)) {
> > +    if (!virtqueue_pop(vq, &req->elem, &error_abort)) {
> >          virtio_scsi_free_req(req);
> >          return NULL;
> >      }
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 95b0643..e26c0a7 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -206,7 +206,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >      VirtQueueElement elem;
> >      MemoryRegionSection section;
> >  
> > -    while (virtqueue_pop(vq, &elem)) {
> > +    while (virtqueue_pop(vq, &elem, &error_abort)) {
> >          size_t offset = 0;
> >          uint32_t pfn;
> >  
> > @@ -246,7 +246,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> >      size_t offset = 0;
> >      qemu_timeval tv;
> >  
> > -    if (!virtqueue_pop(vq, elem)) {
> > +    if (!virtqueue_pop(vq, elem, &error_abort)) {
> >          goto out;
> >      }
> >  
> > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> > index 9e1bd75..c3cbdc3 100644
> > --- a/hw/virtio/virtio-rng.c
> > +++ b/hw/virtio/virtio-rng.c
> > @@ -56,7 +56,7 @@ static void chr_read(void *opaque, const void *buf, size_t size)
> >  
> >      offset = 0;
> >      while (offset < size) {
> > -        if (!virtqueue_pop(vrng->vq, &elem)) {
> > +        if (!virtqueue_pop(vrng->vq, &elem, &error_abort)) {
> >              break;
> >          }
> >          len = iov_from_buf(elem.in_sg, elem.in_num,
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 1cd454b..e6f9f6b 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -481,29 +481,49 @@ fail:
> >      error_setg(errp, "virtio: error trying to map MMIO memory");
> >  }
> >  
> > -int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> > +static void virtqueue_undo_map_sg(struct iovec *sg, hwaddr *addr,
> > +                                  size_t num_sg, int is_write)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < num_sg; i++) {
> > +        cpu_physical_memory_unmap(sg[i].iov_base, sg[i].iov_len,
> > +                                  is_write, 0);
> > +    }
> > +}
> > +
> > +int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem, Error **errp)
> >  {
> >      unsigned int i, head, max;
> > +    int ret;
> >      hwaddr desc_pa = vq->vring.desc;
> >      VirtIODevice *vdev = vq->vdev;
> > +    Error *local_err = NULL;
> >  
> > -    if (!virtqueue_num_heads(vq, vq->last_avail_idx, &error_abort))
> > -        return 0;
> > +    ret = virtqueue_num_heads(vq, vq->last_avail_idx, &local_err);
> > +    if (ret <= 0) {
> > +        goto err;
> > +    }
> >  
> 
> Strange.
> Doesn't this make pop print an error if ring is empty?

Code at err label propagates local_err (NULL if empty, hence nop) and returns
ret (0 if empty). Maybe rename it to "out".

> 
> 
> 
> >      /* When we start there are none of either input nor output. */
> >      elem->out_num = elem->in_num = 0;
> >  
> >      max = vq->vring.num;
> >  
> > -    i = head = virtqueue_get_head(vq, vq->last_avail_idx++, &error_abort);
> > +    ret = virtqueue_get_head(vq, vq->last_avail_idx++, &local_err);
> > +    if (ret < 0) {
> > +        goto err;
> > +    }
> > +    head = i = ret;
> > +
> >      if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> >          vring_set_avail_event(vq, vq->last_avail_idx);
> >      }
> >  
> >      if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
> >          if (vring_desc_len(vdev, desc_pa, i) % sizeof(VRingDesc)) {
> > -            error_report("Invalid size for indirect buffer table");
> > -            exit(1);
> > +            error_setg(errp, "Invalid size for indirect buffer table");
> > +            return -EINVAL;
> 
> Again, you have both error_setg and a return code.
> There's no need for this.
> Just return ring empty on error.

How would caller distinguish real ring empty from error then?

Fam
Michael S. Tsirkin April 21, 2015, 9:51 a.m. UTC | #3
On Tue, Apr 21, 2015 at 03:24:25PM +0800, Fam Zheng wrote:
> On Tue, 04/21 08:49, Michael S. Tsirkin wrote:
> > On Fri, Apr 17, 2015 at 03:59:21PM +0800, Fam Zheng wrote:
> > > When getting invalid data from vring, virtqueue_pop used to print an
> > > error and exit.
> > > 
> > > Add an errp parameter so it can return the error to callers.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  hw/9pfs/virtio-9p.c         |  2 +-
> > >  hw/block/virtio-blk.c       |  2 +-
> > >  hw/char/virtio-serial-bus.c | 10 +++----
> > >  hw/net/virtio-net.c         |  6 ++--
> > >  hw/scsi/virtio-scsi.c       |  2 +-
> > >  hw/virtio/virtio-balloon.c  |  4 +--
> > >  hw/virtio/virtio-rng.c      |  2 +-
> > >  hw/virtio/virtio.c          | 70 ++++++++++++++++++++++++++++++++++-----------
> > >  include/hw/virtio/virtio.h  |  2 +-
> > >  9 files changed, 69 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> > > index 4964da0..17d0c4a 100644
> > > --- a/hw/9pfs/virtio-9p.c
> > > +++ b/hw/9pfs/virtio-9p.c
> > > @@ -3259,7 +3259,7 @@ void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
> > >      ssize_t len;
> > >  
> > >      while ((pdu = alloc_pdu(s)) &&
> > > -            (len = virtqueue_pop(vq, &pdu->elem)) != 0) {
> > > +            (len = virtqueue_pop(vq, &pdu->elem, &error_abort)) != 0) {
> > >          uint8_t *ptr;
> > >          pdu->s = s;
> > >          BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index f7d8528..0b66ee1 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -191,7 +191,7 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
> > >  {
> > >      VirtIOBlockReq *req = virtio_blk_alloc_request(s);
> > >  
> > > -    if (!virtqueue_pop(s->vq, &req->elem)) {
> > > +    if (!virtqueue_pop(s->vq, &req->elem, &error_abort)) {
> > >          virtio_blk_free_request(req);
> > >          return NULL;
> > >      }
> > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> > > index a56dafc..76a934b 100644
> > > --- a/hw/char/virtio-serial-bus.c
> > > +++ b/hw/char/virtio-serial-bus.c
> > > @@ -94,7 +94,7 @@ static size_t write_to_port(VirtIOSerialPort *port,
> > >      while (offset < size) {
> > >          size_t len;
> > >  
> > > -        if (!virtqueue_pop(vq, &elem)) {
> > > +        if (!virtqueue_pop(vq, &elem, &error_abort)) {
> > >              break;
> > >          }
> > >  
> > > @@ -116,7 +116,7 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
> > >      if (!virtio_queue_ready(vq)) {
> > >          return;
> > >      }
> > > -    while (virtqueue_pop(vq, &elem)) {
> > > +    while (virtqueue_pop(vq, &elem, &error_abort)) {
> > >          virtqueue_push(vq, &elem, 0);
> > >      }
> > >      virtio_notify(vdev, vq);
> > > @@ -137,7 +137,7 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
> > >  
> > >          /* Pop an elem only if we haven't left off a previous one mid-way */
> > >          if (!port->elem.out_num) {
> > > -            if (!virtqueue_pop(vq, &port->elem)) {
> > > +            if (!virtqueue_pop(vq, &port->elem, &error_abort)) {
> > >                  break;
> > >              }
> > >              port->iov_idx = 0;
> > > @@ -190,7 +190,7 @@ static size_t send_control_msg(VirtIOSerial *vser, void *buf, size_t len)
> > >      if (!virtio_queue_ready(vq)) {
> > >          return 0;
> > >      }
> > > -    if (!virtqueue_pop(vq, &elem)) {
> > > +    if (!virtqueue_pop(vq, &elem, &error_abort)) {
> > >          return 0;
> > >      }
> > >  
> > > @@ -420,7 +420,7 @@ static void control_out(VirtIODevice *vdev, VirtQueue *vq)
> > >  
> > >      len = 0;
> > >      buf = NULL;
> > > -    while (virtqueue_pop(vq, &elem)) {
> > > +    while (virtqueue_pop(vq, &elem, &error_abort)) {
> > >          size_t cur_len;
> > >  
> > >          cur_len = iov_size(elem.out_sg, elem.out_num);
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 59f76bc..bbcb51f 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -804,7 +804,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> > >      struct iovec *iov, *iov2;
> > >      unsigned int iov_cnt;
> > >  
> > > -    while (virtqueue_pop(vq, &elem)) {
> > > +    while (virtqueue_pop(vq, &elem, &error_abort)) {
> > >          if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
> > >              iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
> > >              error_report("virtio-net ctrl missing headers");
> > > @@ -1031,7 +1031,7 @@ static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
> > >  
> > >          total = 0;
> > >  
> > > -        if (virtqueue_pop(q->rx_vq, &elem) == 0) {
> > > +        if (virtqueue_pop(q->rx_vq, &elem, &error_abort) == 0) {
> > >              if (i == 0)
> > >                  return -1;
> > >              error_report("virtio-net unexpected empty queue: "
> > > @@ -1134,7 +1134,7 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
> > >          return num_packets;
> > >      }
> > >  
> > > -    while (virtqueue_pop(q->tx_vq, &elem)) {
> > > +    while (virtqueue_pop(q->tx_vq, &elem, &error_abort)) {
> > >          ssize_t ret, len;
> > >          unsigned int out_num = elem.out_num;
> > >          struct iovec *out_sg = &elem.out_sg[0];
> > > diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> > > index c9bea06..40ba03d 100644
> > > --- a/hw/scsi/virtio-scsi.c
> > > +++ b/hw/scsi/virtio-scsi.c
> > > @@ -177,7 +177,7 @@ static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
> > >  static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq)
> > >  {
> > >      VirtIOSCSIReq *req = virtio_scsi_init_req(s, vq);
> > > -    if (!virtqueue_pop(vq, &req->elem)) {
> > > +    if (!virtqueue_pop(vq, &req->elem, &error_abort)) {
> > >          virtio_scsi_free_req(req);
> > >          return NULL;
> > >      }
> > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > index 95b0643..e26c0a7 100644
> > > --- a/hw/virtio/virtio-balloon.c
> > > +++ b/hw/virtio/virtio-balloon.c
> > > @@ -206,7 +206,7 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> > >      VirtQueueElement elem;
> > >      MemoryRegionSection section;
> > >  
> > > -    while (virtqueue_pop(vq, &elem)) {
> > > +    while (virtqueue_pop(vq, &elem, &error_abort)) {
> > >          size_t offset = 0;
> > >          uint32_t pfn;
> > >  
> > > @@ -246,7 +246,7 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
> > >      size_t offset = 0;
> > >      qemu_timeval tv;
> > >  
> > > -    if (!virtqueue_pop(vq, elem)) {
> > > +    if (!virtqueue_pop(vq, elem, &error_abort)) {
> > >          goto out;
> > >      }
> > >  
> > > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> > > index 9e1bd75..c3cbdc3 100644
> > > --- a/hw/virtio/virtio-rng.c
> > > +++ b/hw/virtio/virtio-rng.c
> > > @@ -56,7 +56,7 @@ static void chr_read(void *opaque, const void *buf, size_t size)
> > >  
> > >      offset = 0;
> > >      while (offset < size) {
> > > -        if (!virtqueue_pop(vrng->vq, &elem)) {
> > > +        if (!virtqueue_pop(vrng->vq, &elem, &error_abort)) {
> > >              break;
> > >          }
> > >          len = iov_from_buf(elem.in_sg, elem.in_num,
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 1cd454b..e6f9f6b 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -481,29 +481,49 @@ fail:
> > >      error_setg(errp, "virtio: error trying to map MMIO memory");
> > >  }
> > >  
> > > -int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
> > > +static void virtqueue_undo_map_sg(struct iovec *sg, hwaddr *addr,
> > > +                                  size_t num_sg, int is_write)
> > > +{
> > > +    int i;
> > > +
> > > +    for (i = 0; i < num_sg; i++) {
> > > +        cpu_physical_memory_unmap(sg[i].iov_base, sg[i].iov_len,
> > > +                                  is_write, 0);
> > > +    }
> > > +}
> > > +
> > > +int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem, Error **errp)
> > >  {
> > >      unsigned int i, head, max;
> > > +    int ret;
> > >      hwaddr desc_pa = vq->vring.desc;
> > >      VirtIODevice *vdev = vq->vdev;
> > > +    Error *local_err = NULL;
> > >  
> > > -    if (!virtqueue_num_heads(vq, vq->last_avail_idx, &error_abort))
> > > -        return 0;
> > > +    ret = virtqueue_num_heads(vq, vq->last_avail_idx, &local_err);
> > > +    if (ret <= 0) {
> > > +        goto err;
> > > +    }
> > >  
> > 
> > Strange.
> > Doesn't this make pop print an error if ring is empty?
> 
> Code at err label propagates local_err (NULL if empty, hence nop) and returns
> ret (0 if empty). Maybe rename it to "out".

But there's no need to print two errors. virtqueue_num_heads already
prints an error, so just exit here.

> > 
> > 
> > 
> > >      /* When we start there are none of either input nor output. */
> > >      elem->out_num = elem->in_num = 0;
> > >  
> > >      max = vq->vring.num;
> > >  
> > > -    i = head = virtqueue_get_head(vq, vq->last_avail_idx++, &error_abort);
> > > +    ret = virtqueue_get_head(vq, vq->last_avail_idx++, &local_err);
> > > +    if (ret < 0) {
> > > +        goto err;
> > > +    }
> > > +    head = i = ret;
> > > +
> > >      if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > >          vring_set_avail_event(vq, vq->last_avail_idx);
> > >      }
> > >  
> > >      if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
> > >          if (vring_desc_len(vdev, desc_pa, i) % sizeof(VRingDesc)) {
> > > -            error_report("Invalid size for indirect buffer table");
> > > -            exit(1);
> > > +            error_setg(errp, "Invalid size for indirect buffer table");
> > > +            return -EINVAL;
> > 
> > Again, you have both error_setg and a return code.
> > There's no need for this.
> > Just return ring empty on error.
> 
> How would caller distinguish real ring empty from error then?
> 
> Fam

There's no need for it to do this.  When some function detects an error,
it should set the NEEDS_RESET status, and be done with it.  No need to
propagate the error condition back and forth, it would be a bunch of
poorly-tested code.
diff mbox

Patch

diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
index 4964da0..17d0c4a 100644
--- a/hw/9pfs/virtio-9p.c
+++ b/hw/9pfs/virtio-9p.c
@@ -3259,7 +3259,7 @@  void handle_9p_output(VirtIODevice *vdev, VirtQueue *vq)
     ssize_t len;
 
     while ((pdu = alloc_pdu(s)) &&
-            (len = virtqueue_pop(vq, &pdu->elem)) != 0) {
+            (len = virtqueue_pop(vq, &pdu->elem, &error_abort)) != 0) {
         uint8_t *ptr;
         pdu->s = s;
         BUG_ON(pdu->elem.out_num == 0 || pdu->elem.in_num == 0);
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f7d8528..0b66ee1 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -191,7 +191,7 @@  static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
 {
     VirtIOBlockReq *req = virtio_blk_alloc_request(s);
 
-    if (!virtqueue_pop(s->vq, &req->elem)) {
+    if (!virtqueue_pop(s->vq, &req->elem, &error_abort)) {
         virtio_blk_free_request(req);
         return NULL;
     }
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index a56dafc..76a934b 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -94,7 +94,7 @@  static size_t write_to_port(VirtIOSerialPort *port,
     while (offset < size) {
         size_t len;
 
-        if (!virtqueue_pop(vq, &elem)) {
+        if (!virtqueue_pop(vq, &elem, &error_abort)) {
             break;
         }
 
@@ -116,7 +116,7 @@  static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
     if (!virtio_queue_ready(vq)) {
         return;
     }
-    while (virtqueue_pop(vq, &elem)) {
+    while (virtqueue_pop(vq, &elem, &error_abort)) {
         virtqueue_push(vq, &elem, 0);
     }
     virtio_notify(vdev, vq);
@@ -137,7 +137,7 @@  static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
 
         /* Pop an elem only if we haven't left off a previous one mid-way */
         if (!port->elem.out_num) {
-            if (!virtqueue_pop(vq, &port->elem)) {
+            if (!virtqueue_pop(vq, &port->elem, &error_abort)) {
                 break;
             }
             port->iov_idx = 0;
@@ -190,7 +190,7 @@  static size_t send_control_msg(VirtIOSerial *vser, void *buf, size_t len)
     if (!virtio_queue_ready(vq)) {
         return 0;
     }
-    if (!virtqueue_pop(vq, &elem)) {
+    if (!virtqueue_pop(vq, &elem, &error_abort)) {
         return 0;
     }
 
@@ -420,7 +420,7 @@  static void control_out(VirtIODevice *vdev, VirtQueue *vq)
 
     len = 0;
     buf = NULL;
-    while (virtqueue_pop(vq, &elem)) {
+    while (virtqueue_pop(vq, &elem, &error_abort)) {
         size_t cur_len;
 
         cur_len = iov_size(elem.out_sg, elem.out_num);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 59f76bc..bbcb51f 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -804,7 +804,7 @@  static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     struct iovec *iov, *iov2;
     unsigned int iov_cnt;
 
-    while (virtqueue_pop(vq, &elem)) {
+    while (virtqueue_pop(vq, &elem, &error_abort)) {
         if (iov_size(elem.in_sg, elem.in_num) < sizeof(status) ||
             iov_size(elem.out_sg, elem.out_num) < sizeof(ctrl)) {
             error_report("virtio-net ctrl missing headers");
@@ -1031,7 +1031,7 @@  static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t
 
         total = 0;
 
-        if (virtqueue_pop(q->rx_vq, &elem) == 0) {
+        if (virtqueue_pop(q->rx_vq, &elem, &error_abort) == 0) {
             if (i == 0)
                 return -1;
             error_report("virtio-net unexpected empty queue: "
@@ -1134,7 +1134,7 @@  static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
         return num_packets;
     }
 
-    while (virtqueue_pop(q->tx_vq, &elem)) {
+    while (virtqueue_pop(q->tx_vq, &elem, &error_abort)) {
         ssize_t ret, len;
         unsigned int out_num = elem.out_num;
         struct iovec *out_sg = &elem.out_sg[0];
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index c9bea06..40ba03d 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -177,7 +177,7 @@  static int virtio_scsi_parse_req(VirtIOSCSIReq *req,
 static VirtIOSCSIReq *virtio_scsi_pop_req(VirtIOSCSI *s, VirtQueue *vq)
 {
     VirtIOSCSIReq *req = virtio_scsi_init_req(s, vq);
-    if (!virtqueue_pop(vq, &req->elem)) {
+    if (!virtqueue_pop(vq, &req->elem, &error_abort)) {
         virtio_scsi_free_req(req);
         return NULL;
     }
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 95b0643..e26c0a7 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -206,7 +206,7 @@  static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
     VirtQueueElement elem;
     MemoryRegionSection section;
 
-    while (virtqueue_pop(vq, &elem)) {
+    while (virtqueue_pop(vq, &elem, &error_abort)) {
         size_t offset = 0;
         uint32_t pfn;
 
@@ -246,7 +246,7 @@  static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
     size_t offset = 0;
     qemu_timeval tv;
 
-    if (!virtqueue_pop(vq, elem)) {
+    if (!virtqueue_pop(vq, elem, &error_abort)) {
         goto out;
     }
 
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 9e1bd75..c3cbdc3 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -56,7 +56,7 @@  static void chr_read(void *opaque, const void *buf, size_t size)
 
     offset = 0;
     while (offset < size) {
-        if (!virtqueue_pop(vrng->vq, &elem)) {
+        if (!virtqueue_pop(vrng->vq, &elem, &error_abort)) {
             break;
         }
         len = iov_from_buf(elem.in_sg, elem.in_num,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 1cd454b..e6f9f6b 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -481,29 +481,49 @@  fail:
     error_setg(errp, "virtio: error trying to map MMIO memory");
 }
 
-int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
+static void virtqueue_undo_map_sg(struct iovec *sg, hwaddr *addr,
+                                  size_t num_sg, int is_write)
+{
+    int i;
+
+    for (i = 0; i < num_sg; i++) {
+        cpu_physical_memory_unmap(sg[i].iov_base, sg[i].iov_len,
+                                  is_write, 0);
+    }
+}
+
+int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem, Error **errp)
 {
     unsigned int i, head, max;
+    int ret;
     hwaddr desc_pa = vq->vring.desc;
     VirtIODevice *vdev = vq->vdev;
+    Error *local_err = NULL;
 
-    if (!virtqueue_num_heads(vq, vq->last_avail_idx, &error_abort))
-        return 0;
+    ret = virtqueue_num_heads(vq, vq->last_avail_idx, &local_err);
+    if (ret <= 0) {
+        goto err;
+    }
 
     /* When we start there are none of either input nor output. */
     elem->out_num = elem->in_num = 0;
 
     max = vq->vring.num;
 
-    i = head = virtqueue_get_head(vq, vq->last_avail_idx++, &error_abort);
+    ret = virtqueue_get_head(vq, vq->last_avail_idx++, &local_err);
+    if (ret < 0) {
+        goto err;
+    }
+    head = i = ret;
+
     if (virtio_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
         vring_set_avail_event(vq, vq->last_avail_idx);
     }
 
     if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_INDIRECT) {
         if (vring_desc_len(vdev, desc_pa, i) % sizeof(VRingDesc)) {
-            error_report("Invalid size for indirect buffer table");
-            exit(1);
+            error_setg(errp, "Invalid size for indirect buffer table");
+            return -EINVAL;
         }
 
         /* loop over the indirect descriptor table */
@@ -518,15 +538,17 @@  int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 
         if (vring_desc_flags(vdev, desc_pa, i) & VRING_DESC_F_WRITE) {
             if (elem->in_num >= ARRAY_SIZE(elem->in_sg)) {
-                error_report("Too many write descriptors in indirect table");
-                exit(1);
+                error_setg(errp,
+                           "Too many write descriptors in indirect table");
+                return -EINVAL;
             }
             elem->in_addr[elem->in_num] = vring_desc_addr(vdev, desc_pa, i);
             sg = &elem->in_sg[elem->in_num++];
         } else {
             if (elem->out_num >= ARRAY_SIZE(elem->out_sg)) {
-                error_report("Too many read descriptors in indirect table");
-                exit(1);
+                error_setg(errp,
+                           "Too many read descriptors in indirect table");
+                return -EINVAL;
             }
             elem->out_addr[elem->out_num] = vring_desc_addr(vdev, desc_pa, i);
             sg = &elem->out_sg[elem->out_num++];
@@ -536,20 +558,31 @@  int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 
         /* If we've got too many, that implies a descriptor loop. */
         if ((elem->in_num + elem->out_num) > max) {
-            error_report("Looped descriptor");
-            exit(1);
+            error_setg(errp, "Looped descriptor");
+            return -EINVAL;
         }
-        i = virtqueue_next_desc(vdev, desc_pa, i, max, &error_abort);
-        if (i == max) {
+        ret = virtqueue_next_desc(vdev, desc_pa, i, max, &local_err);
+        if (ret < 0) {
+            goto err;
+        } else if (ret == max) {
             break;
         }
+        i = ret;
     }
 
     /* Now map what we have collected */
     virtqueue_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1,
-                     &error_abort);
+                     &local_err);
+    if (local_err) {
+        ret = -EINVAL;
+        goto err;
+    }
     virtqueue_map_sg(elem->out_sg, elem->out_addr, elem->out_num, 0,
-                     &error_abort);
+                     &local_err);
+    if (local_err) {
+        ret = -EINVAL;
+        goto err_unmap;
+    }
 
     elem->index = head;
 
@@ -557,6 +590,11 @@  int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
 
     trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
     return elem->in_num + elem->out_num;
+err_unmap:
+    virtqueue_undo_map_sg(elem->in_sg, elem->in_addr, elem->in_num, 1);
+err:
+    error_propagate(errp, local_err);
+    return ret;
 }
 
 /* virtio device */
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index a37ee64..c478f48 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -142,7 +142,7 @@  void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 void virtqueue_map_sg(struct iovec *sg, hwaddr *addr,
                       size_t num_sg, int is_write,
                       Error **errp);
-int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem);
+int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem, Error **errp);
 int virtqueue_avail_bytes(VirtQueue *vq, unsigned int in_bytes,
                           unsigned int out_bytes);
 void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,