[PATCH] virtio-ccw: commands on revision-less devices

[PATCH] virtio-ccw: commands on revision-less devices

[Cornelia Huck]

The virtio standard specifies that any non-transitional device must
reject commands prior to revision setting (which we do) and else
assume revision 0 (legacy) if the driver sends a non-revision-setting
command first. We neglected to do the latter.

Fortunately, nearly everything worked as intended anyway; the only
problem was not properly rejecting revision setting after some other
command had been issued. Easy to fix by setting revision to 0 if
we see a non-revision command on a legacy-capable revision-less
device.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/virtio-ccw.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 4582e94ae7dc..06c06056814b 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                                    ccw.cmd_code);
     check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
 
-    if (dev->force_revision_1 && dev->revision < 0 &&
-        ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
-        /*
-         * virtio-1 drivers must start with negotiating to a revision >= 1,
-         * so post a command reject for all other commands
-         */
-        return -ENOSYS;
+    if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
+        if (dev->force_revision_1) {
+            /*
+             * virtio-1 drivers must start with negotiating to a revision >= 1,
+             * so post a command reject for all other commands
+             */
+            return -ENOSYS;
+        } else {
+            /*
+             * If the driver issues any command that is not SET_VIRTIO_REV,
+             * we'll have to operate the device in legacy mode.
+             */
+            dev->revision = 0;
+        }
     }
 
     /* Look at the command. */
-- 
2.26.2

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Thomas Huth]

On 16/02/2021 12.18, Cornelia Huck wrote:
> The virtio standard specifies that any non-transitional device must
> reject commands prior to revision setting (which we do) and else
> assume revision 0 (legacy) if the driver sends a non-revision-setting
> command first. We neglected to do the latter.
> 
> Fortunately, nearly everything worked as intended anyway; the only
> problem was not properly rejecting revision setting after some other
> command had been issued. Easy to fix by setting revision to 0 if
> we see a non-revision command on a legacy-capable revision-less
> device.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   hw/s390x/virtio-ccw.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 4582e94ae7dc..06c06056814b 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                                      ccw.cmd_code);
>       check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
>   
> -    if (dev->force_revision_1 && dev->revision < 0 &&
> -        ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
> -        /*
> -         * virtio-1 drivers must start with negotiating to a revision >= 1,
> -         * so post a command reject for all other commands
> -         */
> -        return -ENOSYS;
> +    if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
> +        if (dev->force_revision_1) {
> +            /*
> +             * virtio-1 drivers must start with negotiating to a revision >= 1,
> +             * so post a command reject for all other commands
> +             */
> +            return -ENOSYS;
> +        } else {
> +            /*
> +             * If the driver issues any command that is not SET_VIRTIO_REV,
> +             * we'll have to operate the device in legacy mode.
> +             */
> +            dev->revision = 0;
> +        }
>       }
>   
>       /* Look at the command. */
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Michael S. Tsirkin]

On Tue, Feb 16, 2021 at 12:18:30PM +0100, Cornelia Huck wrote:
> The virtio standard specifies that any non-transitional device must
> reject commands prior to revision setting (which we do) and else
> assume revision 0 (legacy) if the driver sends a non-revision-setting
> command first. We neglected to do the latter.
> 
> Fortunately, nearly everything worked as intended anyway; the only
> problem was not properly rejecting revision setting after some other
> command had been issued. Easy to fix by setting revision to 0 if
> we see a non-revision command on a legacy-capable revision-less
> device.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Feel free to merge.

Curious how was this discovered.

> ---
>  hw/s390x/virtio-ccw.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 4582e94ae7dc..06c06056814b 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                                     ccw.cmd_code);
>      check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
>  
> -    if (dev->force_revision_1 && dev->revision < 0 &&
> -        ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
> -        /*
> -         * virtio-1 drivers must start with negotiating to a revision >= 1,
> -         * so post a command reject for all other commands
> -         */
> -        return -ENOSYS;
> +    if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
> +        if (dev->force_revision_1) {
> +            /*
> +             * virtio-1 drivers must start with negotiating to a revision >= 1,
> +             * so post a command reject for all other commands
> +             */
> +            return -ENOSYS;
> +        } else {
> +            /*
> +             * If the driver issues any command that is not SET_VIRTIO_REV,
> +             * we'll have to operate the device in legacy mode.
> +             */
> +            dev->revision = 0;
> +        }
>      }
>  
>      /* Look at the command. */
> -- 
> 2.26.2

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Cornelia Huck]

