[PATCH] virtio-spec: add field for scsi command size

[PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

Add field for guest to specify command size for virtio-blk.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 virtio-spec.lyx | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 78 insertions(+), 5 deletions(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index a8ce3f9..fea97ed 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -5826,6 +5826,16 @@ VIRTIO_BLK_F_TOPOLOGY (10) Device exports information on optimal I/O alignment.
 \change_inserted 1531152142 1341302349
 VIRTIO_BLK_F_CONFIG_WCE (11) Device can toggle its cache between writeback
  and writethrough modes.
+\change_inserted 1986246365 1363257418
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 1986246365 1363258629
+VIRTIO_BLK_F_CMD_SIZE (12) cmd_size field is valid.
+\change_inserted 1531152142 1341302349
+
 \end_layout
 
 \end_deeper
@@ -5994,6 +6004,30 @@ struct virtio_blk_config {
 \change_inserted 1531152142 1341301918
 
     u8 writeback;
+\change_inserted 1986246365 1363257385
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1363258610
+
+    u8 reserved[3];
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1363259147
+
+    // buffer length for cmd field in requests.
+ reset value 0.
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1363257810
+
+    u32 cmd_size;
 \change_unchanged
 
 \end_layout
@@ -6085,6 +6119,21 @@ Until version 1.1, QEMU remained in writeback mode even after a guest announced
 \end_inset
 
 .
+\change_inserted 1986246365 1363258989
+
+\end_layout
+
+\begin_layout Enumerate
+
+\change_inserted 1986246365 1363259237
+If VIRTIO_BLK_F_SCSI_CMD_SIZE is negotiated, Guest should set the desired
+ size for scsi commands to the 
+\emph on
+cmd_size 
+\emph default
+field.
+\change_unchanged
+
 \end_layout
 
 \begin_layout Section*
@@ -6369,8 +6418,33 @@ cmd
 \emph default
  field is only present for scsi packet command requests, and indicates the
  command to perform.
- This field must reside in a single, separate read-only buffer; command
- length can be derived from the length of this buffer.
+ 
+\change_inserted 1986246365 1363258881
+If VIRTIO_BLK_F_SCSI_CMD_SIZE feature bit is negotiated, Guest must set
+ the command length by writing it to the cmd_size field in configuration
+ space
+\begin_inset Foot
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 1986246365 1363258931
+Guest should always set cmd_size during initialization.
+\change_unchanged
+
+\end_layout
+
+\end_inset
+
+.
+ If VIRTIO_BLK_F_SCSI_CMD_SIZE is not negotiated, 
+\change_deleted 1986246365 1363258703
+T
+\change_inserted 1986246365 1363258703
+t
+\change_unchanged
+his field must reside in a single, separate read-only buffer; command length
+ can be derived from the length of this buffer.
  
 \end_layout
 
-- 
MST

Re: [PATCH] virtio-spec: add field for scsi command size

[Paolo Bonzini]

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@redhat.com>
> ---
>  virtio-spec.lyx | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 78 insertions(+), 5 deletions(-)
> 
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index a8ce3f9..fea97ed 100644
> --- a/virtio-spec.lyx
> +++ b/virtio-spec.lyx
> @@ -5826,6 +5826,16 @@ VIRTIO_BLK_F_TOPOLOGY (10) Device exports information on optimal I/O alignment.
>  \change_inserted 1531152142 1341302349
>  VIRTIO_BLK_F_CONFIG_WCE (11) Device can toggle its cache between writeback
>   and writethrough modes.
> +\change_inserted 1986246365 1363257418
> +
> +\end_layout
> +
> +\begin_layout Description
> +
> +\change_inserted 1986246365 1363258629
> +VIRTIO_BLK_F_CMD_SIZE (12) cmd_size field is valid.

SCSI_CMD_SIZE and scsi_cmd_size please.

> +\change_inserted 1531152142 1341302349
> +
>  \end_layout
>  
>  \end_deeper
> @@ -5994,6 +6004,30 @@ struct virtio_blk_config {
>  \change_inserted 1531152142 1341301918
>  
>      u8 writeback;
> +\change_inserted 1986246365 1363257385
> +
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1363258610
> +
> +    u8 reserved[3];
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1363259147
> +
> +    // buffer length for cmd field in requests.

size of the cmd field in VIRTIO_BLK_T_SCSI_CMD requests.

> + reset value 0.
> +\end_layout
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1363257810
> +
> +    u32 cmd_size;

scsi_cmd_size

>  \change_unchanged
>  
>  \end_layout
> @@ -6085,6 +6119,21 @@ Until version 1.1, QEMU remained in writeback mode even after a guest announced
>  \end_inset
>  
>  .
> +\change_inserted 1986246365 1363258989
> +
> +\end_layout
> +
> +\begin_layout Enumerate
> +
> +\change_inserted 1986246365 1363259237
> +If VIRTIO_BLK_F_SCSI_CMD_SIZE is negotiated, Guest should set the desired
> + size for scsi commands to the 

If both VIRTIO_BLK_F_SCSI and VIRTIO_BLK_F_SCSI_CMD_SIZE are negotiated,
the driver should write a non-zero value to the scsi_cmd_size field
before sending a VIRTIO_BLK_T_SCSI_CMD request.

> +\emph on
> +cmd_size 
> +\emph default
> +field.
> +\change_unchanged
> +
>  \end_layout
>  
>  \begin_layout Section*
> @@ -6369,8 +6418,33 @@ cmd
>  \emph default
>   field is only present for scsi packet command requests, and indicates the
>   command to perform.
> - This field must reside in a single, separate read-only buffer; command
> - length can be derived from the length of this buffer.
> + 
> +\change_inserted 1986246365 1363258881
> +If VIRTIO_BLK_F_SCSI_CMD_SIZE feature bit is negotiated, Guest must set
> + the command length by writing it to the cmd_size field in configuration
> + space

If the VIRTIO_BLK_F_SCSI_CMD_SIZE feature bit is negotiated, the driver
should write the length of the cmd field to the scsi_cmd_size field of
the configuration space.

(please make sure bold and emphasis are consistent)

> +\begin_inset Foot
> +status open
> +
> +\begin_layout Plain Layout
> +
> +\change_inserted 1986246365 1363258931
> +Guest should always set cmd_size during initialization.

scsi_cmd_size

> +\change_unchanged
> +
> +\end_layout
> +
> +\end_inset
> +
> +.
> + If VIRTIO_BLK_F_SCSI_CMD_SIZE is not negotiated, 
> +\change_deleted 1986246365 1363258703
> +T
> +\change_inserted 1986246365 1363258703
> +t
> +\change_unchanged
> +his field must reside in a single, separate read-only buffer; command length
> + can be derived from the length of this buffer.

the command length...

>   
>  \end_layout
>  
> 

Thanks,

Paolo

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

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@redhat.com>

OK, but Rusty usually tweaks wording anyway.
Rusty want to apply and make changes Paolo suggested yourself
or want me to?

> > ---
> >  virtio-spec.lyx | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 78 insertions(+), 5 deletions(-)
> > 
> > diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> > index a8ce3f9..fea97ed 100644
> > --- a/virtio-spec.lyx
> > +++ b/virtio-spec.lyx
> > @@ -5826,6 +5826,16 @@ VIRTIO_BLK_F_TOPOLOGY (10) Device exports information on optimal I/O alignment.
> >  \change_inserted 1531152142 1341302349
> >  VIRTIO_BLK_F_CONFIG_WCE (11) Device can toggle its cache between writeback
> >   and writethrough modes.
> > +\change_inserted 1986246365 1363257418
> > +
> > +\end_layout
> > +
> > +\begin_layout Description
> > +
> > +\change_inserted 1986246365 1363258629
> > +VIRTIO_BLK_F_CMD_SIZE (12) cmd_size field is valid.
> 
> SCSI_CMD_SIZE and scsi_cmd_size please.
> 
> > +\change_inserted 1531152142 1341302349
> > +
> >  \end_layout
> >  
> >  \end_deeper
> > @@ -5994,6 +6004,30 @@ struct virtio_blk_config {
> >  \change_inserted 1531152142 1341301918
> >  
> >      u8 writeback;
> > +\change_inserted 1986246365 1363257385
> > +
> > +\end_layout
> > +
> > +\begin_layout Plain Layout
> > +
> > +\change_inserted 1986246365 1363258610
> > +
> > +    u8 reserved[3];
> > +\end_layout
> > +
> > +\begin_layout Plain Layout
> > +
> > +\change_inserted 1986246365 1363259147
> > +
> > +    // buffer length for cmd field in requests.
> 
> size of the cmd field in VIRTIO_BLK_T_SCSI_CMD requests.
> 
> > + reset value 0.
> > +\end_layout
> > +
> > +\begin_layout Plain Layout
> > +
> > +\change_inserted 1986246365 1363257810
> > +
> > +    u32 cmd_size;
> 
> scsi_cmd_size
> 
> >  \change_unchanged
> >  
> >  \end_layout
> > @@ -6085,6 +6119,21 @@ Until version 1.1, QEMU remained in writeback mode even after a guest announced
> >  \end_inset
> >  
> >  .
> > +\change_inserted 1986246365 1363258989
> > +
> > +\end_layout
> > +
> > +\begin_layout Enumerate
> > +
> > +\change_inserted 1986246365 1363259237
> > +If VIRTIO_BLK_F_SCSI_CMD_SIZE is negotiated, Guest should set the desired
> > + size for scsi commands to the 
> 
> If both VIRTIO_BLK_F_SCSI and VIRTIO_BLK_F_SCSI_CMD_SIZE are negotiated,
> the driver should write a non-zero value to the scsi_cmd_size field
> before sending a VIRTIO_BLK_T_SCSI_CMD request.
> 
> > +\emph on
> > +cmd_size 
> > +\emph default
> > +field.
> > +\change_unchanged
> > +
> >  \end_layout
> >  
> >  \begin_layout Section*
> > @@ -6369,8 +6418,33 @@ cmd
> >  \emph default
> >   field is only present for scsi packet command requests, and indicates the
> >   command to perform.
> > - This field must reside in a single, separate read-only buffer; command
> > - length can be derived from the length of this buffer.
> > + 
> > +\change_inserted 1986246365 1363258881
> > +If VIRTIO_BLK_F_SCSI_CMD_SIZE feature bit is negotiated, Guest must set
> > + the command length by writing it to the cmd_size field in configuration
> > + space
> 
> If the VIRTIO_BLK_F_SCSI_CMD_SIZE feature bit is negotiated, the driver
> should write the length of the cmd field to the scsi_cmd_size field of
> the configuration space.
> 
> (please make sure bold and emphasis are consistent)
> 
> > +\begin_inset Foot
> > +status open
> > +
> > +\begin_layout Plain Layout
> > +
> > +\change_inserted 1986246365 1363258931
> > +Guest should always set cmd_size during initialization.
> 
> scsi_cmd_size
> 
> > +\change_unchanged
> > +
> > +\end_layout
> > +
> > +\end_inset
> > +
> > +.
> > + If VIRTIO_BLK_F_SCSI_CMD_SIZE is not negotiated, 
> > +\change_deleted 1986246365 1363258703
> > +T
> > +\change_inserted 1986246365 1363258703
> > +t
> > +\change_unchanged
> > +his field must reside in a single, separate read-only buffer; command length
> > + can be derived from the length of this buffer.
> 
> the command length...
> 
> >   
> >  \end_layout
> >  
> > 
> 
> Thanks,
> 
> Paolo

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Thu, Mar 14, 2013 at 07:39:18PM +0200, Michael S. Tsirkin wrote:
> 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@redhat.com>
> 
> OK, but Rusty usually tweaks wording anyway.
> Rusty want to apply and make changes Paolo suggested yourself
> or want me to?

Ping. Rusty could you indicate your preference please?

> > > ---
> > >  virtio-spec.lyx | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 78 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> > > index a8ce3f9..fea97ed 100644
> > > --- a/virtio-spec.lyx
> > > +++ b/virtio-spec.lyx
> > > @@ -5826,6 +5826,16 @@ VIRTIO_BLK_F_TOPOLOGY (10) Device exports information on optimal I/O alignment.
> > >  \change_inserted 1531152142 1341302349
> > >  VIRTIO_BLK_F_CONFIG_WCE (11) Device can toggle its cache between writeback
> > >   and writethrough modes.
> > > +\change_inserted 1986246365 1363257418
> > > +
> > > +\end_layout
> > > +
> > > +\begin_layout Description
> > > +
> > > +\change_inserted 1986246365 1363258629
> > > +VIRTIO_BLK_F_CMD_SIZE (12) cmd_size field is valid.
> > 
> > SCSI_CMD_SIZE and scsi_cmd_size please.
> > 
> > > +\change_inserted 1531152142 1341302349
> > > +
> > >  \end_layout
> > >  
> > >  \end_deeper
> > > @@ -5994,6 +6004,30 @@ struct virtio_blk_config {
> > >  \change_inserted 1531152142 1341301918
> > >  
> > >      u8 writeback;
> > > +\change_inserted 1986246365 1363257385
> > > +
> > > +\end_layout
> > > +
> > > +\begin_layout Plain Layout
> > > +
> > > +\change_inserted 1986246365 1363258610
> > > +
> > > +    u8 reserved[3];
> > > +\end_layout
> > > +
> > > +\begin_layout Plain Layout
> > > +
> > > +\change_inserted 1986246365 1363259147
> > > +
> > > +    // buffer length for cmd field in requests.
> > 
> > size of the cmd field in VIRTIO_BLK_T_SCSI_CMD requests.
> > 
> > > + reset value 0.
> > > +\end_layout
> > > +
> > > +\begin_layout Plain Layout
> > > +
> > > +\change_inserted 1986246365 1363257810
> > > +
> > > +    u32 cmd_size;
> > 
> > scsi_cmd_size
> > 
> > >  \change_unchanged
> > >  
> > >  \end_layout
> > > @@ -6085,6 +6119,21 @@ Until version 1.1, QEMU remained in writeback mode even after a guest announced
> > >  \end_inset
> > >  
> > >  .
> > > +\change_inserted 1986246365 1363258989
> > > +
> > > +\end_layout
> > > +
> > > +\begin_layout Enumerate
> > > +
> > > +\change_inserted 1986246365 1363259237
> > > +If VIRTIO_BLK_F_SCSI_CMD_SIZE is negotiated, Guest should set the desired
> > > + size for scsi commands to the 
> > 
> > If both VIRTIO_BLK_F_SCSI and VIRTIO_BLK_F_SCSI_CMD_SIZE are negotiated,
> > the driver should write a non-zero value to the scsi_cmd_size field
> > before sending a VIRTIO_BLK_T_SCSI_CMD request.
> > 
> > > +\emph on
> > > +cmd_size 
> > > +\emph default
> > > +field.
> > > +\change_unchanged
> > > +
> > >  \end_layout
> > >  
> > >  \begin_layout Section*
> > > @@ -6369,8 +6418,33 @@ cmd
> > >  \emph default
> > >   field is only present for scsi packet command requests, and indicates the
> > >   command to perform.
> > > - This field must reside in a single, separate read-only buffer; command
> > > - length can be derived from the length of this buffer.
> > > + 
> > > +\change_inserted 1986246365 1363258881
> > > +If VIRTIO_BLK_F_SCSI_CMD_SIZE feature bit is negotiated, Guest must set
> > > + the command length by writing it to the cmd_size field in configuration
> > > + space
> > 
> > If the VIRTIO_BLK_F_SCSI_CMD_SIZE feature bit is negotiated, the driver
> > should write the length of the cmd field to the scsi_cmd_size field of
> > the configuration space.
> > 
> > (please make sure bold and emphasis are consistent)
> > 
> > > +\begin_inset Foot
> > > +status open
> > > +
> > > +\begin_layout Plain Layout
> > > +
> > > +\change_inserted 1986246365 1363258931
> > > +Guest should always set cmd_size during initialization.
> > 
> > scsi_cmd_size
> > 
> > > +\change_unchanged
> > > +
> > > +\end_layout
> > > +
> > > +\end_inset
> > > +
> > > +.
> > > + If VIRTIO_BLK_F_SCSI_CMD_SIZE is not negotiated, 
> > > +\change_deleted 1986246365 1363258703
> > > +T
> > > +\change_inserted 1986246365 1363258703
> > > +t
> > > +\change_unchanged
> > > +his field must reside in a single, separate read-only buffer; command length
> > > + can be derived from the length of this buffer.
> > 
> > the command length...
> > 
> > >   
> > >  \end_layout
> > >  
> > > 
> > 
> > Thanks,
> > 
> > Paolo

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Thu, Mar 14, 2013 at 01:10:20PM +0200, Michael S. Tsirkin wrote:
> Add field for guest to specify command size for virtio-blk.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

There's one concern here: are we going to add a
feature flag for flexible layout? If yes this feature
could thinkably just reuse it.

It allows a nice optimization in virtio net that is very easy
with latest qemu.

Rusty, what is our plan wrt this?  Are you fine with a per-device
feature bit?  Or do we need a transport feature bit (there's the small
issue that we almost run out of them for virtio-pci, do we use the last
one?).

As an aside, the new virtio pci config proposal seems to be stuck too,
are you looking into it or want me to?
-- 
MST

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Mar 14, 2013 at 01:10:20PM +0200, Michael S. Tsirkin wrote:
>> Add field for guest to specify command size for virtio-blk.
>> 
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> There's one concern here: are we going to add a
> feature flag for flexible layout? If yes this feature
> could thinkably just reuse it.
>
> It allows a nice optimization in virtio net that is very easy
> with latest qemu.
>
> Rusty, what is our plan wrt this?  Are you fine with a per-device
> feature bit?  Or do we need a transport feature bit (there's the small
> issue that we almost run out of them for virtio-pci, do we use the last
> one?).

I'd really like to tie it to the virtio PCI config work, but I need
cycles to complete that....

> As an aside, the new virtio pci config proposal seems to be stuck too,
> are you looking into it or want me to?

That would be good.  If I re-spin the kernel side, will you be able to
look at the QEMU parts?

I can weave this in between my vhost/vringh work and other commitments,
I hope.

Thanks,
Rusty.

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Tue, Mar 19, 2013 at 11:51:35AM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Thu, Mar 14, 2013 at 01:10:20PM +0200, Michael S. Tsirkin wrote:
> >> Add field for guest to specify command size for virtio-blk.
> >> 
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > There's one concern here: are we going to add a
> > feature flag for flexible layout? If yes this feature
> > could thinkably just reuse it.
> >
> > It allows a nice optimization in virtio net that is very easy
> > with latest qemu.
> >
> > Rusty, what is our plan wrt this?  Are you fine with a per-device
> > feature bit?  Or do we need a transport feature bit (there's the small
> > issue that we almost run out of them for virtio-pci, do we use the last
> > one?).
> 
> I'd really like to tie it to the virtio PCI config work, but I need
> cycles to complete that....
> 
> > As an aside, the new virtio pci config proposal seems to be stuck too,
> > are you looking into it or want me to?
> 
> That would be good.  If I re-spin the kernel side, will you be able to
> look at the QEMU parts?
> 
> I can weave this in between my vhost/vringh work and other commitments,
> I hope.
> 
> Thanks,
> Rusty.

Let's start with the spec, OK?

I think there's one other issue I'd like to solve and that's
the need for IO BAR for kvm.  PCI folks are really trying to
move away from IO BARs. Pls take a look at the mail titled
	virtio PCI on KVM without IO BARs
the agreement on that thread was that virtio pci
devices will have a new capability which gives
the address/data pair to use for notifications.
For maximum flexibility, we can make it look kind of
like the MSIX table, and put it in the memory BAR,
with address/data pairs for each vq.

ISR is legacy so maybe it's okay to have it in the
memory BAR.

Making sense? Should I send a more formal proposal?

-- 
MST

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

"Michael S. Tsirkin" <mst@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@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.

Thanks,
Rusty.

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Thu, Jun 13, 2013 at 02:12:22PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@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@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.
> 
> Thanks,
> Rusty.

Since I got your attention, how about
	virtio-spec: balloon: MUST_TELL_HOST is optional
?

We are still discussing more ideas for balloon but this one
seems uncontroversial since this just codifies what guests
do.

-- 
MST

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Thu, Jun 13, 2013 at 02:12:22PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@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@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.




> Thanks,
> Rusty.

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

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

> 
> 
> > Thanks,
> > Rusty.

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

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

> > 
> > 
> > > Thanks,
> > > Rusty.

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

"Michael S. Tsirkin" <mst@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@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@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.

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Wed, Jun 19, 2013 at 02:16:06PM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@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@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@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@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

Re: [PATCH] virtio-spec: add field for scsi command size

[Paolo Bonzini]

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@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").

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Wed, Jun 19, 2013 at 10:28:50AM +0200, Paolo Bonzini wrote:
> 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

One of the reasons is we've run out of transport features :)

> >> > 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@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").

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

Paolo Bonzini <pbonzini@redhat.com> writes:
> 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.

Mainly because I'm not sure that *all* devices are now safe.  Are they?

I had a stress test half-written for this, pasted below.

Otherwise I'd be happy to do both: use feature 25 for
VIRTIO_F_ANY_LAYOUT and another feature bit for the virtio-blk layout
change.

Cheers,
Rusty.

virtio: CONFIG_VIRTIO_DEVICE_TORTURE

Virtio devices are not supposed to depend on the framing of the scatter-gather
lists, but various implementations did.  Safeguard this in future by adding
an option to deliberately create perverse descriptors.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 8d5bddb..99c0187 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -5,6 +5,22 @@ config VIRTIO
 	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST,
 	  CONFIG_RPMSG or CONFIG_S390_GUEST.
 
+config VIRTIO_TORTURE
+	bool "Virtio torture debugging"
+	depends on VIRTIO && DEBUG_KERNEL
+	help
+
+	  This makes the virtio_ring implementation stress-test
+	  devices.  In particularly, creatively change the format of
+	  requests to make sure that devices are properly implemented,
+	  as well as implement various checks to ensure drivers are
+	  correct.  This will make your virtual machine slow *and*
+	  unreliable!  Say N.
+
+	  Put virtio_ring.device_torture to your boot commandline to
+	  torture devices (otherwise only simply sanity checks are
+	  done).
+
 menu "Virtio drivers"
 
 config VIRTIO_PCI
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index e82821a..6e5271c 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -45,35 +45,6 @@
 #define virtio_wmb(vq) wmb()
 #endif
 
-#ifdef DEBUG
-/* For development, we want to crash whenever the ring is screwed. */
-#define BAD_RING(_vq, fmt, args...)				\
-	do {							\
-		dev_err(&(_vq)->vq.vdev->dev,			\
-			"%s:"fmt, (_vq)->vq.name, ##args);	\
-		BUG();						\
-	} while (0)
-/* Caller is supposed to guarantee no reentry. */
-#define START_USE(_vq)						\
-	do {							\
-		if ((_vq)->in_use)				\
-			panic("%s:in_use = %i\n",		\
-			      (_vq)->vq.name, (_vq)->in_use);	\
-		(_vq)->in_use = __LINE__;			\
-	} while (0)
-#define END_USE(_vq) \
-	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0)
-#else
-#define BAD_RING(_vq, fmt, args...)				\
-	do {							\
-		dev_err(&_vq->vq.vdev->dev,			\
-			"%s:"fmt, (_vq)->vq.name, ##args);	\
-		(_vq)->broken = true;				\
-	} while (0)
-#define START_USE(vq)
-#define END_USE(vq)
-#endif
-
 struct indirect_cache {
 	unsigned int max;
 	struct vring_desc *cache;
@@ -109,7 +80,7 @@ struct vring_virtqueue
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);
 
-#ifdef DEBUG
+#ifdef CONFIG_VIRTIO_TORTURE
 	/* They're supposed to lock for us. */
 	unsigned int in_use;
 
@@ -134,6 +105,200 @@ static inline struct indirect_cache *indirect_cache(struct vring_virtqueue *vq)
 	return (struct indirect_cache *)&vq->data[vq->vring.num];
 }
 
+#ifdef CONFIG_VIRTIO_TORTURE
+/* For development, we want to crash whenever the ring is screwed. */
+#define BAD_RING(_vq, fmt, args...)				\
+	do {							\
+		dev_err(&(_vq)->vq.vdev->dev,			\
+			"%s:"fmt, (_vq)->vq.name, ##args);	\
+		BUG();						\
+	} while (0)
+/* Caller is supposed to guarantee no reentry. */
+#define START_USE(_vq)						\
+	do {							\
+		if ((_vq)->in_use)				\
+			panic("%s:in_use = %i\n",		\
+			      (_vq)->vq.name, (_vq)->in_use);	\
+		(_vq)->in_use = __LINE__;			\
+	} while (0)
+#define END_USE(_vq) \
+	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0)
+
+static bool device_torture;
+module_param(device_torture, bool, 0644);
+
+struct torture {
+	unsigned int orig_out, orig_in;
+	void *orig_data;
+	struct scatterlist sg[4];
+	struct scatterlist orig_sg[];
+};
+
+static unsigned tot_len(struct scatterlist sg[], unsigned num)
+{
+	unsigned len, i;
+
+	for (len = 0, i = 0; i < num; i++)
+		len += sg[i].length;
+
+	return len;
+}
+
+static void copy_sg_data(const struct scatterlist *dst, unsigned dnum,
+			 const struct scatterlist *src, unsigned snum)
+{
+	unsigned len;
+	struct scatterlist s, d;
+
+	s = *src;
+	d = *dst;
+
+	while (snum && dnum) {
+		len = min(s.length, d.length);
+		memcpy(sg_virt(&d), sg_virt(&s), len);
+		d.offset += len;
+		d.length -= len;
+		s.offset += len;
+		s.length -= len;
+		if (!s.length) {
+			BUG_ON(snum == 0);
+			src++;
+			snum--;
+			s = *src;
+		}
+		if (!d.length) {
+			BUG_ON(dnum == 0);
+			dst++;
+			dnum--;
+			d = *dst;
+		}
+	}
+}
+
+static bool torture_replace(struct scatterlist **sg,
+			     unsigned int *out,
+			     unsigned int *in,
+			     void **data,
+			     gfp_t gfp)
+{
+	static size_t seed;
+	struct torture *t;
+	unsigned long outlen, inlen, ourseed, len1;
+	void *buf;
+
+	if (!device_torture)
+		return true;
+
+	outlen = tot_len(*sg, *out);
+	inlen = tot_len(*sg + *out, *in);
+
+	/* This will break horribly on large block requests. */
+	t = kmalloc(sizeof(*t) + (*out + *in) * sizeof(t->orig_sg[1])
+		    + outlen + 1 + inlen + 1, gfp);
+	if (!t)
+		return false;
+
+	sg_init_table(t->sg, 4);
+	buf = &t->orig_sg[*out + *in];
+
+	memcpy(t->orig_sg, *sg, sizeof(**sg) * (*out + *in));
+	t->orig_out = *out;
+	t->orig_in = *in;
+	t->orig_data = *data;
+	*data = t;
+
+	ourseed = ACCESS_ONCE(seed);
+	seed++;
+
+	*sg = t->sg;
+	if (outlen) {
+		/* Split outbuf into two parts, one byte apart. */
+		*out = 2;
+		len1 = ourseed % (outlen + 1);
+		sg_set_buf(&t->sg[0], buf, len1);
+		buf += len1 + 1;
+		sg_set_buf(&t->sg[1], buf, outlen - len1);
+		buf += outlen - len1;
+		copy_sg_data(t->sg, *out, t->orig_sg, t->orig_out);
+	}
+
+	if (inlen) {
+		/* Split inbuf into two parts, one byte apart. */
+		*in = 2;
+		len1 = ourseed % (inlen + 1);
+		sg_set_buf(&t->sg[*out], buf, len1);
+		buf += len1 + 1;
+		sg_set_buf(&t->sg[*out + 1], buf, inlen - len1);
+		buf += inlen - len1;
+	}
+	return true;
+}
+
+static void *torture_done(struct torture *t)
+{
+	void *data;
+
+	if (!device_torture)
+		return t;
+
+	if (t->orig_in)
+		copy_sg_data(t->orig_sg + t->orig_out, t->orig_in,
+			     t->sg + (t->orig_out ? 2 : 0), 2);
+
+	data = t->orig_data;
+	kfree(t);
+	return data;
+}
+
+static unsigned long tot_inlen(struct virtqueue *vq, unsigned int i)
+{
+	struct vring_desc *desc;
+	unsigned long len = 0;
+
+	if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT) {
+		unsigned int num = vq->vring.desc[i].len / sizeof(*desc);
+		desc = phys_to_virt(vq->vring.desc[i].addr);
+
+		for (i = 0; i < num; i++) {
+			if (desc[i].flags & VRING_DESC_F_WRITE)
+				len += desc[i].flags.len;
+		}
+	} else {
+		desc = vq->vring.desc;
+		while (desc[i].flags & VRING_DESC_F_NEXT) {
+			if (desc[i].flags & VRING_DESC_F_WRITE)
+				len += desc[i].flags.len;
+			i = desc[i].next;
+		}
+	}
+
+	return len;
+}
+#else
+static bool torture_replace(struct scatterlist **sg,
+			     unsigned int *out,
+			     unsigned int *in,
+			     void **data,
+			     gfp_t gfp)
+{
+	return true;
+}
+
+static void *torture_done(void *data)
+{
+	return data;
+}
+
+#define BAD_RING(_vq, fmt, args...)				\
+	do {							\
+		dev_err(&_vq->vq.vdev->dev,			\
+			"%s:"fmt, (_vq)->vq.name, ##args);	\
+		(_vq)->broken = true;				\
+	} while (0)
+#define START_USE(vq)
+#define END_USE(vq)
+#endif /* CONFIG_VIRTIO_TORTURE */
+
 /* Set up an indirect table of descriptors and add it to the queue. */
 static int vring_add_indirect(struct vring_virtqueue *vq,
 			      struct scatterlist sg[],
@@ -228,7 +393,10 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 
 	BUG_ON(data == NULL);
 
-#ifdef DEBUG
+	if (!torture_replace(&sg, &out, &in, &data, gfp))
+		return -ENOMEM;
+
+#ifdef CONFIG_VIRTIO_TORTURE
 	{
 		ktime_t now = ktime_get();
 
@@ -261,6 +429,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
 		if (out)
 			vq->notify(&vq->vq);
 		END_USE(vq);
+		torture_done(data);
 		return -ENOSPC;
 	}
 
@@ -341,7 +510,7 @@ bool virtqueue_kick_prepare(struct virtqueue *_vq)
 	new = vq->vring.avail->idx;
 	vq->num_added = 0;
 
-#ifdef DEBUG
+#ifdef CONFIG_VIRTIO_TORTURE
 	if (vq->last_add_time_valid) {
 		WARN_ON(ktime_to_ms(ktime_sub(ktime_get(),
 					      vq->last_add_time)) > 100);
@@ -474,6 +643,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 		return NULL;
 	}
 
+#ifdef CONFIG_VIRTIO_TORTURE
+	if (unlikely(tot_inlen(vq, i) < *len)) {
+		BAD_RING(vq, "id %u: %u > %u used!\n",
+			 i, *len, tot_inlen(vq, i));
+		return NULL;
+	}
+#endif
+
 	/* detach_buf clears data, so grab it now. */
 	ret = vq->data[i];
 	detach_buf(vq, i);
@@ -486,12 +663,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 		virtio_mb(vq);
 	}
 
-#ifdef DEBUG
+#ifdef CONFIG_VIRTIO_TORTURE
 	vq->last_add_time_valid = false;
+	BUG_ON(*len > 
+
 #endif
 
 	END_USE(vq);
-	return ret;
+	return torture_done(ret);
 }
 EXPORT_SYMBOL_GPL(virtqueue_get_buf);
 
@@ -683,7 +862,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
 	vq->last_used_idx = 0;
 	vq->num_added = 0;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
-#ifdef DEBUG
+#ifdef CONFIG_VIRTIO_TORTURE
 	vq->in_use = false;
 	vq->last_add_time_valid = false;
 #endif

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Wed, Jun 19, 2013 at 02:16:06PM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@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@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@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@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.

Re: [PATCH] virtio-spec: add field for scsi command size

[Paolo Bonzini]

Il 20/06/2013 04:40, Rusty Russell ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 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.
> 
> Mainly because I'm not sure that *all* devices are now safe.  Are they?

virtio-scsi's implementation in QEMU is not safe (been delaying that for
too long, sorry), but the spec is safe.

Paolo

> I had a stress test half-written for this, pasted below.
> 
> Otherwise I'd be happy to do both: use feature 25 for
> VIRTIO_F_ANY_LAYOUT and another feature bit for the virtio-blk layout
> change.
> 
> Cheers,
> Rusty.
> 
> virtio: CONFIG_VIRTIO_DEVICE_TORTURE
> 
> Virtio devices are not supposed to depend on the framing of the scatter-gather
> lists, but various implementations did.  Safeguard this in future by adding
> an option to deliberately create perverse descriptors.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 8d5bddb..99c0187 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -5,6 +5,22 @@ config VIRTIO
>  	  bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST,
>  	  CONFIG_RPMSG or CONFIG_S390_GUEST.
>  
> +config VIRTIO_TORTURE
> +	bool "Virtio torture debugging"
> +	depends on VIRTIO && DEBUG_KERNEL
> +	help
> +
> +	  This makes the virtio_ring implementation stress-test
> +	  devices.  In particularly, creatively change the format of
> +	  requests to make sure that devices are properly implemented,
> +	  as well as implement various checks to ensure drivers are
> +	  correct.  This will make your virtual machine slow *and*
> +	  unreliable!  Say N.
> +
> +	  Put virtio_ring.device_torture to your boot commandline to
> +	  torture devices (otherwise only simply sanity checks are
> +	  done).
> +
>  menu "Virtio drivers"
>  
>  config VIRTIO_PCI
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index e82821a..6e5271c 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -45,35 +45,6 @@
>  #define virtio_wmb(vq) wmb()
>  #endif
>  
> -#ifdef DEBUG
> -/* For development, we want to crash whenever the ring is screwed. */
> -#define BAD_RING(_vq, fmt, args...)				\
> -	do {							\
> -		dev_err(&(_vq)->vq.vdev->dev,			\
> -			"%s:"fmt, (_vq)->vq.name, ##args);	\
> -		BUG();						\
> -	} while (0)
> -/* Caller is supposed to guarantee no reentry. */
> -#define START_USE(_vq)						\
> -	do {							\
> -		if ((_vq)->in_use)				\
> -			panic("%s:in_use = %i\n",		\
> -			      (_vq)->vq.name, (_vq)->in_use);	\
> -		(_vq)->in_use = __LINE__;			\
> -	} while (0)
> -#define END_USE(_vq) \
> -	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0)
> -#else
> -#define BAD_RING(_vq, fmt, args...)				\
> -	do {							\
> -		dev_err(&_vq->vq.vdev->dev,			\
> -			"%s:"fmt, (_vq)->vq.name, ##args);	\
> -		(_vq)->broken = true;				\
> -	} while (0)
> -#define START_USE(vq)
> -#define END_USE(vq)
> -#endif
> -
>  struct indirect_cache {
>  	unsigned int max;
>  	struct vring_desc *cache;
> @@ -109,7 +80,7 @@ struct vring_virtqueue
>  	/* How to notify other side. FIXME: commonalize hcalls! */
>  	void (*notify)(struct virtqueue *vq);
>  
> -#ifdef DEBUG
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	/* They're supposed to lock for us. */
>  	unsigned int in_use;
>  
> @@ -134,6 +105,200 @@ static inline struct indirect_cache *indirect_cache(struct vring_virtqueue *vq)
>  	return (struct indirect_cache *)&vq->data[vq->vring.num];
>  }
>  
> +#ifdef CONFIG_VIRTIO_TORTURE
> +/* For development, we want to crash whenever the ring is screwed. */
> +#define BAD_RING(_vq, fmt, args...)				\
> +	do {							\
> +		dev_err(&(_vq)->vq.vdev->dev,			\
> +			"%s:"fmt, (_vq)->vq.name, ##args);	\
> +		BUG();						\
> +	} while (0)
> +/* Caller is supposed to guarantee no reentry. */
> +#define START_USE(_vq)						\
> +	do {							\
> +		if ((_vq)->in_use)				\
> +			panic("%s:in_use = %i\n",		\
> +			      (_vq)->vq.name, (_vq)->in_use);	\
> +		(_vq)->in_use = __LINE__;			\
> +	} while (0)
> +#define END_USE(_vq) \
> +	do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0)
> +
> +static bool device_torture;
> +module_param(device_torture, bool, 0644);
> +
> +struct torture {
> +	unsigned int orig_out, orig_in;
> +	void *orig_data;
> +	struct scatterlist sg[4];
> +	struct scatterlist orig_sg[];
> +};
> +
> +static unsigned tot_len(struct scatterlist sg[], unsigned num)
> +{
> +	unsigned len, i;
> +
> +	for (len = 0, i = 0; i < num; i++)
> +		len += sg[i].length;
> +
> +	return len;
> +}
> +
> +static void copy_sg_data(const struct scatterlist *dst, unsigned dnum,
> +			 const struct scatterlist *src, unsigned snum)
> +{
> +	unsigned len;
> +	struct scatterlist s, d;
> +
> +	s = *src;
> +	d = *dst;
> +
> +	while (snum && dnum) {
> +		len = min(s.length, d.length);
> +		memcpy(sg_virt(&d), sg_virt(&s), len);
> +		d.offset += len;
> +		d.length -= len;
> +		s.offset += len;
> +		s.length -= len;
> +		if (!s.length) {
> +			BUG_ON(snum == 0);
> +			src++;
> +			snum--;
> +			s = *src;
> +		}
> +		if (!d.length) {
> +			BUG_ON(dnum == 0);
> +			dst++;
> +			dnum--;
> +			d = *dst;
> +		}
> +	}
> +}
> +
> +static bool torture_replace(struct scatterlist **sg,
> +			     unsigned int *out,
> +			     unsigned int *in,
> +			     void **data,
> +			     gfp_t gfp)
> +{
> +	static size_t seed;
> +	struct torture *t;
> +	unsigned long outlen, inlen, ourseed, len1;
> +	void *buf;
> +
> +	if (!device_torture)
> +		return true;
> +
> +	outlen = tot_len(*sg, *out);
> +	inlen = tot_len(*sg + *out, *in);
> +
> +	/* This will break horribly on large block requests. */
> +	t = kmalloc(sizeof(*t) + (*out + *in) * sizeof(t->orig_sg[1])
> +		    + outlen + 1 + inlen + 1, gfp);
> +	if (!t)
> +		return false;
> +
> +	sg_init_table(t->sg, 4);
> +	buf = &t->orig_sg[*out + *in];
> +
> +	memcpy(t->orig_sg, *sg, sizeof(**sg) * (*out + *in));
> +	t->orig_out = *out;
> +	t->orig_in = *in;
> +	t->orig_data = *data;
> +	*data = t;
> +
> +	ourseed = ACCESS_ONCE(seed);
> +	seed++;
> +
> +	*sg = t->sg;
> +	if (outlen) {
> +		/* Split outbuf into two parts, one byte apart. */
> +		*out = 2;
> +		len1 = ourseed % (outlen + 1);
> +		sg_set_buf(&t->sg[0], buf, len1);
> +		buf += len1 + 1;
> +		sg_set_buf(&t->sg[1], buf, outlen - len1);
> +		buf += outlen - len1;
> +		copy_sg_data(t->sg, *out, t->orig_sg, t->orig_out);
> +	}
> +
> +	if (inlen) {
> +		/* Split inbuf into two parts, one byte apart. */
> +		*in = 2;
> +		len1 = ourseed % (inlen + 1);
> +		sg_set_buf(&t->sg[*out], buf, len1);
> +		buf += len1 + 1;
> +		sg_set_buf(&t->sg[*out + 1], buf, inlen - len1);
> +		buf += inlen - len1;
> +	}
> +	return true;
> +}
> +
> +static void *torture_done(struct torture *t)
> +{
> +	void *data;
> +
> +	if (!device_torture)
> +		return t;
> +
> +	if (t->orig_in)
> +		copy_sg_data(t->orig_sg + t->orig_out, t->orig_in,
> +			     t->sg + (t->orig_out ? 2 : 0), 2);
> +
> +	data = t->orig_data;
> +	kfree(t);
> +	return data;
> +}
> +
> +static unsigned long tot_inlen(struct virtqueue *vq, unsigned int i)
> +{
> +	struct vring_desc *desc;
> +	unsigned long len = 0;
> +
> +	if (vq->vring.desc[i].flags & VRING_DESC_F_INDIRECT) {
> +		unsigned int num = vq->vring.desc[i].len / sizeof(*desc);
> +		desc = phys_to_virt(vq->vring.desc[i].addr);
> +
> +		for (i = 0; i < num; i++) {
> +			if (desc[i].flags & VRING_DESC_F_WRITE)
> +				len += desc[i].flags.len;
> +		}
> +	} else {
> +		desc = vq->vring.desc;
> +		while (desc[i].flags & VRING_DESC_F_NEXT) {
> +			if (desc[i].flags & VRING_DESC_F_WRITE)
> +				len += desc[i].flags.len;
> +			i = desc[i].next;
> +		}
> +	}
> +
> +	return len;
> +}
> +#else
> +static bool torture_replace(struct scatterlist **sg,
> +			     unsigned int *out,
> +			     unsigned int *in,
> +			     void **data,
> +			     gfp_t gfp)
> +{
> +	return true;
> +}
> +
> +static void *torture_done(void *data)
> +{
> +	return data;
> +}
> +
> +#define BAD_RING(_vq, fmt, args...)				\
> +	do {							\
> +		dev_err(&_vq->vq.vdev->dev,			\
> +			"%s:"fmt, (_vq)->vq.name, ##args);	\
> +		(_vq)->broken = true;				\
> +	} while (0)
> +#define START_USE(vq)
> +#define END_USE(vq)
> +#endif /* CONFIG_VIRTIO_TORTURE */
> +
>  /* Set up an indirect table of descriptors and add it to the queue. */
>  static int vring_add_indirect(struct vring_virtqueue *vq,
>  			      struct scatterlist sg[],
> @@ -228,7 +393,10 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  
>  	BUG_ON(data == NULL);
>  
> -#ifdef DEBUG
> +	if (!torture_replace(&sg, &out, &in, &data, gfp))
> +		return -ENOMEM;
> +
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	{
>  		ktime_t now = ktime_get();
>  
> @@ -261,6 +429,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
>  		if (out)
>  			vq->notify(&vq->vq);
>  		END_USE(vq);
> +		torture_done(data);
>  		return -ENOSPC;
>  	}
>  
> @@ -341,7 +510,7 @@ bool virtqueue_kick_prepare(struct virtqueue *_vq)
>  	new = vq->vring.avail->idx;
>  	vq->num_added = 0;
>  
> -#ifdef DEBUG
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	if (vq->last_add_time_valid) {
>  		WARN_ON(ktime_to_ms(ktime_sub(ktime_get(),
>  					      vq->last_add_time)) > 100);
> @@ -474,6 +643,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>  		return NULL;
>  	}
>  
> +#ifdef CONFIG_VIRTIO_TORTURE
> +	if (unlikely(tot_inlen(vq, i) < *len)) {
> +		BAD_RING(vq, "id %u: %u > %u used!\n",
> +			 i, *len, tot_inlen(vq, i));
> +		return NULL;
> +	}
> +#endif
> +
>  	/* detach_buf clears data, so grab it now. */
>  	ret = vq->data[i];
>  	detach_buf(vq, i);
> @@ -486,12 +663,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>  		virtio_mb(vq);
>  	}
>  
> -#ifdef DEBUG
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	vq->last_add_time_valid = false;
> +	BUG_ON(*len > 
> +
>  #endif
>  
>  	END_USE(vq);
> -	return ret;
> +	return torture_done(ret);
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_get_buf);
>  
> @@ -683,7 +862,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>  	vq->last_used_idx = 0;
>  	vq->num_added = 0;
>  	list_add_tail(&vq->vq.list, &vdev->vqs);
> -#ifdef DEBUG
> +#ifdef CONFIG_VIRTIO_TORTURE
>  	vq->in_use = false;
>  	vq->last_add_time_valid = false;
>  #endif
> 

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 20/06/2013 04:40, Rusty Russell ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> 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.
>> 
>> Mainly because I'm not sure that *all* devices are now safe.  Are they?
>
> virtio-scsi's implementation in QEMU is not safe (been delaying that for
> too long, sorry), but the spec is safe.

Then if we added a transport feature, we couldn't use it :(

So I think it needs to be a per-device feature bit.

Cheers,
Rusty.

Re: [PATCH] virtio-spec: add field for scsi command size

[Paolo Bonzini]

Il 01/07/2013 01:47, Rusty Russell ha scritto:
> > > 
> > > Mainly because I'm not sure that *all* devices are now safe.  Are they?
> >
> > virtio-scsi's implementation in QEMU is not safe (been delaying that for
> > too long, sorry), but the spec is safe.
> 
> Then if we added a transport feature, we couldn't use it :(

Transport feature bits are still negotiated per device though.
virtio-scsi devices in QEMU would not negotiate that feature.

Paolo

> So I think it needs to be a per-device feature bit.

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 01/07/2013 01:47, Rusty Russell ha scritto:
>> > > 
>> > > Mainly because I'm not sure that *all* devices are now safe.  Are they?
>> >
>> > virtio-scsi's implementation in QEMU is not safe (been delaying that for
>> > too long, sorry), but the spec is safe.
>> 
>> Then if we added a transport feature, we couldn't use it :(
>
> Transport feature bits are still negotiated per device though.
> virtio-scsi devices in QEMU would not negotiate that feature.

That's a good point; I tend to think of them as tied to the transport
but there's nothing specifying that, nor any implementation requiring
it.

OK, so VIRTIO_F_ANY_LAYOUT it is?

Cheers,
Rusty.

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 using VIRTIO_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 dividing a network packet into 1500
single-byte descriptors!

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Mon, Jul 01, 2013 at 09:17:13AM +0930, Rusty Russell wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 20/06/2013 04:40, Rusty Russell ha scritto:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >>> 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.
> >> 
> >> Mainly because I'm not sure that *all* devices are now safe.  Are they?
> >
> > virtio-scsi's implementation in QEMU is not safe (been delaying that for
> > too long, sorry), but the spec is safe.
> 
> Then if we added a transport feature, we couldn't use it :(
> 
> So I think it needs to be a per-device feature bit.
> 
> Cheers,
> Rusty.

Yea.
Could you push the updated spec so I can send the updated patch?
Would be nice to have the virtio-net optimization in 3.11.

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Mon, Jul 01, 2013 at 01:57:44PM +0200, Paolo Bonzini wrote:
> Il 01/07/2013 01:47, Rusty Russell ha scritto:
> > > > 
> > > > Mainly because I'm not sure that *all* devices are now safe.  Are they?
> > >
> > > virtio-scsi's implementation in QEMU is not safe (been delaying that for
> > > too long, sorry), but the spec is safe.
> > 
> > Then if we added a transport feature, we couldn't use it :(
> 
> Transport feature bits are still negotiated per device though.
> virtio-scsi devices in QEMU would not negotiate that feature.
> 
> Paolo
> 
> > So I think it needs to be a per-device feature bit.

We don't know that it's helpful for any devices besides -net
so I'd say keep it simple.

-- 
MST

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Tue, Jul 02, 2013 at 03:34:09PM +0930, Rusty Russell wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> > Il 01/07/2013 01:47, Rusty Russell ha scritto:
> >> > > 
> >> > > Mainly because I'm not sure that *all* devices are now safe.  Are they?
> >> >
> >> > virtio-scsi's implementation in QEMU is not safe (been delaying that for
> >> > too long, sorry), but the spec is safe.
> >> 
> >> Then if we added a transport feature, we couldn't use it :(
> >
> > Transport feature bits are still negotiated per device though.
> > virtio-scsi devices in QEMU would not negotiate that feature.
> 
> That's a good point; I tend to think of them as tied to the transport
> but there's nothing specifying that, nor any implementation requiring
> it.
> 
> OK, so VIRTIO_F_ANY_LAYOUT it is?
> 
> Cheers,
> Rusty.
> 
> 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 using VIRTIO_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 dividing a network packet into 1500
> single-byte descriptors!

That's fine with me too.
So which bit are we using for this?
I'd like to rebase to latest bits and merge the optimization for 3.11.

-- 
MST

Re: [PATCH] virtio-spec: add field for scsi command size

[Paolo Bonzini]

Il 04/07/2013 09:39, Michael S. Tsirkin ha scritto:
>>>>> > > > > Mainly because I'm not sure that *all* devices are now safe.  Are they?
>>>> > > >
>>>> > > > virtio-scsi's implementation in QEMU is not safe (been delaying that for
>>>> > > > too long, sorry), but the spec is safe.
>>> > > 
>>> > > Then if we added a transport feature, we couldn't use it :(
>> > 
>> > Transport feature bits are still negotiated per device though.
>> > virtio-scsi devices in QEMU would not negotiate that feature.
>> > 
>> > Paolo
>> > 
>>> > > So I think it needs to be a per-device feature bit.
> We don't know that it's helpful for any devices besides -net
> so I'd say keep it simple.

