vhost-scsi support for ANY_LAYOUT

vhost-scsi support for ANY_LAYOUT

[Nicholas A. Bellinger]

Hi MST & Paolo,

So I'm currently working on vhost-scsi support for ANY_LAYOUT, and
wanted to verify some assumptions based upon your earlier emails..

*) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
virtio-scsi request + response headers will (always..?) be within a
single iovec.

*) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
virtio-scsi request + response headers may be (but not always..?)
combined with data-out + data-in payloads into a single iovec.

*) When ANY_LAYOUT + T10_PI is negotiated by vhost-scsi, it's expected
that PI and data payloads for data-out + data-in may be (but not
always..?) within the same iovec.  Consequently, both headers + PI +
data-payloads may also be within a single iovec.

*) Is it still safe to use 'out' + 'in' values from vhost_get_vq_desc()
in order to determine the data_direction...?  If not, what's the
preferred way of determining this information for get_user_pages_fast()
permission bits and target_submit_cmd_map_sgls()..?

Also, what is required on the QEMU side in order to start generating
ANY_LAYOUT style iovecs to verify the WIP changes..?  I see
hw/scsi/virtio-scsi.c has been converted to accept any_layout=1, but
AFAICT the changes where only related to code not shared between
hw/scsi/vhost-scsi.c.

Thank you,

--nab

Re: vhost-scsi support for ANY_LAYOUT

[Paolo Bonzini]

On 27/01/2015 07:35, Nicholas A. Bellinger wrote:
> Hi MST & Paolo,
> 
> So I'm currently working on vhost-scsi support for ANY_LAYOUT, and
> wanted to verify some assumptions based upon your earlier emails..
> 
> *) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
> virtio-scsi request + response headers will (always..?) be within a
> single iovec.
> 
> *) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
> virtio-scsi request + response headers may be (but not always..?)
> combined with data-out + data-in payloads into a single iovec.
> 
> *) When ANY_LAYOUT + T10_PI is negotiated by vhost-scsi, it's expected
> that PI and data payloads for data-out + data-in may be (but not
> always..?) within the same iovec.  Consequently, both headers + PI +
> data-payloads may also be within a single iovec.

s/expected/possible/g

Any split between header and payload is possible.  It's also possible to
split the header in multiple parts, e.g. put the CDB or sense buffer in
a separate iovec.  Even a single field could be split across two iovecs.

> *) Is it still safe to use 'out' + 'in' values from vhost_get_vq_desc()
> in order to determine the data_direction...?  If not, what's the
> preferred way of determining this information for get_user_pages_fast()
> permission bits and target_submit_cmd_map_sgls()..?

No, it's not safe.  What QEMU does is to check if the output buffers are
collectively larger than the request header, and if the input buffers
are collectively larger than the response header.  This lets you cpmpute
the data direction.

> Also, what is required on the QEMU side in order to start generating
> ANY_LAYOUT style iovecs to verify the WIP changes..? 

Nothing on the QEMU side.  You could test any-layout payloads by
changing the kernel side.  Splitting the sense buffer into its own iovec
for example is a trivial patch to drivers/scsi/virtio_scsi.c.

It also helps to adhere to coding conventions.  For example in QEMU I'm
never using iov[N], and I'm restricting usage of iovcnt to (iov, iovcnt)
parameter pairs.  This was suggested by Michael and found a couple bugs
indeed.

Paolo

Re: vhost-scsi support for ANY_LAYOUT

[Paolo Bonzini]

On 27/01/2015 09:42, Paolo Bonzini wrote:
> > *) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
> > virtio-scsi request + response headers may be (but not always..?)
> > combined with data-out + data-in payloads into a single iovec.