On Tue, 16 Feb 2021 06:40:41 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Feb 16, 2021 at 12:18:30PM +0100, Cornelia Huck wrote:
> > The virtio standard specifies that any non-transitional device must
> > reject commands prior to revision setting (which we do) and else
> > assume revision 0 (legacy) if the driver sends a non-revision-setting
> > command first. We neglected to do the latter.
> > 
> > Fortunately, nearly everything worked as intended anyway; the only
> > problem was not properly rejecting revision setting after some other
> > command had been issued. Easy to fix by setting revision to 0 if
> > we see a non-revision command on a legacy-capable revision-less
> > device.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>  
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Feel free to merge.
> 
> Curious how was this discovered.

Code reading :)

I have not seen any errors in the wild.

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Michael S. Tsirkin]

On Tue, Feb 16, 2021 at 12:51:50PM +0100, Cornelia Huck wrote:
> On Tue, 16 Feb 2021 06:40:41 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Feb 16, 2021 at 12:18:30PM +0100, Cornelia Huck wrote:
> > > The virtio standard specifies that any non-transitional device must
> > > reject commands prior to revision setting (which we do) and else
> > > assume revision 0 (legacy) if the driver sends a non-revision-setting
> > > command first. We neglected to do the latter.
> > > 
> > > Fortunately, nearly everything worked as intended anyway; the only
> > > problem was not properly rejecting revision setting after some other
> > > command had been issued. Easy to fix by setting revision to 0 if
> > > we see a non-revision command on a legacy-capable revision-less
> > > device.
> > > 
> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>  
> > 
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > Feel free to merge.
> > 
> > Curious how was this discovered.
> 
> Code reading :)
> 
> I have not seen any errors in the wild.

Worth including in the commit log.

-- 
MST

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Halil Pasic]

On Tue, 16 Feb 2021 12:18:30 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> The virtio standard specifies that any non-transitional device must
> reject commands prior to revision setting (which we do) and else
> assume revision 0 (legacy) if the driver sends a non-revision-setting
> command first. We neglected to do the latter.

Huh, I my opinion, it ain't very clear what is specified by the virtio
standard (which starts with version 1.0) for the described situation.

The corresponding device normative section (4.3.2.1.1 Device
Requirements: Setting the Virtio Revision) says that: "A device MUST
treat the revision as unset from the time the associated subchannel has
been enabled until a revision has been successfully set by the driver.
This implies that revisions are not persistent across disabling and
enabling of the associated subchannel.". It doesn't say anything more
about the situation where the first command is not SET_VIRTIO_REV.

The section "4.3.2.1.3 Legacy Interfaces: A Note on Setting the Virtio
Revision" which is to my best knowledge not normative, as none of the
legacy-interface stuff is normative, but a mere advice on how to deal
with legacy then says: "A legacy driver will not issue the
CCW_CMD_SET_VIRTIO_REV prior to issuing other virtio-ccw specific
channel commands." ... "A transitional device MUST assume
in this case that the driver is a legacy driver and continue as if the
driver selected revision 0. This implies that the device MUST reject any
command not valid for revision 0, including a subsequent
CCW_CMD_SET_VIRTIO_REV."

Do we agree that the legacy interface sections in general, and 4.3.2.1.3
in particular is non-normative?

In my opinion the normative 'must threat as unset until set by driver'
and 'if first cmd is not _REV, must continue as if the driver selected
revision 0' is in a slight collision.


> 
> Fortunately, nearly everything worked as intended anyway; the only
> problem was not properly rejecting revision setting after some other
> command had been issued. Easy to fix by setting revision to 0 if
> we see a non-revision command on a legacy-capable revision-less
> device.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>

The change won't hurt so with a toned down commit message:
Acked-by: Halil Pasic <pasic@linux.ibm.com>

