Linux Virtualization - Jun 2013 - [PATCH] virtio-spec: add field for scsi command size

"Michael S. Tsirkin" <mst at redhat.com>
writes:
> On Thu, Jun 13, 2013 at 11:10:47AM +0300, Michael S. Tsirkin wrote:
>> On Thu, Jun 13, 2013 at 11:02:59AM +0300, Michael S. Tsirkin wrote:
>> > On Thu, Jun 13, 2013 at 02:12:22PM +0930, Rusty Russell wrote:
>> > > "Michael S. Tsirkin" <mst at redhat.com>
writes:
>> > > > On Thu, Mar 14, 2013 at 04:15:28PM +0100, Paolo Bonzini
wrote:
>> > > >> Il 14/03/2013 12:10, Michael S. Tsirkin ha scritto:
>> > > >> > Add field for guest to specify command size for
virtio-blk.
>> > > >> > 
>> > > >> > Signed-off-by: Michael S. Tsirkin <mst at
redhat.com>
>> > > >
>> > > > OK, but Rusty usually tweaks wording anyway.
>> > > > Rusty want to apply and make changes Paolo suggested
yourself
>> > > > or want me to?
>> > > 
>> > > (MST drew my attention back to this)
>> > > 
>> > > Please do.  And please add a note about this feature: that
without it,
>> > > the descriptor layout must be on the right boundaries for
historical
>> > > reasons.
>> > 
>> > It's there already, isn't it:
>> > 
>> >  If VIRTIO_BLK_F_SCSI_CMD_SIZE is not negotiated,
>> >  This field must reside in a single, separate read-only buffer; 
>> >  the command length
>> >  can be derived from the length of this buffer.
>> > 
>> 
>> Wait, I think I got it: you actually want to rename this
>> VIRTIO_BLK_F_ANY_SG and have it affect all requests?
>
> Sorry about being dense - could you please clarify?
OK, here's the new section I've written (but not committed!) on Message
Framing:

  Message Framing
 
The original intent of the specification was that message framing 
(the particular layout of descriptors) be independent of the 
contents of the buffers. For example, a network transmit buffer 
consists of a 12 byte header followed by the network packet. This 
could be most simply placed in the descriptor table as a 12 byte 
output descriptor followed by a 1514 byte output descriptor, but 
it could also consist of a single 1526 byte output descriptor in 
the case where the header and packet are adjacent, or even three 
or more descriptors (possibly with loss of efficiency in that 
case).

Regrettably, initial driver implementations used simple layouts, 
and devices came to rely on it, despite this specification 
wording. It is thus recommended that drivers be conservative in 
their assumptions, unless specific device features indicate that 
general layout is permitted, such VIRTIO_NET_F_ANY_LAYOUT or 
VIRTIO_BLK_F_ANY_LAYOUT. In addition, some implementations may 
have large-but-reasonable restrictions on total descriptor size 
(such as based on IOV_MAX in the host OS). This has not been a 
problem in practice: little sympathy will be given to drivers 
which create unreasonably-sized descriptors such as by dividing a 
network packet into 1500 single-byte descriptors!
==
Notes:
1) The restrictions are still in place by default.
2) We introduce VIRTIO_NET_F_ANY_LAYOUT and VIRTIO_BLK_F_ANY_LAYOUT
   specifically for net and block (note the new names).
3) I note the special case of stupid descriptors.
4) Enjoy our humble pie, don't hide it in a footnote!

If that seems OK, I'll modify the net and block specs appropriately.

Cheers,
Rusty.

