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? 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 <coh...@redhat.com> The change won't hurt so with a toned down commit message: Acked-by: Halil Pasic <pa...@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. */