> ---
>  hw/s390x/virtio-ccw.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 4582e94ae7dc..06c06056814b 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                                     ccw.cmd_code);
>      check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
>  
> -    if (dev->force_revision_1 && dev->revision < 0 &&
> -        ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
> -        /*
> -         * virtio-1 drivers must start with negotiating to a revision >= 1,
> -         * so post a command reject for all other commands
> -         */
> -        return -ENOSYS;
> +    if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
> +        if (dev->force_revision_1) {
> +            /*
> +             * virtio-1 drivers must start with negotiating to a revision >= 1,
> +             * so post a command reject for all other commands
> +             */
> +            return -ENOSYS;
> +        } else {
> +            /*
> +             * If the driver issues any command that is not SET_VIRTIO_REV,
> +             * we'll have to operate the device in legacy mode.
> +             */
> +            dev->revision = 0;
> +        }
>      }
>  
>      /* Look at the command. */

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Cornelia Huck]

On Tue, 16 Feb 2021 15:19:45 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 16 Feb 2021 12:18:30 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > The virtio standard specifies that any non-transitional device must
> > reject commands prior to revision setting (which we do) and else
> > assume revision 0 (legacy) if the driver sends a non-revision-setting
> > command first. We neglected to do the latter.  
> 
> Huh, I my opinion, it ain't very clear what is specified by the virtio
> standard (which starts with version 1.0) for the described situation.
> 
> The corresponding device normative section (4.3.2.1.1 Device
> Requirements: Setting the Virtio Revision) says that: "A device MUST
> treat the revision as unset from the time the associated subchannel has
> been enabled until a revision has been successfully set by the driver.
> This implies that revisions are not persistent across disabling and
> enabling of the associated subchannel.". It doesn't say anything more
> about the situation where the first command is not SET_VIRTIO_REV.
> 
> The section "4.3.2.1.3 Legacy Interfaces: A Note on Setting the Virtio
> Revision" which is to my best knowledge not normative, as none of the
> legacy-interface stuff is normative, but a mere advice on how to deal
> with legacy then says: "A legacy driver will not issue the
> CCW_CMD_SET_VIRTIO_REV prior to issuing other virtio-ccw specific
> channel commands." ... "A transitional device MUST assume
> in this case that the driver is a legacy driver and continue as if the
> driver selected revision 0. This implies that the device MUST reject any
> command not valid for revision 0, including a subsequent
> CCW_CMD_SET_VIRTIO_REV."
> 
> Do we agree that the legacy interface sections in general, and 4.3.2.1.3
> in particular is non-normative?

IMHO, normative and non-normative are not something that applies to the
legacy sections. The legacy sections are supposed to give guidance on
how to write transitional devices/drivers that can deal with legacy
implementations. If you want to write something that strictly only
adheres to normative statements, you have to write a non-transitional
device/driver. Legacy support was never an official standard, so
'normative' is meaningless in that context.

> 
> In my opinion the normative 'must threat as unset until set by driver'
> and 'if first cmd is not _REV, must continue as if the driver selected
> revision 0' is in a slight collision.

I don't think there's a collision. If we want to accommodate legacy
drivers, we have to deal with their lack of revision handling, and
therefore treat non-_REV commands as implicitly selecting revision 0.

If we want to implement a non-transitional device, the implicit
selection of revision 0 goes away, of course. In fact, I'm currently
trying to make legacy support optional for virtio-ccw, so that's why I
had been looking at the revision handling :)

> 
> 
> > 
> > Fortunately, nearly everything worked as intended anyway; the only
> > problem was not properly rejecting revision setting after some other
> > command had been issued. Easy to fix by setting revision to 0 if
> > we see a non-revision command on a legacy-capable revision-less
> > device.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>  
> 
> The change won't hurt so with a toned down commit message:
> Acked-by: Halil Pasic <pasic@linux.ibm.com>

Replace 'and else' with 'a transitional device needs to'?