Then we should stop calling it a "device model bug".

If we call it a bug, we must strive for support in all devices.  If we
don't care, we should stop calling it a bug.

Paolo

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Thu, Jul 04, 2013 at 10:49:42AM +0300, Michael S. Tsirkin wrote:
> On Tue, Jul 02, 2013 at 03:34:09PM +0930, Rusty Russell wrote:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > > Il 01/07/2013 01:47, Rusty Russell ha scritto:
> > >> > > 
> > >> > > Mainly because I'm not sure that *all* devices are now safe.  Are they?
> > >> >
> > >> > virtio-scsi's implementation in QEMU is not safe (been delaying that for
> > >> > too long, sorry), but the spec is safe.
> > >> 
> > >> Then if we added a transport feature, we couldn't use it :(
> > >
> > > Transport feature bits are still negotiated per device though.
> > > virtio-scsi devices in QEMU would not negotiate that feature.
> > 
> > That's a good point; I tend to think of them as tied to the transport
> > but there's nothing specifying that, nor any implementation requiring
> > it.
> > 
> > OK, so VIRTIO_F_ANY_LAYOUT it is?
> > 
> > Cheers,
> > Rusty.
> > 
> > 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 using VIRTIO_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 dividing a network packet into 1500
> > single-byte descriptors!
> 
> That's fine with me too.
> So which bit are we using for this?
> I'd like to rebase to latest bits and merge the optimization for 3.11.


