Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

On Sun, Apr 06, 2014 at 09:32:09PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@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@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Nicholas Bellinger <nab@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

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

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@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@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > Cc: H. Peter Anvin <hpa@zytor.com>
> > Signed-off-by: Nicholas Bellinger <nab@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

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

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@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@redhat.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Cc: Hannes Reinecke <hare@suse.de>
> > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > Signed-off-by: Nicholas Bellinger <nab@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

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Mon, 2014-04-07 at 12:02 +0300, Michael S. Tsirkin wrote:
> 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@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@redhat.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > Signed-off-by: Nicholas Bellinger <nab@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 don't have a objection to changing the interface post-merge.

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

Yes, but I would think that it is actually fixable.  ;)

Otherwise, a v3.16 merge is an option as well.  It's really your +
Paolo's call here. 

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Paolo Bonzini]

Il 07/04/2014 05:13, Nicholas A. Bellinger ha scritto:
>> > 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?
>> >
> Yes, but I would think that it is actually fixable.  ;)
>
> Otherwise, a v3.16 merge is an option as well.  It's really your +
> Paolo's call here.

With s/niov/nbytes/, I think the interface is fine.  If you can do it 
this week, it is your call as target maintainer...

I don't have to do much more than give my acked-by for the changes to 
the virtio-scsi drivers, it's up to you whether to use it for 3.15 or 3.16.

Paolo

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

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@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@redhat.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Cc: Hannes Reinecke <hare@suse.de>
> > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > Signed-off-by: Nicholas Bellinger <nab@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
> 

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> 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@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@redhat.com>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > Signed-off-by: Nicholas Bellinger <nab@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.
> 

Hi MST,

So I've finally got some cycles to get back to this code, and wanted to
verify the outstanding items you had previously raised:

- Convert vhost-scsi to be independent of IOV layout using 
  memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
  operation..?)
- Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
- Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
  iovecs.
- Ensure virtio_scsi_cmd_req_pi is naturally aligned
- Figure out why QEMU is not acking (any) vhost-scsi feature bits

Is there anything else that you'd like to see for an initial merge, or
other issues that need to be addressed with the above..?

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 12:07:03PM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> > 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@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@redhat.com>
> > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > > Signed-off-by: Nicholas Bellinger <nab@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.
> > 
> 
> Hi MST,
> 
> So I've finally got some cycles to get back to this code, and wanted to
> verify the outstanding items you had previously raised:
> 
> - Convert vhost-scsi to be independent of IOV layout using 
>   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
>   operation..?)

Ideally yes.

> - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
>   iovecs.
> - Ensure virtio_scsi_cmd_req_pi is naturally aligned

It turns out other virtio scsi commands aren't aligned?
If true we don't need to make an exception here.

> - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> 
> Is there anything else that you'd like to see for an initial merge, or
> other issues that need to be addressed with the above..?
> 
> --nab

FYI Paolo sent a spec patch for the feature.
Have you seen it?

-- 
MST

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Mon, 2014-05-19 at 22:15 +0300, Michael S. Tsirkin wrote:
> On Mon, May 19, 2014 at 12:07:03PM -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> > > 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@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@redhat.com>
> > > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > > > Signed-off-by: Nicholas Bellinger <nab@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.
> > > 
> > 
> > Hi MST,
> > 
> > So I've finally got some cycles to get back to this code, and wanted to
> > verify the outstanding items you had previously raised:
> > 
> > - Convert vhost-scsi to be independent of IOV layout using 
> >   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
> >   operation..?)
> 
> Ideally yes.

Er, so changing vhost-scsi to be independent of IOV layout will have the
side effect of breaking existing non PI virtio-scsi logic..?

> 
> > - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> > - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
> >   iovecs.
> > - Ensure virtio_scsi_cmd_req_pi is naturally aligned
> 
> It turns out other virtio scsi commands aren't aligned?

Correct, virtio_scsi_cmd_req is currently 51 bytes.

> If true we don't need to make an exception here.

<nod>

> 
> > - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> > 
> > Is there anything else that you'd like to see for an initial merge, or
> > other issues that need to be addressed with the above..?
> > 
> > --nab
> 
> FYI Paolo sent a spec patch for the feature.
> Have you seen it?
> 

Yep, looks fine.

--nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Michael S. Tsirkin]