> 
> > ---
> >  hw/s390x/virtio-ccw.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index 4582e94ae7dc..06c06056814b 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -327,13 +327,20 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >                                     ccw.cmd_code);
> >      check_len = !((ccw.flags & CCW_FLAG_SLI) && !(ccw.flags & CCW_FLAG_DC));
> >  
> > -    if (dev->force_revision_1 && dev->revision < 0 &&
> > -        ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
> > -        /*
> > -         * virtio-1 drivers must start with negotiating to a revision >= 1,
> > -         * so post a command reject for all other commands
> > -         */
> > -        return -ENOSYS;
> > +    if (dev->revision < 0 && ccw.cmd_code != CCW_CMD_SET_VIRTIO_REV) {
> > +        if (dev->force_revision_1) {
> > +            /*
> > +             * virtio-1 drivers must start with negotiating to a revision >= 1,
> > +             * so post a command reject for all other commands
> > +             */
> > +            return -ENOSYS;
> > +        } else {
> > +            /*
> > +             * If the driver issues any command that is not SET_VIRTIO_REV,
> > +             * we'll have to operate the device in legacy mode.
> > +             */
> > +            dev->revision = 0;
> > +        }
> >      }
> >  
> >      /* Look at the command. */  
> 

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Halil Pasic]

On Tue, 16 Feb 2021 16:54:05 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 16 Feb 2021 15:19:45 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Tue, 16 Feb 2021 12:18:30 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > The virtio standard specifies that any non-transitional device must
> > > reject commands prior to revision setting (which we do) and else
> > > assume revision 0 (legacy) if the driver sends a non-revision-setting
> > > command first. We neglected to do the latter.    
> > 
> > Huh, I my opinion, it ain't very clear what is specified by the virtio
> > standard (which starts with version 1.0) for the described situation.
> > 
> > The corresponding device normative section (4.3.2.1.1 Device
> > Requirements: Setting the Virtio Revision) says that: "A device MUST
> > treat the revision as unset from the time the associated subchannel has
> > been enabled until a revision has been successfully set by the driver.
> > This implies that revisions are not persistent across disabling and
> > enabling of the associated subchannel.". It doesn't say anything more
> > about the situation where the first command is not SET_VIRTIO_REV.
> > 
> > The section "4.3.2.1.3 Legacy Interfaces: A Note on Setting the Virtio
> > Revision" which is to my best knowledge not normative, as none of the
> > legacy-interface stuff is normative, but a mere advice on how to deal
> > with legacy then says: "A legacy driver will not issue the
> > CCW_CMD_SET_VIRTIO_REV prior to issuing other virtio-ccw specific
> > channel commands." ... "A transitional device MUST assume
> > in this case that the driver is a legacy driver and continue as if the
> > driver selected revision 0. This implies that the device MUST reject any
> > command not valid for revision 0, including a subsequent
> > CCW_CMD_SET_VIRTIO_REV."
> > 
> > Do we agree that the legacy interface sections in general, and 4.3.2.1.3
> > in particular is non-normative?  
> 
> IMHO, normative and non-normative are not something that applies to the
> legacy sections. The legacy sections are supposed to give guidance on
> how to write transitional devices/drivers that can deal with legacy
> implementations. If you want to write something that strictly only
> adheres to normative statements, you have to write a non-transitional
> device/driver. Legacy support was never an official standard, so
> 'normative' is meaningless in that context.

Exactly, so the legacy stuff is not normative, and strictly speaking not
included in the standard (i.e. standardized).

But then I find usage of keywords like MUST in legacy interface sections
misreading. I believe some Oasis guy complained about keyword usage
outside of normative sections before.

> 
> > 
> > In my opinion the normative 'must threat as unset until set by driver'
> > and 'if first cmd is not _REV, must continue as if the driver selected
> > revision 0' is in a slight collision.  
> 
> I don't think there's a collision. If we want to accommodate legacy
> drivers, we have to deal with their lack of revision handling, and
> therefore treat non-_REV commands as implicitly selecting revision 0.
> 
> If we want to implement a non-transitional device, the implicit
> selection of revision 0 goes away, of course. In fact, I'm currently
> trying to make legacy support optional for virtio-ccw, so that's why I
> had been looking at the revision handling :)

