Skip to contentSkip to conversation

SCSI passthrough for virtio-blk

RFC PATCH In review Hannes Reinecke 2 participants 7 messages Aug 29, 2008

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 SCSI passthrough for virtio-blk

Hi all,

I got bored and implemented SCSI passthrough for the virtio-blk driver. Principle is quite simple, just put the missing fields (cdb, sense and status header) on the virtio queue and then call the SG_IO ioctl on the host.

So when using '-drive file=/dev/sgXX,if=virtio,format=host_device' you can happily call any sg_XX command on the resulting vdX device. Quite neat, methinks. And it's even backwards compatible, so each of these patches should work without the other one applied.

As one would have guessed there are two patches, one for the linux kernel to modify the virtio-blk driver in the guest and one for the qemu/kvm userland program to modify the virtio-blk driver on the host. This patch is relative to avi's kvm-userland tree from kernel.org.

As usual, comments etc to me.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

virtio: Implement SCSI passthrough for virtio-blk

This patch implements SCSI passthrough for any virtio-blk device. The data on the virtio queue will only be modified for a SCSI command, so the normal I/O flow is unchanged.

drivers/block/virtio_blk.c +40 -19
index 4225109..46f03d2 100644
@@ -35,6 +35,7 @@ struct virtblk_req
 	struct list_head list;
 	struct request *req;
 	struct virtio_blk_outhdr out_hdr;
+	struct virtio_blk_inhdr in_hdr;
 	u8 status;
 };
 
@@ -47,20 +48,29 @@ static void blk_done(struct virtqueue *vq)
 
 	spin_lock_irqsave(&vblk->lock, flags);
 	while ((vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len)) != NULL) {
-		int uptodate;
+		int error;
+		unsigned int bytes;
 		switch (vbr->status) {
 		case VIRTIO_BLK_S_OK:
-			uptodate = 1;
+			error = 0;
 			break;
 		case VIRTIO_BLK_S_UNSUPP:
-			uptodate = -ENOTTY;
+			error = -ENOTTY;
 			break;
 		default:
-			uptodate = 0;
+			error = -EIO;
 			break;
 		}
 
-		end_dequeued_request(vbr->req, uptodate);
+		if (blk_pc_request(vbr->req)) {
+			vbr->req->data_len = vbr->in_hdr.residual;
+			bytes = vbr->in_hdr.data_len;
+			vbr->req->sense_len = vbr->in_hdr.sense_len;
+			vbr->req->errors = vbr->in_hdr.status;
+		} else
+			bytes = blk_rq_bytes(vbr->req);
+
+		__blk_end_request(vbr->req, error, bytes);
 		list_del(&vbr->list);
 		mempool_free(vbr, vblk->pool);
 	}