Rusty, could you please tell me which feature bit do you
prefer for ANY_LAYOUT?
It would be sad to miss another release of both qemu and kernel with
this obvious optimization for the only reason we can't settle on a bit
to use to signal it.


> -- 
> MST

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Jul 04, 2013 at 10:49:42AM +0300, Michael S. Tsirkin wrote:
>> > 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 using VIRTIO_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 dividing a network packet into 1500
>> > single-byte descriptors!
>> 
>> That's fine with me too.
>> So which bit are we using for this?
>> I'd like to rebase to latest bits and merge the optimization for 3.11.
>
>
> Rusty, could you please tell me which feature bit do you
> prefer for ANY_LAYOUT?
> It would be sad to miss another release of both qemu and kernel with
> this obvious optimization for the only reason we can't settle on a bit
> to use to signal it.

Let's use bit 30.  Here's the kernel patch:

virtio: VIRTIO_F_ANY_LAYOUT feature

Also known as the "no really, I read the spec" bit.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index b7cda39..4b5da48 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -51,4 +51,7 @@
  * suppressed them? */
 #define VIRTIO_F_NOTIFY_ON_EMPTY	24
 
+/* Can the device handle any descriptor layout? */
+#define VIRTIO_F_ANY_LAYOUT		30
+
 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 04/07/2013 09:39, Michael S. Tsirkin ha scritto:
>>> > Transport feature bits are still negotiated per device though.
>>> > virtio-scsi devices in QEMU would not negotiate that feature.
>>> > 
>>> > Paolo
>>> > 
>>>> > > So I think it needs to be a per-device feature bit.
>> We don't know that it's helpful for any devices besides -net
>> so I'd say keep it simple.
>
> Then we should stop calling it a "device model bug".
>
> If we call it a bug, we must strive for support in all devices.  If we
> don't care, we should stop calling it a bug.

This has been a cause of hot debate: this bug has fossilized into a
feature, so we ignored it as long as we could :)

I plan on proposing that this be a requirement for the 1.0 spec, so we
might as well fix it everywhere now in preparation.

Cheers,
Rusty.

Re: [PATCH] virtio-spec: add field for scsi command size

[Michael S. Tsirkin]

On Mon, Jul 08, 2013 at 10:51:39AM +0930, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Thu, Jul 04, 2013 at 10:49:42AM +0300, Michael S. Tsirkin wrote:
> >> > 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 using VIRTIO_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 dividing a network packet into 1500
> >> > single-byte descriptors!
> >> 
> >> That's fine with me too.
> >> So which bit are we using for this?
> >> I'd like to rebase to latest bits and merge the optimization for 3.11.
> >
> >
> > Rusty, could you please tell me which feature bit do you
> > prefer for ANY_LAYOUT?
> > It would be sad to miss another release of both qemu and kernel with
> > this obvious optimization for the only reason we can't settle on a bit
> > to use to signal it.
> 
> Let's use bit 30.  Here's the kernel patch:
> 
> virtio: VIRTIO_F_ANY_LAYOUT feature
> 
> Also known as the "no really, I read the spec" bit.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