Do you mean optional like build time configurable in QEMU? I think the
legacy support is already optional when it comes to the spec.

Let me explain how do I interpret device compliance with respect to
virtio revisions and first command is a non-_REV.

1) If the first virtio command after the virtio-ccw device is enabled is
a non-_REV command, the virtio-ccw device not answering it with a
command reject does not preclude the device form being virtio 1.0
conform. I.e. this behavior is conform, because does not violate
any of the sections linked in "7.3.3 Clause 17: Channel I/O Device
Conformance" in general, and thus does not violate "4.3.2.1.1 Device
Requirements: Setting the Virtio Revision" in particular. If you disagree,
please point me to the corresponding device normative section.

2) Rejecting the first command which which happens to be a non-_REV
however does not preclude virtio (1.0) conformance either. The device
is free to do whatever, because in my reading it ain't specified what
needs to be done.

3) It is OK-ish, that the device is free to do anything there, because
a virtio 1.0 conform driver will never put the device in this situation. 

4) The following, non-normative section recommends what a transitional,
and a non-transitional device should do. There fore I think it would have
been wiser to use should instead of MUST in that section.

> 
> > 
> >   
> > > 
> > > Fortunately, nearly everything worked as intended anyway; the only
> > > problem was not properly rejecting revision setting after some other
> > > command had been issued. Easy to fix by setting revision to 0 if
> > > we see a non-revision command on a legacy-capable revision-less
> > > device.
> > > 
> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>    
> > 
> > The change won't hurt so with a toned down commit message:
> > Acked-by: Halil Pasic <pasic@linux.ibm.com>  
> 
> Replace 'and else' with 'a transitional device needs to'?

Sounds good but, I would also replace 'The virtio standard specifies'
with 'The non-normative part of the virtio specification recommends'
and probably also replace 'MUST' with 'SHOULD'.

The current patch description sounds like, we are in violation of the
spec, and the change is necessary to have a spec conform device, but it
is not.

Regards,
Halil

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Cornelia Huck]

On Wed, 17 Feb 2021 15:39:36 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Tue, 16 Feb 2021 16:54:05 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 16 Feb 2021 15:19:45 +0100
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > On Tue, 16 Feb 2021 12:18:30 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >     
> > > > The virtio standard specifies that any non-transitional device must
> > > > reject commands prior to revision setting (which we do) and else
> > > > assume revision 0 (legacy) if the driver sends a non-revision-setting
> > > > command first. We neglected to do the latter.      
> > > 
> > > Huh, I my opinion, it ain't very clear what is specified by the virtio
> > > standard (which starts with version 1.0) for the described situation.
> > > 
> > > The corresponding device normative section (4.3.2.1.1 Device
> > > Requirements: Setting the Virtio Revision) says that: "A device MUST
> > > treat the revision as unset from the time the associated subchannel has
> > > been enabled until a revision has been successfully set by the driver.
> > > This implies that revisions are not persistent across disabling and
> > > enabling of the associated subchannel.". It doesn't say anything more
> > > about the situation where the first command is not SET_VIRTIO_REV.
> > > 
> > > The section "4.3.2.1.3 Legacy Interfaces: A Note on Setting the Virtio
> > > Revision" which is to my best knowledge not normative, as none of the
> > > legacy-interface stuff is normative, but a mere advice on how to deal
> > > with legacy then says: "A legacy driver will not issue the
> > > CCW_CMD_SET_VIRTIO_REV prior to issuing other virtio-ccw specific
> > > channel commands." ... "A transitional device MUST assume
> > > in this case that the driver is a legacy driver and continue as if the
> > > driver selected revision 0. This implies that the device MUST reject any
> > > command not valid for revision 0, including a subsequent
> > > CCW_CMD_SET_VIRTIO_REV."
> > > 
> > > Do we agree that the legacy interface sections in general, and 4.3.2.1.3
> > > in particular is non-normative?    
> > 
> > IMHO, normative and non-normative are not something that applies to the
> > legacy sections. The legacy sections are supposed to give guidance on
> > how to write transitional devices/drivers that can deal with legacy
> > implementations. If you want to write something that strictly only
> > adheres to normative statements, you have to write a non-transitional
> > device/driver. Legacy support was never an official standard, so
> > 'normative' is meaningless in that context.  
> 
> Exactly, so the legacy stuff is not normative, and strictly speaking not
> included in the standard (i.e. standardized).
> 
> But then I find usage of keywords like MUST in legacy interface sections
> misreading. I believe some Oasis guy complained about keyword usage
> outside of normative sections before.