@@ -72,7 +82,7 @@ static void blk_done(struct virtqueue *vq)
 static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
 		   struct request *req)
 {
-	unsigned long num, out, in;
+	unsigned long num, out = 0, in = 0;
 	struct virtblk_req *vbr;
 
 	vbr = mempool_alloc(vblk->pool, GFP_ATOMIC);
@@ -99,20 +109,31 @@ static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
 
 	/* This init could be done at vblk creation time */
 	sg_init_table(vblk->sg, VIRTIO_MAX_SG);
-	sg_set_buf(&vblk->sg[0], &vbr->out_hdr, sizeof(vbr->out_hdr));
-	num = blk_rq_map_sg(q, vbr->req, vblk->sg+1);
-	sg_set_buf(&vblk->sg[num+1], &vbr->status, sizeof(vbr->status));
-
-	if (rq_data_dir(vbr->req) == WRITE) {
-		vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
-		out = 1 + num;
-		in = 1;
-	} else {
-		vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
-		out = 1;
-		in = 1 + num;
+	sg_set_buf(&vblk->sg[out], &vbr->out_hdr, sizeof(vbr->out_hdr));
+	out++;
+	if (blk_pc_request(vbr->req)) {
+		sg_set_buf(&vblk->sg[out], vbr->req->cmd, vbr->req->cmd_len);
+		out++;
+	}
+	num = blk_rq_map_sg(q, vbr->req, vblk->sg+out);
+	if (blk_pc_request(vbr->req)) {
+		sg_set_buf(&vblk->sg[num+out+in], vbr->req->sense, 96);
+		in++;
+		sg_set_buf(&vblk->sg[num+out+in], &vbr->in_hdr,
+			   sizeof(vbr->in_hdr));
+		in++;
+	}
+	sg_set_buf(&vblk->sg[num+out+in], &vbr->status, sizeof(vbr->status));
+	in++;
+	if (num) {
+		if (rq_data_dir(vbr->req) == WRITE) {
+			vbr->out_hdr.type |= VIRTIO_BLK_T_OUT;
+			out += num;
+		} else {
+			vbr->out_hdr.type |= VIRTIO_BLK_T_IN;
+			in += num;
+		}
 	}
-
 	if (vblk->vq->vq_ops->add_buf(vblk->vq, vblk->sg, out, in, vbr)) {
 		mempool_free(vbr, vblk->pool);
 		return false;
include/linux/virtio_blk.h +8 -0
index c1aef85..089e596 100644
@@ -54,6 +54,14 @@ struct virtio_blk_outhdr
 	__u64 sector;
 };
 
+struct virtio_blk_inhdr
+{
+	__u32 status;
+	__u32 data_len;
+	__u32 sense_len;
+	__u32 residual;
+};
+
 /* And this is the final byte of the write scatter-gather list. */
 #define VIRTIO_BLK_S_OK		0
 #define VIRTIO_BLK_S_IOERR	1

virtio: Implement SCSI passthrough for virtio-blk

This patch implements SCSI passthrough handling for Qemu/KVM. It's actually quite simple; just construct a SG header and call SG_IO ioctl on the underlying block device. But we have to make sure to map the result and error codes correctly upon return.

qemu/hw/virtio-blk.c +114 -4

148 lines not on this page · raw message

#1 lore raw

Am Freitag, 29. August 2008 schrieb Hannes Reinecke:

Quoted 4 lines

So when using '-drive file=/dev/sgXX,if=virtio,format=host_device' you can happily call any sg_XX command on the resulting vdX device. Quite neat, methinks. And it's even backwards compatible, so each of these patches should work without the other one applied.

Does not work here. If the host does not support the pass-through, the device drivers waits for an response. I tried sdparm /dev/vda with a patched kernel and an unpatched userspace.

I think you should use a feature bit to avoid problems.

Christian

#2 lore raw

Hi Christian,

Quoted 8 lines · Christian Borntraeger

Am Freitag, 29. August 2008 schrieb Hannes Reinecke:

Quoted 4 lines

So when using '-drive file=/dev/sgXX,if=virtio,format=host_device' you can happily call any sg_XX command on the resulting vdX device. Quite neat, methinks. And it's even backwards compatible, so each of these patches should work without the other one applied.

Does not work here. If the host does not support the pass-through, the device drivers waits for an response. I tried sdparm /dev/vda with a patched kernel and an unpatched userspace.

Hmm. Works here, using an unpatched kvm-73.
Which version did you use?

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
#3 lore raw

Am Freitag, 29. August 2008 schrieb Hannes Reinecke:

Quoted 2 lines

Hmm. Works here, using an unpatched kvm-73.
Which version did you use?

I use the s390 userspace prototype kuli which uses an virtio transport similar to lguest.

I retried and it seems to race. Most of the time it works fine, but sometimes sdparm hangs. I will have a 2nd look.

sysrq-t gives me the following trace:

Call Trace:
([<040000000755bc78>] 0x40000000755bc78)
sdparm        D 000000000043659e     0  2493      1
000000000012004a 000000000744f740 000000000744f778 001896469fd23785
       000000000744f778 00000000009e5500 000000000043f230 0000000000120130
       000000000744f778 0000000006d39400 0000000006d39f80 0000000000000001
       00000000009e6f00 00000000076bf8e8 000000000744f7c8 0000000007530670
       000000000043f610 0000000000435e66 000000000744f7c8 000000000744f868
Call Trace:
([<0000000000435e66>] schedule+0x32e/0x7ec)
 [<000000000043659e>] schedule_timeout+0xba/0x10c
 [<00000000004358da>] wait_for_common+0xbe/0x1a8
 [<000000000027ec3e>] blk_execute_rq+0x86/0xc4
 [<0000000000282768>] sg_io+0x1a4/0x360
 [<0000000000282f8c>] scsi_cmd_ioctl+0x2bc/0x3f0
 [<00000000002c3108>] virtblk_ioctl+0x44/0x58
 [<000000000027ff18>] blkdev_driver_ioctl+0x98/0xa4
 [<000000000027ffd8>] blkdev_ioctl+0xb4/0x7f8
 [<00000000001e1572>] block_ioctl+0x3a/0x48
 [<00000000001bca0a>] vfs_ioctl+0x52/0xdc
 [<00000000001bcb0a>] do_vfs_ioctl+0x76/0x350
 [<00000000001bce6e>] sys_ioctl+0x8a/0xa0
 [<000000000011282c>] sysc_tracego+0xe/0x14
 [<0000020000114286>] 0x20000114286
#4 lore raw

Hi Christian,

Quoted 26 lines · Christian Borntraeger

Am Freitag, 29. August 2008 schrieb Hannes Reinecke:

Quoted 2 lines

Hmm. Works here, using an unpatched kvm-73.
Which version did you use?

I use the s390 userspace prototype kuli which uses an virtio transport similar to lguest.

I retried and it seems to race. Most of the time it works fine, but sometimes sdparm hangs. I will have a 2nd look.

sysrq-t gives me the following trace:

Call Trace:
([<040000000755bc78>] 0x40000000755bc78)
sdparm        D 000000000043659e     0  2493      1
000000000012004a 000000000744f740 000000000744f778 001896469fd23785
       000000000744f778 00000000009e5500 000000000043f230 0000000000120130
       000000000744f778 0000000006d39400 0000000006d39f80 0000000000000001
       00000000009e6f00 00000000076bf8e8 000000000744f7c8 0000000007530670
       000000000043f610 0000000000435e66 000000000744f7c8 000000000744f868
Call Trace:
([<0000000000435e66>] schedule+0x32e/0x7ec)
 [<000000000043659e>] schedule_timeout+0xba/0x10c
 [<00000000004358da>] wait_for_common+0xbe/0x1a8
 [<000000000027ec3e>] blk_execute_rq+0x86/0xc4
 [<0000000000282768>] sg_io+0x1a4/0x360
 [<0000000000282f8c>] scsi_cmd_ioctl+0x2bc/0x3f0
 [<00000000002c3108>] virtblk_ioctl+0x44/0x58
Quoted 8 lines
 [<000000000027ff18>] blkdev_driver_ioctl+0x98/0xa4
 [<000000000027ffd8>] blkdev_ioctl+0xb4/0x7f8
 [<00000000001e1572>] block_ioctl+0x3a/0x48
 [<00000000001bca0a>] vfs_ioctl+0x52/0xdc
 [<00000000001bcb0a>] do_vfs_ioctl+0x76/0x350
 [<00000000001bce6e>] sys_ioctl+0x8a/0xa0
 [<000000000011282c>] sysc_tracego+0xe/0x14
 [<0000020000114286>] 0x20000114286

I'm tempted to say 'not my fault'; the submitted SCSI request on the _host_ hangs and doesn't come back.
Looks more like a SCSI problem on the host ...

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
#5 lore raw

Am Freitag, 29. August 2008 schrieb Hannes Reinecke:

Quoted 16 lines
Quoted 15 lines

([<0000000000435e66>] schedule+0x32e/0x7ec)

 [<000000000043659e>] schedule_timeout+0xba/0x10c
 [<00000000004358da>] wait_for_common+0xbe/0x1a8
 [<000000000027ec3e>] blk_execute_rq+0x86/0xc4
 [<0000000000282768>] sg_io+0x1a4/0x360
 [<0000000000282f8c>] scsi_cmd_ioctl+0x2bc/0x3f0
 [<00000000002c3108>] virtblk_ioctl+0x44/0x58
 [<000000000027ff18>] blkdev_driver_ioctl+0x98/0xa4
 [<000000000027ffd8>] blkdev_ioctl+0xb4/0x7f8
 [<00000000001e1572>] block_ioctl+0x3a/0x48
 [<00000000001bca0a>] vfs_ioctl+0x52/0xdc
 [<00000000001bcb0a>] do_vfs_ioctl+0x76/0x350
 [<00000000001bce6e>] sys_ioctl+0x8a/0xa0
 [<000000000011282c>] sysc_tracego+0xe/0x14
 [<0000020000114286>] 0x20000114286
Quoted 3 lines

I'm tempted to say 'not my fault'; the submitted SCSI request on the _host_ hangs and doesn't come back.
Looks more like a SCSI problem on the host ...

It is a guest process trace.

Anyway, after you suggested to look at the len field it appeared to me, that the new code sets the data_len, sense_len and changes the number of reported bytes to random values even if the host returns VIRTIO_BLK_S_UNSUPP.

Moving these assignments to the VIRTIO_BLK_S_OK case seems to fix it. Can you test if SG_IO still works for you after applying this patch:

---
 drivers/block/virtio_blk.c |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
drivers/block/virtio_blk.c +8 -9
Index: kvm/drivers/block/virtio_blk.c
===================================================================
@@ -50,9 +50,17 @@ static void blk_done(struct virtqueue *v
 	while ((vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len)) != NULL) {
 		int error;
 		unsigned int bytes;
+
+		bytes = blk_rq_bytes(vbr->req);
 		switch (vbr->status) {
 		case VIRTIO_BLK_S_OK:
 			error = 0;
+			if (blk_pc_request(vbr->req)) {
+				vbr->req->data_len = vbr->in_hdr.residual;
+				bytes = vbr->in_hdr.data_len;
+				vbr->req->sense_len = vbr->in_hdr.sense_len;
+				vbr->req->errors = vbr->in_hdr.status;
+			}
 			break;
 		case VIRTIO_BLK_S_UNSUPP:
 			error = -ENOTTY;
@@ -61,15 +69,6 @@ static void blk_done(struct virtqueue *v
 			error = -EIO;
 			break;
 		}
-
-		if (blk_pc_request(vbr->req)) {
-			vbr->req->data_len = vbr->in_hdr.residual;
-			bytes = vbr->in_hdr.data_len;
-			vbr->req->sense_len = vbr->in_hdr.sense_len;
-			vbr->req->errors = vbr->in_hdr.status;
-		} else
-			bytes = blk_rq_bytes(vbr->req);
-
 		__blk_end_request(vbr->req, error, bytes);
 		list_del(&vbr->list);
 		mempool_free(vbr, vblk->pool);
#6 lore raw
In reply to Hannes Reinecke

Thanks for your feedback.

Here is a second try to allows to propagate scsi error code from host->guest. Makes sense?

---
 drivers/block/virtio_blk.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/block/virtio_blk.c +1 -1
Index: kvm/drivers/block/virtio_blk.c
===================================================================
@@ -62,7 +62,7 @@ static void blk_done(struct virtqueue *v
 			break;
 		}
 
-		if (blk_pc_request(vbr->req)) {
+		if (blk_pc_request(vbr->req) && len >= sizeof(vbr->in_hdr)) {
 			vbr->req->data_len = vbr->in_hdr.residual;
 			bytes = vbr->in_hdr.data_len;
 			vbr->req->sense_len = vbr->in_hdr.sense_len;