VIRTIO_BLK_T_SCSI_CMD handling in virtio-blk

VIRTIO_BLK_T_SCSI_CMD handling in virtio-blk

[Christoph Hellwig]

Currently virtio-blk just sends down the payload for packet command
requests, setting the VIRTIO_BLK_T_SCSI_CMD flag in the type field and
zeroing out the sector field.

But to make any sense of the payload of a packet command we need the
scsi command block (request->cmd) which specifies the operation,
location and length for this command.

All backends that I checked just fail VIRTIO_BLK_T_SCSI_CMD commands, so
AFAICS no harm is done.  But should we really keep this broken support
in the protocol around?  If we do want to support packet commands in
the future we should probably just add the command as the first S/G list
entry.

Is there an actual protocol spec for virtio-blk somewhere to write these
subtilities down?  Or an list of implementations to check at least?

Re: VIRTIO_BLK_T_SCSI_CMD handling in virtio-blk

[Christoph Hellwig]

Sorry for the second post, I though I had discarded the first one and
couldn't find it in my sent mail folder..

Re: VIRTIO_BLK_T_SCSI_CMD handling in virtio-blk

[Christian Borntraeger]

Am Wednesday 18 March 2009 07:09:19 schrieb Christoph Hellwig:
> Currently virtio-blk just sends down the payload for packet command
> requests, setting the VIRTIO_BLK_T_SCSI_CMD flag in the type field and
> zeroing out the sector field.
> 
> But to make any sense of the payload of a packet command we need the
> scsi command block (request->cmd) which specifies the operation,
> location and length for this command.
> 
> All backends that I checked just fail VIRTIO_BLK_T_SCSI_CMD commands, so
> AFAICS no harm is done.  But should we really keep this broken support
> in the protocol around?  If we do want to support packet commands in
> the future we should probably just add the command as the first S/G list
> entry.

Hannes did some implementation for SCSI command passthrough:
https://lists.linux-foundation.org/pipermail/virtualization/2008-August/011629.html

After some addon fix this was working pretty well. I dont know why Hannes never
pushed the final version upstream.

Christian

Re: VIRTIO_BLK_T_SCSI_CMD handling in virtio-blk

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

Christian Borntraeger wrote:
> Am Wednesday 18 March 2009 07:09:19 schrieb Christoph Hellwig:
>> Currently virtio-blk just sends down the payload for packet command
>> requests, setting the VIRTIO_BLK_T_SCSI_CMD flag in the type field and
>> zeroing out the sector field.
>>
>> But to make any sense of the payload of a packet command we need the
>> scsi command block (request->cmd) which specifies the operation,
>> location and length for this command.
>>
>> All backends that I checked just fail VIRTIO_BLK_T_SCSI_CMD commands, so
>> AFAICS no harm is done.  But should we really keep this broken support
>> in the protocol around?  If we do want to support packet commands in
>> the future we should probably just add the command as the first S/G list
>> entry.
> 
> Hannes did some implementation for SCSI command passthrough:
> https://lists.linux-foundation.org/pipermail/virtualization/2008-August/011629.html
> 
> After some addon fix this was working pretty well. I dont know why Hannes never
> pushed the final version upstream.
> 
Sorry, being busy. Someone keeps sending me multipath bugs to worry about.
Patch is attached.

Please do keep me in the loop here, I'm not subscribed to lkml.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[-- Attachment #2: virtio-blk-scsi-passthrough --]
[-- Type: text/plain, Size: 3381 bytes --]

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4225109..46f03d2 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -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;
diff --git a/include/linux/virtio_blk.h b/include/linux/virtio_blk.h
index c1aef85..089e596 100644
--- a/include/linux/virtio_blk.h
+++ b/include/linux/virtio_blk.h
@@ -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

Re: VIRTIO_BLK_T_SCSI_CMD handling in virtio-blk

[Christian Borntraeger]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1111 bytes --]

Am Wednesday 18 March 2009 10:16:43 schrieb Hannes Reinecke:> -               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

I think we identified a bug in the old patch:http://kerneltrap.org/mailarchive/linux-kvm/2008/8/29/3127594http://kerneltrap.org/index.php?q=mailarchive/linux-kvm/2008/8/29/3128124
Is this fixup still valid? 
-		if (blk_pc_request(vbr->req)) {+		if (blk_pc_request(vbr->req) && len >= sizeof(vbr->in_hdr)) {
I forgot almost all details about the problem.
Christianÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: VIRTIO_BLK_T_SCSI_CMD handling in virtio-blk

[Christoph Hellwig]

Ah nice.  Still needs some work to make the SG_IO submission async
and abstrat the Linux-specific ioctl out of the common virtio-blk.c
code, but that should be a fallout of the scsi-generic layer I'm hacking
on for qemu now.

Re: VIRTIO_BLK_T_SCSI_CMD handling in virtio-blk

[Rusty Russell]

On Wednesday 18 March 2009 16:39:19 Christoph Hellwig wrote:
> All backends that I checked just fail VIRTIO_BLK_T_SCSI_CMD commands, so
> AFAICS no harm is done.  But should we really keep this broken support
> in the protocol around?  If we do want to support packet commands in
> the future we should probably just add the command as the first S/G list
> entry.

It was pretty much a placeholder for "eject".  Feel free to be the one to
define it.

But note that we chose not to use sg boundaries as part of the protocol, so
if there's going to be more than one entry, you'll need some metadata.

> Is there an actual protocol spec for virtio-blk somewhere to write these
> subtilities down?  Or an list of implementations to check at least?

No, but I look forward to reading your spec :)

Thanks!
Rusty.