We can certainly discuss whether we want to change something in the
legacy sections in the spec -- but that's outside the scope of this
patch.

> 
> >   
> > > 
> > > In my opinion the normative 'must threat as unset until set by driver'
> > > and 'if first cmd is not _REV, must continue as if the driver selected
> > > revision 0' is in a slight collision.    
> > 
> > I don't think there's a collision. If we want to accommodate legacy
> > drivers, we have to deal with their lack of revision handling, and
> > therefore treat non-_REV commands as implicitly selecting revision 0.
> > 
> > If we want to implement a non-transitional device, the implicit
> > selection of revision 0 goes away, of course. In fact, I'm currently
> > trying to make legacy support optional for virtio-ccw, so that's why I
> > had been looking at the revision handling :)  
> 
> Do you mean optional like build time configurable in QEMU? I think the
> legacy support is already optional when it comes to the spec.
> 
> Let me explain how do I interpret device compliance with respect to
> virtio revisions and first command is a non-_REV.
> 
> 1) If the first virtio command after the virtio-ccw device is enabled is
> a non-_REV command, the virtio-ccw device not answering it with a
> command reject does not preclude the device form being virtio 1.0
> conform. I.e. this behavior is conform, because does not violate
> any of the sections linked in "7.3.3 Clause 17: Channel I/O Device
> Conformance" in general, and thus does not violate "4.3.2.1.1 Device
> Requirements: Setting the Virtio Revision" in particular. If you disagree,
> please point me to the corresponding device normative section.
> 
> 2) Rejecting the first command which which happens to be a non-_REV
> however does not preclude virtio (1.0) conformance either. The device
> is free to do whatever, because in my reading it ain't specified what
> needs to be done.

If it does that, however, it would be a pretty useless transitional
device, as a legacy driver won't be able to work.

> 
> 3) It is OK-ish, that the device is free to do anything there, because
> a virtio 1.0 conform driver will never put the device in this situation. 
> 
> 4) The following, non-normative section recommends what a transitional,
> and a non-transitional device should do. There fore I think it would have
> been wiser to use should instead of MUST in that section.
> 
> >   
> > > 
> > >     
> > > > 
> > > > Fortunately, nearly everything worked as intended anyway; the only
> > > > problem was not properly rejecting revision setting after some other
> > > > command had been issued. Easy to fix by setting revision to 0 if
> > > > we see a non-revision command on a legacy-capable revision-less
> > > > device.
> > > > 
> > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>      
> > > 
> > > The change won't hurt so with a toned down commit message:
> > > Acked-by: Halil Pasic <pasic@linux.ibm.com>    
> > 
> > Replace 'and else' with 'a transitional device needs to'?  
> 
> Sounds good but, I would also replace 'The virtio standard specifies'
> with 'The non-normative part of the virtio specification recommends'
> and probably also replace 'MUST' with 'SHOULD'.
> 
> The current patch description sounds like, we are in violation of the
> spec, and the change is necessary to have a spec conform device, but it
> is not.

I think you're reading too much into this patch description. The point
of the legacy sections in the spec is to lay down what a device/driver
needs to do if it also wants to support legacy drivers/devices. If we
want to present a useful transitional device, we need (regardless of
any MUST, or SHOULD) to operate in a way that a legacy driver can use
it.

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Halil Pasic]

On Thu, 18 Feb 2021 14:51:09 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 17 Feb 2021 15:39:36 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Tue, 16 Feb 2021 16:54:05 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Tue, 16 Feb 2021 15:19:45 +0100
[..]
> > 
> > Exactly, so the legacy stuff is not normative, and strictly speaking not
> > included in the standard (i.e. standardized).
> > 
> > But then I find usage of keywords like MUST in legacy interface sections
> > misreading. I believe some Oasis guy complained about keyword usage
> > outside of normative sections before.  
> 
> We can certainly discuss whether we want to change something in the
> legacy sections in the spec -- but that's outside the scope of this
> patch.
>