On Wed, Jun 19, 2013 at 02:16:06PM +0930, Rusty Russell
wrote:
> "Michael S. Tsirkin" <mst at redhat.com> writes:
> > On Thu, Jun 13, 2013 at 11:10:47AM +0300, Michael S. Tsirkin wrote:
> >> On Thu, Jun 13, 2013 at 11:02:59AM +0300, Michael S. Tsirkin
wrote:
> >> > On Thu, Jun 13, 2013 at 02:12:22PM +0930, Rusty Russell
wrote:
> >> > > "Michael S. Tsirkin" <mst at redhat.com>
writes:
> >> > > > On Thu, Mar 14, 2013 at 04:15:28PM +0100, Paolo
Bonzini wrote:
> >> > > >> Il 14/03/2013 12:10, Michael S. Tsirkin ha
scritto:
> >> > > >> > Add field for guest to specify command
size for virtio-blk.
> >> > > >> > 
> >> > > >> > Signed-off-by: Michael S. Tsirkin <mst
at redhat.com>
> >> > > >
> >> > > > OK, but Rusty usually tweaks wording anyway.
> >> > > > Rusty want to apply and make changes Paolo
suggested yourself
> >> > > > or want me to?
> >> > > 
> >> > > (MST drew my attention back to this)
> >> > > 
> >> > > Please do.  And please add a note about this feature:
that without it,
> >> > > the descriptor layout must be on the right boundaries
for historical
> >> > > reasons.
> >> > 
> >> > It's there already, isn't it:
> >> > 
> >> >  If VIRTIO_BLK_F_SCSI_CMD_SIZE is not negotiated,
> >> >  This field must reside in a single, separate read-only
buffer;
> >> >  the command length
> >> >  can be derived from the length of this buffer.
> >> > 
> >> 
> >> Wait, I think I got it: you actually want to rename this
> >> VIRTIO_BLK_F_ANY_SG and have it affect all requests?
> >
> > Sorry about being dense - could you please clarify?
> 
> OK, here's the new section I've written (but not committed!) on
Message
> Framing:
> 
>   Message Framing
>  
> The original intent of the specification was that message framing 
> (the particular layout of descriptors) be independent of the 
> contents of the buffers. For example, a network transmit buffer 
> consists of a 12 byte header followed by the network packet. This 
> could be most simply placed in the descriptor table as a 12 byte 
> output descriptor followed by a 1514 byte output descriptor, but 
> it could also consist of a single 1526 byte output descriptor in 
> the case where the header and packet are adjacent, or even three 
> or more descriptors (possibly with loss of efficiency in that 
> case).
> 
> Regrettably, initial driver implementations used simple layouts, 
> and devices came to rely on it, despite this specification 
> wording. It is thus recommended that drivers be conservative in 
> their assumptions, unless specific device features indicate that 
> general layout is permitted, such VIRTIO_NET_F_ANY_LAYOUT or 
> VIRTIO_BLK_F_ANY_LAYOUT. In addition, some implementations may 
> have large-but-reasonable restrictions on total descriptor size 
> (such as based on IOV_MAX in the host OS). This has not been a 
> problem in practice: little sympathy will be given to drivers 
> which create unreasonably-sized descriptors such as by dividing a 
> network packet into 1500 single-byte descriptors!
> ==> 
> Notes:
> 1) The restrictions are still in place by default.
> 2) We introduce VIRTIO_NET_F_ANY_LAYOUT and VIRTIO_BLK_F_ANY_LAYOUT
>    specifically for net and block (note the new names).
> 3) I note the special case of stupid descriptors.
> 4) Enjoy our humble pie, don't hide it in a footnote!
> 
> If that seems OK, I'll modify the net and block specs appropriately.
> 
> Cheers,
> Rusty.
Acked-by: Michael S. Tsirkin <mst at redhat.com>

an additional suggestion: limit max s/g
to vq size (it's in theory possible to go higher with indirect
descriptors, but we need some limit, and queue size gives us
a nice way to say "this is a max descriptor size").

-- 
MST

Il 19/06/2013 10:24, Michael S. Tsirkin ha scritto:
>> > 2) We introduce VIRTIO_NET_F_ANY_LAYOUT and
VIRTIO_BLK_F_ANY_LAYOUT
>> >    specifically for net and block (note the new names).
So why not a transport feature?  Is it just because the SCSI commands
for virtio-blk also require a config space field?  Sorry if I missed
this upthread.

Paolo
>> > 3) I note the special case of stupid descriptors.
>> > 4) Enjoy our humble pie, don't hide it in a footnote!
>> > 
>> > If that seems OK, I'll modify the net and block specs
appropriately.
>> > 
>> > Cheers,
>> > Rusty.
> Acked-by: Michael S. Tsirkin <mst at redhat.com>
> 
> an additional suggestion: limit max s/g
> to vq size (it's in theory possible to go higher with indirect
> descriptors, but we need some limit, and queue size gives us
> a nice way to say "this is a max descriptor size").

