Re: VIRTIO_BLK_T_SCSI_CMD handling in virtio-blk

[Hannes Reinecke <hare@suse.de>]

[-- 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