Linux Virtualization - Apr 2014 - [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger
wrote:
> From: Nicholas Bellinger <nab at linux-iscsi.org>
> 
> This patch updates virtscsi_probe() to setup necessary Scsi_Host
> level protection resources. (currently hardcoded to 1)
> 
> It changes virtscsi_add_cmd() to attach outgoing / incoming
> protection SGLs preceeding the data payload, and is using the
> new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> to signal to vhost/scsi how many prot_sgs to expect.
> 
> v3 changes:
>   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> 
> v2 changes:
>   - Make protection buffer come before data buffer (Paolo)
>   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> 
> Cc: Paolo Bonzini <pbonzini at redhat.com>
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: Martin K. Petersen <martin.petersen at oracle.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Hannes Reinecke <hare at suse.de>
> Cc: Sagi Grimberg <sagig at dev.mellanox.co.il>
> Cc: H. Peter Anvin <hpa at zytor.com>
> Signed-off-by: Nicholas Bellinger <nab at linux-iscsi.org>
OK but we need to document the new interface in the spec
(and incidentially, this will be useful to verify the assumptions
made here and on the host side).
Could you please submit this proposal to the OASIS Virtio TC
for inclusion into the next spec draft?
Ideally as a patch against the tex source, but a prose
description would do as well.
The TC meets on a bi-weekly basis, we should be able to ratify
this quickly.

> ---
>  drivers/scsi/virtio_scsi.c |   78
++++++++++++++++++++++++++++++++++----------
>  1 file changed, 60 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 16bfd50..68d8d1b 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -37,6 +37,7 @@ struct virtio_scsi_cmd {
>  	struct completion *comp;
>  	union {
>  		struct virtio_scsi_cmd_req       cmd;
> +		struct virtio_scsi_cmd_req_pi    cmd_pi;
>  		struct virtio_scsi_ctrl_tmf_req  tmf;
>  		struct virtio_scsi_ctrl_an_req   an;
>  	} req;
> @@ -440,7 +441,7 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
>  			    size_t req_size, size_t resp_size, gfp_t gfp)
>  {
>  	struct scsi_cmnd *sc = cmd->sc;
> -	struct scatterlist *sgs[4], req, resp;
> +	struct scatterlist *sgs[6], req, resp;
>  	struct sg_table *out, *in;
>  	unsigned out_num = 0, in_num = 0;
>  
> @@ -458,16 +459,24 @@ static int virtscsi_add_cmd(struct virtqueue *vq,
>  	sgs[out_num++] = &req;
>  
>  	/* Data-out buffer.  */
> -	if (out)
> +	if (out) {
> +		/* Place WRITE protection SGLs before Data OUT payload */
> +		if (scsi_prot_sg_count(sc))
> +			sgs[out_num++] = scsi_prot_sglist(sc);
>  		sgs[out_num++] = out->sgl;
> +	}
>  
>  	/* Response header.  */
>  	sg_init_one(&resp, &cmd->resp, resp_size);
>  	sgs[out_num + in_num++] = &resp;
>  
>  	/* Data-in buffer */
> -	if (in)
> +	if (in) {
> +		/* Place READ protection SGLs before Data IN payload */
> +		if (scsi_prot_sg_count(sc))
> +			sgs[out_num + in_num++] = scsi_prot_sglist(sc);
>  		sgs[out_num + in_num++] = in->sgl;
> +	}
>  
>  	return virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, gfp);
>  }
> @@ -492,12 +501,36 @@ static int virtscsi_kick_cmd(struct virtio_scsi_vq
*vq,
>  	return err;
>  }
>  
> +static void virtio_scsi_init_hdr(struct virtio_scsi_cmd_req *cmd,
> +				 struct scsi_cmnd *sc)
> +{
> +	cmd->lun[0] = 1;
> +	cmd->lun[1] = sc->device->id;
> +	cmd->lun[2] = (sc->device->lun >> 8) | 0x40;
> +	cmd->lun[3] = sc->device->lun & 0xff;
> +	cmd->tag = (unsigned long)sc;
> +	cmd->task_attr = VIRTIO_SCSI_S_SIMPLE;
> +	cmd->prio = 0;
> +	cmd->crn = 0;
> +}
> +
> +static void virtio_scsi_init_hdr_pi(struct virtio_scsi_cmd_req_pi *cmd_pi,
> +				    struct scsi_cmnd *sc)
> +{
> +	virtio_scsi_init_hdr((struct virtio_scsi_cmd_req *)cmd_pi, sc);
> +
> +	if (sc->sc_data_direction == DMA_TO_DEVICE)
> +		cmd_pi->do_pi_niov = scsi_prot_sg_count(sc);
> +	else if (sc->sc_data_direction == DMA_FROM_DEVICE)
> +		cmd_pi->di_pi_niov = scsi_prot_sg_count(sc);
> +}
> +
>  static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
>  				 struct virtio_scsi_vq *req_vq,
>  				 struct scsi_cmnd *sc)
>  {
>  	struct virtio_scsi_cmd *cmd;
> -	int ret;
> +	int ret, req_size;
>  
>  	struct Scsi_Host *shost = virtio_scsi_host(vscsi->vdev);
>  	BUG_ON(scsi_sg_count(sc) > shost->sg_tablesize);
> @@ -515,22 +548,20 @@ static int virtscsi_queuecommand(struct virtio_scsi
*vscsi,
>  
>  	memset(cmd, 0, sizeof(*cmd));
>  	cmd->sc = sc;
> -	cmd->req.cmd = (struct virtio_scsi_cmd_req){
> -		.lun[0] = 1,
> -		.lun[1] = sc->device->id,
> -		.lun[2] = (sc->device->lun >> 8) | 0x40,
> -		.lun[3] = sc->device->lun & 0xff,
> -		.tag = (unsigned long)sc,
> -		.task_attr = VIRTIO_SCSI_S_SIMPLE,
> -		.prio = 0,
> -		.crn = 0,
> -	};
>  
>  	BUG_ON(sc->cmd_len > VIRTIO_SCSI_CDB_SIZE);
> -	memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
>  
> -	if (virtscsi_kick_cmd(req_vq, cmd,
> -			      sizeof cmd->req.cmd, sizeof cmd->resp.cmd,
> +	if (virtio_has_feature(vscsi->vdev, VIRTIO_SCSI_F_T10_PI)) {
> +		virtio_scsi_init_hdr_pi(&cmd->req.cmd_pi, sc);
> +		memcpy(cmd->req.cmd_pi.cdb, sc->cmnd, sc->cmd_len);
> +		req_size = sizeof(cmd->req.cmd_pi);
> +	} else {
> +		virtio_scsi_init_hdr(&cmd->req.cmd, sc);
> +		memcpy(cmd->req.cmd.cdb, sc->cmnd, sc->cmd_len);
> +		req_size = sizeof(cmd->req.cmd);
> +	}
> +
> +	if (virtscsi_kick_cmd(req_vq, cmd, req_size, sizeof(cmd->resp.cmd),
>  			      GFP_ATOMIC) == 0)
>  		ret = 0;
>  	else
> @@ -871,7 +902,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
>  {
>  	struct Scsi_Host *shost;
>  	struct virtio_scsi *vscsi;
> -	int err;
> +	int err, host_prot;
>  	u32 sg_elems, num_targets;
>  	u32 cmd_per_lun;
>  	u32 num_queues;
> @@ -921,6 +952,16 @@ static int virtscsi_probe(struct virtio_device *vdev)
>  	shost->max_id = num_targets;
>  	shost->max_channel = 0;
>  	shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;
> +
> +	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_T10_PI)) {
> +		host_prot = SHOST_DIF_TYPE1_PROTECTION | SHOST_DIF_TYPE2_PROTECTION |
> +			    SHOST_DIF_TYPE3_PROTECTION | SHOST_DIX_TYPE1_PROTECTION |
> +			    SHOST_DIX_TYPE2_PROTECTION | SHOST_DIX_TYPE3_PROTECTION;
> +
> +		scsi_host_set_prot(shost, host_prot);
> +		scsi_host_set_guard(shost, SHOST_DIX_GUARD_CRC);
> +	}
> +
>  	err = scsi_add_host(shost, &vdev->dev);
>  	if (err)
>  		goto scsi_add_host_failed;
> @@ -990,6 +1031,7 @@ static struct virtio_device_id id_table[] = {
>  static unsigned int features[] = {
>  	VIRTIO_SCSI_F_HOTPLUG,
>  	VIRTIO_SCSI_F_CHANGE,
> +	VIRTIO_SCSI_F_T10_PI,
>  };
>  
>  static struct virtio_driver virtio_scsi_driver = {
> -- 
> 1.7.10.4

On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin
wrote:
> On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab at linux-iscsi.org>
> > 
> > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > level protection resources. (currently hardcoded to 1)
> > 
> > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > protection SGLs preceeding the data payload, and is using the
> > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > to signal to vhost/scsi how many prot_sgs to expect.
> > 
> > v3 changes:
> >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header (Paolo)
> > 
> > v2 changes:
> >   - Make protection buffer come before data buffer (Paolo)
> >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > 
> > Cc: Paolo Bonzini <pbonzini at redhat.com>
> > Cc: Michael S. Tsirkin <mst at redhat.com>
> > Cc: Martin K. Petersen <martin.petersen at oracle.com>
> > Cc: Christoph Hellwig <hch at lst.de>
> > Cc: Hannes Reinecke <hare at suse.de>
> > Cc: Sagi Grimberg <sagig at dev.mellanox.co.il>
> > Cc: H. Peter Anvin <hpa at zytor.com>
> > Signed-off-by: Nicholas Bellinger <nab at linux-iscsi.org>
> 
> OK but we need to document the new interface in the spec
> (and incidentially, this will be useful to verify the assumptions
> made here and on the host side).
> Could you please submit this proposal to the OASIS Virtio TC
> for inclusion into the next spec draft?
> Ideally as a patch against the tex source, but a prose
> description would do as well.
Most certainly.  Please give me a bit to follow up on this, as the next
couple of days are going to be hellishly busy..
> The TC meets on a bi-weekly basis, we should be able to ratify
> this quickly.
> 
Aside from that, please consider ACK'ing the vhost specific changes so
these can make it into v3.15-rc1 code.

--nab

On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger
wrote:
> On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab at linux-iscsi.org>
> > > 
> > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > level protection resources. (currently hardcoded to 1)
> > > 
> > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > protection SGLs preceeding the data payload, and is using the
> > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > to signal to vhost/scsi how many prot_sgs to expect.
> > > 
> > > v3 changes:
> > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header
(Paolo)
> > > 
> > > v2 changes:
> > >   - Make protection buffer come before data buffer (Paolo)
> > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > 
> > > Cc: Paolo Bonzini <pbonzini at redhat.com>
> > > Cc: Michael S. Tsirkin <mst at redhat.com>
> > > Cc: Martin K. Petersen <martin.petersen at oracle.com>
> > > Cc: Christoph Hellwig <hch at lst.de>
> > > Cc: Hannes Reinecke <hare at suse.de>
> > > Cc: Sagi Grimberg <sagig at dev.mellanox.co.il>
> > > Cc: H. Peter Anvin <hpa at zytor.com>
> > > Signed-off-by: Nicholas Bellinger <nab at linux-iscsi.org>
> > 
> > OK but we need to document the new interface in the spec
> > (and incidentially, this will be useful to verify the assumptions
> > made here and on the host side).
> > Could you please submit this proposal to the OASIS Virtio TC
> > for inclusion into the next spec draft?
> > Ideally as a patch against the tex source, but a prose
> > description would do as well.
> 
> Most certainly.  Please give me a bit to follow up on this, as the next
> couple of days are going to be hellishly busy..
> 
> > The TC meets on a bi-weekly basis, we should be able to ratify
> > this quickly.
> > 
> 
> Aside from that, please consider ACK'ing the vhost specific changes so
> these can make it into v3.15-rc1 code.
> 
> --nab
> 
Hmm but what if the TC wants to change the interface somewhat?
I guess we'll still be able to fix it after the merge window -
(or worst case, revert the change) is this what you are suggesting?

-- 
MST

On Mon, Apr 07, 2014 at 01:56:59AM -0700, Nicholas A. Bellinger
wrote:
> On Mon, 2014-04-07 at 11:45 +0300, Michael S. Tsirkin wrote:
> > On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab at linux-iscsi.org>
> > > 
> > > This patch updates virtscsi_probe() to setup necessary Scsi_Host
> > > level protection resources. (currently hardcoded to 1)
> > > 
> > > It changes virtscsi_add_cmd() to attach outgoing / incoming
> > > protection SGLs preceeding the data payload, and is using the
> > > new virtio_scsi_cmd_req_pi->d[oi],pi_niv field to signal
> > > to signal to vhost/scsi how many prot_sgs to expect.
> > > 
> > > v3 changes:
> > >   - Use VIRTIO_SCSI_F_T10_PI to determine PI or non PI header
(Paolo)
> > > 
> > > v2 changes:
> > >   - Make protection buffer come before data buffer (Paolo)
> > >   - Enable virtio_scsi_cmd_req_pi usage (Paolo)
> > > 
> > > Cc: Paolo Bonzini <pbonzini at redhat.com>
> > > Cc: Michael S. Tsirkin <mst at redhat.com>
> > > Cc: Martin K. Petersen <martin.petersen at oracle.com>
> > > Cc: Christoph Hellwig <hch at lst.de>
> > > Cc: Hannes Reinecke <hare at suse.de>
> > > Cc: Sagi Grimberg <sagig at dev.mellanox.co.il>
> > > Cc: H. Peter Anvin <hpa at zytor.com>
> > > Signed-off-by: Nicholas Bellinger <nab at linux-iscsi.org>
> > 
> > OK but we need to document the new interface in the spec
> > (and incidentially, this will be useful to verify the assumptions
> > made here and on the host side).
> > Could you please submit this proposal to the OASIS Virtio TC
> > for inclusion into the next spec draft?
> > Ideally as a patch against the tex source, but a prose
> > description would do as well.
> 
> Most certainly.  Please give me a bit to follow up on this, as the next
> couple of days are going to be hellishly busy..
Ping.
We really need to get this moving to have the interface reviewed for
the next merge window.

> > The TC meets on a bi-weekly basis, we should be able to ratify
> > this quickly.
> > 
> 
> Aside from that, please consider ACK'ing the vhost specific changes so
> these can make it into v3.15-rc1 code.
> 
> --nab
>