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