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

[Halil Pasic <pasic@linux.ibm.com>]

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