Note that request and data-out can be combined, and response + data-in
can be combined, but there still have to be separate iovecs for at least
outgoing and incoming buffers (so 2 buffers for the control and request
queues; 1 buffer for the event queue since it's unidirectional.

Paolo

Re: vhost-scsi support for ANY_LAYOUT

[Michael S. Tsirkin]

On Tue, Jan 27, 2015 at 09:42:22AM +0100, Paolo Bonzini wrote:
> 
> 
> On 27/01/2015 07:35, Nicholas A. Bellinger wrote:
> > Hi MST & Paolo,
> > 
> > So I'm currently working on vhost-scsi support for ANY_LAYOUT, and
> > wanted to verify some assumptions based upon your earlier emails..
> > 
> > *) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
> > virtio-scsi request + response headers will (always..?) be within a
> > single iovec.
> > 
> > *) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
> > virtio-scsi request + response headers may be (but not always..?)
> > combined with data-out + data-in payloads into a single iovec.
> > 
> > *) When ANY_LAYOUT + T10_PI is negotiated by vhost-scsi, it's expected
> > that PI and data payloads for data-out + data-in may be (but not
> > always..?) within the same iovec.  Consequently, both headers + PI +
> > data-payloads may also be within a single iovec.
> 
> s/expected/possible/g
> 
> Any split between header and payload is possible.  It's also possible to
> split the header in multiple parts, e.g. put the CDB or sense buffer in
> a separate iovec.  Even a single field could be split across two iovecs.
> 
> > *) Is it still safe to use 'out' + 'in' values from vhost_get_vq_desc()
> > in order to determine the data_direction...?  If not, what's the
> > preferred way of determining this information for get_user_pages_fast()
> > permission bits and target_submit_cmd_map_sgls()..?
> 
> No, it's not safe.  What QEMU does is to check if the output buffers are
> collectively larger than the request header, and if the input buffers
> are collectively larger than the response header.  This lets you cpmpute
> the data direction.

Generally true, but I'd like to clarify.

I think what we can say is that if there's in value,
host is expected to write data, and if there's out value,
to read data.

But this includes the headers so generally you need to get total length
for in and/or out (separately) and subtract header length.
But if you know there's e.g. no in header, then you can use in value
to figure out gup flags.
Or if you see there's no in data, you know you can use gup with read
only flags.


> > Also, what is required on the QEMU side in order to start generating
> > ANY_LAYOUT style iovecs to verify the WIP changes..? 
> 
> Nothing on the QEMU side.  You could test any-layout payloads by
> changing the kernel side.  Splitting the sense buffer into its own iovec
> for example is a trivial patch to drivers/scsi/virtio_scsi.c.
> 
> It also helps to adhere to coding conventions.  For example in QEMU I'm
> never using iov[N], and I'm restricting usage of iovcnt to (iov, iovcnt)
> parameter pairs.  This was suggested by Michael and found a couple bugs
> indeed.
> 
> Paolo

Re: vhost-scsi support for ANY_LAYOUT

[Michael S. Tsirkin]

On Mon, Jan 26, 2015 at 10:35:17PM -0800, Nicholas A. Bellinger wrote:
> Hi MST & Paolo,
> 
> So I'm currently working on vhost-scsi support for ANY_LAYOUT, and
> wanted to verify some assumptions based upon your earlier emails..
> 
> *) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
> virtio-scsi request + response headers will (always..?) be within a
> single iovec.

Do you mean a single struct iovec - as opposed to an array?
If so this is not a valid assumption I think, it can
be split across entries as well.
For example:
	struct virtio_scsi_cmd_req {
		__u8 lun[8];            /* Logical Unit Number */
		__virtio64 tag;         /* Command identifier */
		__u8 task_attr;         /* Task attribute */
		__u8 prio;              /* SAM command priority field */
		__u8 crn;
		__u8 cdb[VIRTIO_SCSI_CDB_SIZE];
	} __attribute__((packed));

We get struct iovec *iov and len 4, then
lun[0-3] might be in iov[0], lun[4-7], tag, task_attr, prio might be in
iov[1], crn, cdb and data-out might be in iov[2].
data-in must be separate in iov[3].

