linux-kernel - Re: [RFC 0/6] vhost/scsi: Add T10 PI SGL passthrough support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 24 Feb 2014 22:05:11 -0800
From:	"Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To:	Paolo Bonzini <pbonzini@...hat.com>
Cc:	"Nicholas A. Bellinger" <nab@...erainc.com>,
	target-devel <target-devel@...r.kernel.org>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	kvm-devel <kvm@...r.kernel.org>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	Christoph Hellwig <hch@....de>, Hannes Reinecke <hare@...e.de>,
	Sagi Grimberg <sagig@...lanox.com>
Subject: Re: [RFC 0/6] vhost/scsi: Add T10 PI SGL passthrough support

On Mon, 2014-02-24 at 11:23 +0100, Paolo Bonzini wrote:
> Il 24/02/2014 06:32, Nicholas A. Bellinger ha scritto:
> > AFAICT up until this point the ->prio field has been unused, but
> > I'm certainly open to better ways of signaling (to vhost) that some
> > number of metadata iovs are to be expected..  Any thoughts..?
> 
> Hi nab,
> 
> the virtio-scsi side of the patch is nice and readable.  As requested, 
> here are my thoughts on how to add it to the standard.
> 
> The ->prio field is there to mimic SAM's command priority field (8.7 in 
> my copy of the standard).  I'd rather leave it alone; I understand this 
> is the main reason why this patch is RFC.

Yes.  ;)

> 
> Since we have a new feature bit, we can add a new element before the 
> cdb.  It could be a count of scatter/gather list like you did here, or 
> it could be a byte count.  Even better, we can add _two_ new fields, one 
> for protection data out and one for protection data in.
> 

Having two 16-bit fields for data out / data in protection count in the
command header should be fine.

So that said, adding a new virtio_scsi_cmd_req_pi definition per your
recommendation, and will update the series to use this when the
VIRTIO_SCSI_F_T10_PI feature bit has been negotiated on both ends.

> Also, do we need an equivalent of the residual field, but for metadata?
> 

Mmm, at least for PI I don't think a residual field is necessary.

Any time the metadata is not fully read on outgoing WRITEs, or written
on incoming READs the next hop performing a VERIFY operation will end up
failing with a GUARD or REFERENCE TAG failure.

MKP..?

> Finally, any reason why you put the data sg elements before the metadata 
> sg elements?

Nope, no particular reason for this.

>  I would have thought that processing is a bit simpler if 
> either the metadata comes first, or you store in the command header the 
> data count (either sg or byte).  Because the virtio buffers form a 
> linked list, it's a bit backwards to put metadata last, and store 
> metadata count in the command header; it prevents you from processing 
> the buffers online because you don't know when the metadata starts. 
> Even though the Linux virtio layer always gives you a buffer count, this 
> need not be the case in general.
> 

No objection here.  Updating the patch series to place protection
information ahead of the actual data payload.

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/