"Michael S. Tsirkin" <mst at redhat.com>
writes:
> On Wed, Jun 19, 2013 at 02:16:06PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst at redhat.com> writes:
>> > On Thu, Jun 13, 2013 at 11:10:47AM +0300, Michael S. Tsirkin
wrote:
>> >> On Thu, Jun 13, 2013 at 11:02:59AM +0300, Michael S. Tsirkin
wrote:
>> >> > On Thu, Jun 13, 2013 at 02:12:22PM +0930, Rusty Russell
wrote:
>> >> > > "Michael S. Tsirkin" <mst at
redhat.com> writes:
>> >> > > > On Thu, Mar 14, 2013 at 04:15:28PM +0100, Paolo
Bonzini wrote:
>> >> > > >> Il 14/03/2013 12:10, Michael S. Tsirkin ha
scritto:
>> >> > > >> > Add field for guest to specify command
size for virtio-blk.
>> >> > > >> > 
>> >> > > >> > Signed-off-by: Michael S. Tsirkin
<mst at redhat.com>
>> >> > > >
>> >> > > > OK, but Rusty usually tweaks wording anyway.
>> >> > > > Rusty want to apply and make changes Paolo
suggested yourself
>> >> > > > or want me to?
>> >> > > 
>> >> > > (MST drew my attention back to this)
>> >> > > 
>> >> > > Please do.  And please add a note about this
feature: that without it,
>> >> > > the descriptor layout must be on the right
boundaries for historical
>> >> > > reasons.
>> >> > 
>> >> > It's there already, isn't it:
>> >> > 
>> >> >  If VIRTIO_BLK_F_SCSI_CMD_SIZE is not negotiated,
>> >> >  This field must reside in a single, separate read-only
buffer;
>> >> >  the command length
>> >> >  can be derived from the length of this buffer.
>> >> > 
>> >> 
>> >> Wait, I think I got it: you actually want to rename this
>> >> VIRTIO_BLK_F_ANY_SG and have it affect all requests?
>> >
>> > Sorry about being dense - could you please clarify?
>> 
>> OK, here's the new section I've written (but not committed!) on
Message
>> Framing:
>> 
>>   Message Framing
>>  
>> The original intent of the specification was that message framing 
>> (the particular layout of descriptors) be independent of the 
>> contents of the buffers. For example, a network transmit buffer 
>> consists of a 12 byte header followed by the network packet. This 
>> could be most simply placed in the descriptor table as a 12 byte 
>> output descriptor followed by a 1514 byte output descriptor, but 
>> it could also consist of a single 1526 byte output descriptor in 
>> the case where the header and packet are adjacent, or even three 
>> or more descriptors (possibly with loss of efficiency in that 
>> case).
>> 
>> Regrettably, initial driver implementations used simple layouts, 
>> and devices came to rely on it, despite this specification 
>> wording. It is thus recommended that drivers be conservative in 
>> their assumptions, unless specific device features indicate that 
>> general layout is permitted, such VIRTIO_NET_F_ANY_LAYOUT or 
>> VIRTIO_BLK_F_ANY_LAYOUT. In addition, some implementations may 
>> have large-but-reasonable restrictions on total descriptor size 
>> (such as based on IOV_MAX in the host OS). This has not been a 
>> problem in practice: little sympathy will be given to drivers 
>> which create unreasonably-sized descriptors such as by dividing a 
>> network packet into 1500 single-byte descriptors!
>> ==>> 
>> Notes:
>> 1) The restrictions are still in place by default.
>> 2) We introduce VIRTIO_NET_F_ANY_LAYOUT and VIRTIO_BLK_F_ANY_LAYOUT
>>    specifically for net and block (note the new names).
>> 3) I note the special case of stupid descriptors.
>> 4) Enjoy our humble pie, don't hide it in a footnote!
>> 
>> If that seems OK, I'll modify the net and block specs
appropriately.
>> 
>> Cheers,
>> Rusty.
>
> Acked-by: Michael S. Tsirkin <mst at redhat.com>
>
> an additional suggestion: limit max s/g
> to vq size (it's in theory possible to go higher with indirect
> descriptors, but we need some limit, and queue size gives us
> a nice way to say "this is a max descriptor size").
Indeed, that limit is currently respected by Linux.

I've added the following to 2.3.3:

	Note that the total number of chained direct descriptors must
        not exceed the Queue Size, thus it should always be possible to
        queue one buffer without use of indirect descriptors.

Cheers,
Rusty.