But if it makes sense, you can optimize for current
guest behaviour.



> *) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
> virtio-scsi request + response headers may be (but not always..?)
> combined with data-out + data-in payloads into a single iovec.

No. From virtio POV, you have to separate out (written by guest,
read by host) and in (written by host, read by guest).
Including both header and data.

The rule is that
1. out comes before in
2. out and in entries are separate


> *) When ANY_LAYOUT + T10_PI is negotiated by vhost-scsi, it's expected
> that PI and data payloads for data-out + data-in may be (but not
> always..?) within the same iovec.  Consequently, both headers + PI +
> data-payloads may also be within a single iovec.
> 
> *) Is it still safe to use 'out' + 'in' values from vhost_get_vq_desc()
> in order to determine the data_direction...?  If not, what's the
> preferred way of determining this information for get_user_pages_fast()
> permission bits and target_submit_cmd_map_sgls()..?
> 
> Also, what is required on the QEMU side in order to start generating
> ANY_LAYOUT style iovecs to verify the WIP changes..?  I see
> hw/scsi/virtio-scsi.c has been converted to accept any_layout=1, but
> AFAICT the changes where only related to code not shared between
> hw/scsi/vhost-scsi.c.

QEMU needs to write-list this feature bit.

> Thank you,
> 
> --nab

Re: vhost-scsi support for ANY_LAYOUT

[Michael S. Tsirkin]

On Mon, Jan 26, 2015 at 10:35:17PM -0800, Nicholas A. Bellinger wrote:
> Hi MST & Paolo,
> 
> So I'm currently working on vhost-scsi support for ANY_LAYOUT, and
> wanted to verify some assumptions based upon your earlier emails..
> 
> *) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
> virtio-scsi request + response headers will (always..?) be within a
> single iovec.

Do you mean a single struct iovec - as opposed to an array?
If so this is not a valid assumption I think, it can
be split across entries as well.
For example:
	struct virtio_scsi_cmd_req {
		__u8 lun[8];            /* Logical Unit Number */
		__virtio64 tag;         /* Command identifier */
		__u8 task_attr;         /* Task attribute */
		__u8 prio;              /* SAM command priority field */
		__u8 crn;
		__u8 cdb[VIRTIO_SCSI_CDB_SIZE];
	} __attribute__((packed));

We get struct iovec *iov and len 4, then
lun[0-3] might be in iov[0], lun[4-7], tag, task_attr, prio might be in
iov[1], crn, cdb and data-out might be in iov[2].
data-in must be separate in iov[3].

But if it makes sense, you can optimize for current
guest behaviour.



> *) When ANY_LAYOUT is negotiated by vhost-scsi, it's expected that
> virtio-scsi request + response headers may be (but not always..?)
> combined with data-out + data-in payloads into a single iovec.

No. From virtio POV, you have to separate out (written by guest,
read by host) and in (written by host, read by guest).
Including both header and data.

The rule is that
1. out comes before in
2. out and in entries are separate


> *) When ANY_LAYOUT + T10_PI is negotiated by vhost-scsi, it's expected
> that PI and data payloads for data-out + data-in may be (but not
> always..?) within the same iovec.  Consequently, both headers + PI +
> data-payloads may also be within a single iovec.
> 
> *) Is it still safe to use 'out' + 'in' values from vhost_get_vq_desc()
> in order to determine the data_direction...?  If not, what's the
> preferred way of determining this information for get_user_pages_fast()
> permission bits and target_submit_cmd_map_sgls()..?
> 
> Also, what is required on the QEMU side in order to start generating
> ANY_LAYOUT style iovecs to verify the WIP changes..?  I see
> hw/scsi/virtio-scsi.c has been converted to accept any_layout=1, but
> AFAICT the changes where only related to code not shared between
> hw/scsi/vhost-scsi.c.

QEMU needs to write-list this feature bit.

> Thank you,
> 
> --nab