This is already used in qemu:
/* A guest should never accept this.  It implies negotiation is broken. */
#define VIRTIO_F_BAD_FEATURE            30



> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> index b7cda39..4b5da48 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -51,4 +51,7 @@
>   * suppressed them? */
>  #define VIRTIO_F_NOTIFY_ON_EMPTY	24
>  
> +/* Can the device handle any descriptor layout? */
> +#define VIRTIO_F_ANY_LAYOUT		30
> +
>  #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */

Re: [PATCH] virtio-spec: add field for scsi command size

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Mon, Jul 08, 2013 at 10:51:39AM +0930, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Thu, Jul 04, 2013 at 10:49:42AM +0300, Michael S. Tsirkin wrote:
>> >> > 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 using VIRTIO_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 dividing a network packet into 1500
>> >> > single-byte descriptors!
>> >> 
>> >> That's fine with me too.
>> >> So which bit are we using for this?
>> >> I'd like to rebase to latest bits and merge the optimization for 3.11.
>> >
>> >
>> > Rusty, could you please tell me which feature bit do you
>> > prefer for ANY_LAYOUT?
>> > It would be sad to miss another release of both qemu and kernel with
>> > this obvious optimization for the only reason we can't settle on a bit
>> > to use to signal it.
>> 
>> Let's use bit 30.  Here's the kernel patch:
>> 
>> virtio: VIRTIO_F_ANY_LAYOUT feature
>> 
>> Also known as the "no really, I read the spec" bit.
>> 
>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>
> This is already used in qemu:
> /* A guest should never accept this.  It implies negotiation is broken. */
> #define VIRTIO_F_BAD_FEATURE            30

How annoying.  Changed to bit 27, which isn't used.  Which forced a
rebase of my virtio-next tree, but I really don't want the old version
exposed in mainline, even for 3 commits.

Thanks,
Rusty.

commit 62525a00b87cc967bce9779d63fcc84fb9199130
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Mon Jul 8 10:33:37 2013 +0930

    virtio: VIRTIO_F_ANY_LAYOUT feature
    
    Also known as the "no really, I read the spec" bit.
    
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index b7cda39..3ce768c 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -51,4 +51,7 @@
  * suppressed them? */
 #define VIRTIO_F_NOTIFY_ON_EMPTY	24
 
+/* Can the device handle any descriptor layout? */
+#define VIRTIO_F_ANY_LAYOUT		27
+
 #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */