On Tue, 16 Feb 2021 15:19:45 +0100
Halil Pasic <pa...@linux.ibm.com> wrote:
> On Tue, 16 Feb 2021 12:18:30 +0100
> Cornelia Huck <coh...@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 <coh...@redhat.com>
>
> The change won't hurt so with a toned down commit message:
> Acked-by: Halil Pasic <pa...@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. */
>