I agree. I just pointed out that those requirements are non-normative
despite the fact that MUST is used.

[..]

> > > If we want to implement a non-transitional device, the implicit
> > > selection of revision 0 goes away, of course. In fact, I'm currently
> > > trying to make legacy support optional for virtio-ccw, so that's why I
> > > had been looking at the revision handling :)    
> > 
> > Do you mean optional like build time configurable in QEMU? I think the
> > legacy support is already optional when it comes to the spec.
> > 
> > Let me explain how do I interpret device compliance with respect to
> > virtio revisions and first command is a non-_REV.
> > 
> > 1) If the first virtio command after the virtio-ccw device is enabled is
> > a non-_REV command, the virtio-ccw device not answering it with a
> > command reject does not preclude the device form being virtio 1.0
> > conform. I.e. this behavior is conform, because does not violate
> > any of the sections linked in "7.3.3 Clause 17: Channel I/O Device
> > Conformance" in general, and thus does not violate "4.3.2.1.1 Device
> > Requirements: Setting the Virtio Revision" in particular. If you disagree,
> > please point me to the corresponding device normative section.
> > 
> > 2) Rejecting the first command which which happens to be a non-_REV
> > however does not preclude virtio (1.0) conformance either. The device
> > is free to do whatever, because in my reading it ain't specified what
> > needs to be done.  
> 
> If it does that, however, it would be a pretty useless transitional
> device, as a legacy driver won't be able to work.
> 

Clear, a transitional device can't do that.

[..]

> > > 
> > > Replace 'and else' with 'a transitional device needs to'?    
> > 
> > Sounds good but, I would also replace 'The virtio standard specifies'
> > with 'The non-normative part of the virtio specification recommends'
> > and probably also replace 'MUST' with 'SHOULD'.
> > 
> > The current patch description sounds like, we are in violation of the
> > spec, and the change is necessary to have a spec conform device, but it
> > is not.  
> 
> I think you're reading too much into this patch description. The point
> of the legacy sections in the spec is to lay down what a device/driver
> needs to do if it also wants to support legacy drivers/devices. If we
> want to present a useful transitional device, we need (regardless of
> any MUST, or SHOULD) to operate in a way that a legacy driver can use
> it.
> 

I agree. And I was never disputing that. What I don't like is that the
patch description reads to me like a conformance fix, but it is not. The
QEMU implementation was VIRTIO spec conform before, and remains VIRTIO
spec conform later. Are you saying that a construct: 'The X
standard specifies that Y must Z, but our implementation does not Z.' does
not imply that our our implementation of Y is not conform to standard X?

In any case, the commit message ain't worth all this discussion. It
won't be the first or the last one that is not very precise. Please do
whatever you like.

Regards,
Halil

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Cornelia Huck]

On Tue, 16 Feb 2021 12:18:30 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> The virtio standard specifies that any non-transitional device must
> reject commands prior to revision setting (which we do) and else
> assume revision 0 (legacy) if the driver sends a non-revision-setting
> command first. We neglected to do the latter.
> 
> Fortunately, nearly everything worked as intended anyway; the only
> problem was not properly rejecting revision setting after some other
> command had been issued. Easy to fix by setting revision to 0 if
> we see a non-revision command on a legacy-capable revision-less
> device.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)

I now have:

Author: Cornelia Huck <cohuck@redhat.com>
Date:   Tue Feb 16 12:18:30 2021 +0100

    virtio-ccw: commands on revision-less devices
    
    The virtio standard specifies that any non-transitional device must
    reject commands prior to revision setting (which we do). Devices
    that are transitional need to assume revision 0 (legacy) if the
    driver sends a non-revision-setting command first in order to
    support legacy drivers. We neglected to do the latter.
    
    Fortunately, nearly everything worked as intended anyway; the only
    problem was not properly rejecting revision setting after some other
    command had been issued. Easy to fix by setting revision to 0 if
    we see a non-revision command on a legacy-capable revision-less
    device.
    
    Found by code inspection, not observed in the wild.
    
    Signed-off-by: Cornelia Huck <cohuck@redhat.com>
    Reviewed-by: Thomas Huth <thuth@redhat.com>
    Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
    Acked-by: Halil Pasic <pasic@linux.ibm.com>
    Message-Id: <20210216111830.1087847-1-cohuck@redhat.com>