On Mon, May 19, 2014 at 01:54:50PM -0700, Nicholas A. Bellinger wrote:
> On Mon, 2014-05-19 at 22:15 +0300, Michael S. Tsirkin wrote:
> > On Mon, May 19, 2014 at 12:07:03PM -0700, Nicholas A. Bellinger wrote:
> > > On Wed, 2014-05-07 at 12:13 +0300, Michael S. Tsirkin wrote:
> > > > 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@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@redhat.com>
> > > > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > > > Cc: Martin K. Petersen <martin.petersen@oracle.com>
> > > > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > > > > Cc: Sagi Grimberg <sagig@dev.mellanox.co.il>
> > > > > > > Cc: H. Peter Anvin <hpa@zytor.com>
> > > > > > > Signed-off-by: Nicholas Bellinger <nab@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.
> > > > 
> > > 
> > > Hi MST,
> > > 
> > > So I've finally got some cycles to get back to this code, and wanted to
> > > verify the outstanding items you had previously raised:
> > > 
> > > - Convert vhost-scsi to be independent of IOV layout using 
> > >   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi 
> > >   operation..?)
> > 
> > Ideally yes.
> 
> Er, so changing vhost-scsi to be independent of IOV layout will have the
> side effect of breaking existing non PI virtio-scsi logic..?

Sorry I didn't make myself clear.
I merely think that all code should do memcpy_fromiovecend etc.
If done peoperly guests will not notice unless they test
VIRTIO_F_ANY_LAYOUT.



> > 
> > > - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> > > - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of 
> > >   iovecs.
> > > - Ensure virtio_scsi_cmd_req_pi is naturally aligned
> > 
> > It turns out other virtio scsi commands aren't aligned?
> 
> Correct, virtio_scsi_cmd_req is currently 51 bytes.
> 
> > If true we don't need to make an exception here.
> 
> <nod>
> 
> > 
> > > - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> > > 
> > > Is there anything else that you'd like to see for an initial merge, or
> > > other issues that need to be addressed with the above..?
> > > 
> > > --nab
> > 
> > FYI Paolo sent a spec patch for the feature.
> > Have you seen it?
> > 
> 
> Yep, looks fine.
> 
> --nab

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Paolo Bonzini]

Il 19/05/2014 21:07, Nicholas A. Bellinger ha scritto:
> Hi MST,
>
> So I've finally got some cycles to get back to this code, and wanted to
> verify the outstanding items you had previously raised:
>
> - Convert vhost-scsi to be independent of IOV layout using
>   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi
>   operation..?)
> - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of
>   iovecs.

This is the only item really required, since the bytes field is what 
will be in VIRTIO 1.0.  The other would be nice to have, but not a 
blocker for PI support.

> - Ensure virtio_scsi_cmd_req_pi is naturally aligned

mst already commented on this.

> - Figure out why QEMU is not acking (any) vhost-scsi feature bits

This is a separate bug, isn't it?  It need not block PI support.

Paolo

Re: [PATCH 6/6] virtio-scsi: Enable DIF/DIX modes in SCSI host LLD

[Nicholas A. Bellinger]

On Tue, 2014-05-20 at 10:35 +0200, Paolo Bonzini wrote:
> Il 19/05/2014 21:07, Nicholas A. Bellinger ha scritto:
> > Hi MST,
> >
> > So I've finally got some cycles to get back to this code, and wanted to
> > verify the outstanding items you had previously raised:
> >
> > - Convert vhost-scsi to be independent of IOV layout using
> >   memcpy_fromiovecend. (Does this effect existing non PI virtio-scsi
> >   operation..?)
> > - Report VIRTIO_F_ANY_LAYOUT feature bit to userspace.
> > - Convert virtio_scsi_cmd_req_pi to bytes field instead of number of
> >   iovecs.
> 
> This is the only item really required, since the bytes field is what 
> will be in VIRTIO 1.0.  The other would be nice to have, but not a 
> blocker for PI support.
> 
> > - Ensure virtio_scsi_cmd_req_pi is naturally aligned
> 
> mst already commented on this.
> 
> > - Figure out why QEMU is not acking (any) vhost-scsi feature bits
> 
> This is a separate bug, isn't it?  It need not block PI support.
> 

Thanks for the feedback.  I'll get the series updated to use bytes
instead of number of iovecs in virtio_scsi_cmd_req_pi, along with a
simple memcpy_fromiovecend conversion.

--nab