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. */  
>