Any objections?

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Cornelia Huck]

On Fri, 19 Feb 2021 12:21:36 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 16 Feb 2021 12:18:30 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > The virtio standard specifies that any non-transitional device must
> > reject commands prior to revision setting (which we do) and else
> > assume revision 0 (legacy) if the driver sends a non-revision-setting
> > command first. We neglected to do the latter.
> > 
> > Fortunately, nearly everything worked as intended anyway; the only
> > problem was not properly rejecting revision setting after some other
> > command had been issued. Easy to fix by setting revision to 0 if
> > we see a non-revision command on a legacy-capable revision-less
> > device.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/virtio-ccw.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)  
> 
> I now have:
> 
> Author: Cornelia Huck <cohuck@redhat.com>
> Date:   Tue Feb 16 12:18:30 2021 +0100
> 
>     virtio-ccw: commands on revision-less devices
>     
>     The virtio standard specifies that any non-transitional device must
>     reject commands prior to revision setting (which we do). Devices
>     that are transitional need to assume revision 0 (legacy) if the
>     driver sends a non-revision-setting command first in order to
>     support legacy drivers. We neglected to do the latter.
>     
>     Fortunately, nearly everything worked as intended anyway; the only
>     problem was not properly rejecting revision setting after some other
>     command had been issued. Easy to fix by setting revision to 0 if
>     we see a non-revision command on a legacy-capable revision-less
>     device.
>     
>     Found by code inspection, not observed in the wild.
>     
>     Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>     Reviewed-by: Thomas Huth <thuth@redhat.com>
>     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>     Acked-by: Halil Pasic <pasic@linux.ibm.com>
>     Message-Id: <20210216111830.1087847-1-cohuck@redhat.com>
> 
> Any objections?

Queued now with this description.

Re: [PATCH] virtio-ccw: commands on revision-less devices

[Michael S. Tsirkin]

On Fri, Feb 19, 2021 at 12:21:36PM +0100, Cornelia Huck wrote:
> On Tue, 16 Feb 2021 12:18:30 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > The virtio standard specifies that any non-transitional device must
> > reject commands prior to revision setting (which we do) and else
> > assume revision 0 (legacy) if the driver sends a non-revision-setting
> > command first. We neglected to do the latter.
> > 
> > Fortunately, nearly everything worked as intended anyway; the only
> > problem was not properly rejecting revision setting after some other
> > command had been issued. Easy to fix by setting revision to 0 if
> > we see a non-revision command on a legacy-capable revision-less
> > device.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  hw/s390x/virtio-ccw.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> I now have:
> 
> Author: Cornelia Huck <cohuck@redhat.com>
> Date:   Tue Feb 16 12:18:30 2021 +0100
> 
>     virtio-ccw: commands on revision-less devices
>     
>     The virtio standard specifies that any non-transitional device must
>     reject commands prior to revision setting (which we do). Devices
>     that are transitional need to assume revision 0 (legacy) if the
>     driver sends a non-revision-setting command first in order to
>     support legacy drivers. We neglected to do the latter.
>     
>     Fortunately, nearly everything worked as intended anyway; the only
>     problem was not properly rejecting revision setting after some other
>     command had been issued. Easy to fix by setting revision to 0 if
>     we see a non-revision command on a legacy-capable revision-less
>     device.
>     
>     Found by code inspection, not observed in the wild.
>     
>     Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>     Reviewed-by: Thomas Huth <thuth@redhat.com>
>     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>     Acked-by: Halil Pasic <pasic@linux.ibm.com>
>     Message-Id: <20210216111830.1087847-1-cohuck@redhat.com>
> 
> Any objections?

Acked-by: Michael S. Tsirkin <mst@redhat.com>