Hello Rusty, On Thu, Nov 8, 2012 at 7:47 AM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote: > Asias He <asias@xxxxxxxxxx> writes: >> vhost-blk is an in-kernel virito-blk device accelerator. >> >> Due to lack of proper in-kernel AIO interface, this version converts >> guest's I/O request to bio and use submit_bio() to submit I/O directly. >> So this version any supports raw block device as guest's disk image, >> e.g. /dev/sda, /dev/ram0. We can add file based image support to >> vhost-blk once we have in-kernel AIO interface. There are some work in >> progress for in-kernel AIO interface from Dave Kleikamp and Zach Brown: >> >> http://marc.info/?l=linux-fsdevel&m=133312234313122 > > OK, this generally looks quite neat. There is one significant bug, > however: > >> +/* The block header is in the first and separate buffer. */ >> +#define BLK_HDR 0 > > You need to do a proper pull off the iovec; you can't simply assume > this. I'm working on fixing qemu, too. Yes. I have changed the code to handle the buffer without assumption about the layout already. Just haven't sent out the new version. I will send it out after the kvm forum. Cheers. > linux/drivers/vhost/net.c simply skips the header, you want something > which actually copies it from userspace: > > /* Returns 0, -EFAULT or -EINVAL (too short) */ > int copy_from_iovec_user(void *dst, size_t len, struct iovec *iov, int iov_nr); > int copy_to_iovec_user(struct iovec *iov, int iov_nr, const void *src, size_t len); > > These consume the iov in place. You could pass struct iovec **iov and > int * if you wanted to be really efficient (otherwise you have > zero-length iov entries at the front after you've pulled things off). > > This goes away: >> + if (hdr->type == VIRTIO_BLK_T_IN || hdr->type == VIRTIO_BLK_T_GET_ID) >> + iov_nr = in - 1; >> + else >> + iov_nr = out - 1; > > This becomes a simple assignment: > >> + /* The block data buffer follows block header buffer */ >> + req->iov = &vq->iov[BLK_HDR + 1]; > > This one actually requires iteration, since you should handle the case > where the last iov is zero length: > >> + /* The block status buffer follows block data buffer */ >> + req->status = vq->iov[iov_nr + 1].iov_base; > > This becomes copy_to_iovec_user: > >> + case VIRTIO_BLK_T_GET_ID: { >> + char id[VIRTIO_BLK_ID_BYTES]; >> + int len; >> + >> + ret = snprintf(id, VIRTIO_BLK_ID_BYTES, >> + "vhost-blk%d", blk->index); >> + if (ret < 0) >> + break; >> + len = ret; >> + ret = __copy_to_user(req->iov[0].iov_base, id, len); > > This becomes copy_from_iovec_user: > >> + if (unlikely(copy_from_user(&hdr, vq->iov[BLK_HDR].iov_base, >> + sizeof(hdr)))) { >> + vq_err(vq, "Failed to get block header!\n"); >> + vhost_discard_vq_desc(vq, 1); >> + break; >> + } > > The rest looks OK, at a glance. > > Thanks, > Rusty. > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Asias He -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
