Virtualization - Feb 2021 - [PATCH] virtio/s390: implement virtio-ccw revision 2 correctly

CCW_CMD_READ_STATUS was introduced with revision 2 of virtio-ccw,
and drivers should only rely on it being implemented when they
negotiated at least that revision with the device.

However, virtio_ccw_get_status() issued READ_STATUS for any
device operating at least at revision 1. If the device accepts
READ_STATUS regardless of the negotiated revision (which it is
free to do), everything works as intended; a device rejecting the
command should also be handled gracefully. For correctness, we
should really limit the command to revision 2 or higher, though.

We also negotiated the revision to at most 1, as we never bumped
the maximum revision; let's do that now.

Fixes: 7d3ce5ab9430 ("virtio/s390: support READ_STATUS command for
virtio-ccw")
Signed-off-by: Cornelia Huck <cohuck at redhat.com>
---

QEMU does not fence off READ_STATUS for revisions < 2, which is probably
why we never noticed this. I'm not aware of other hypervisors that do
fence it off, nor any that cannot deal properly with an unknown command.

Not sure whether this is stable worthy?

---
 drivers/s390/virtio/virtio_ccw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 5730572b52cd..54e686dca6de 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -117,7 +117,7 @@ struct virtio_rev_info {
 };
 
 /* the highest virtio-ccw revision we support */
-#define VIRTIO_CCW_REV_MAX 1
+#define VIRTIO_CCW_REV_MAX 2
 
 struct virtio_ccw_vq_info {
 	struct virtqueue *vq;
@@ -952,7 +952,7 @@ static u8 virtio_ccw_get_status(struct virtio_device *vdev)
 	u8 old_status = vcdev->dma_area->status;
 	struct ccw1 *ccw;
 
-	if (vcdev->revision < 1)
+	if (vcdev->revision < 2)
 		return vcdev->dma_area->status;
 
 	ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));
-- 
2.26.2

On Fri, 12 Feb 2021 18:04:11 +0100
Cornelia Huck <cohuck at redhat.com> wrote:
> CCW_CMD_READ_STATUS was introduced with revision 2 of virtio-ccw,
> and drivers should only rely on it being implemented when they
> negotiated at least that revision with the device.
> 
> However, virtio_ccw_get_status() issued READ_STATUS for any
> device operating at least at revision 1. If the device accepts
> READ_STATUS regardless of the negotiated revision (which it is
> free to do),
So, looking at the standard again, the device is actually required to
reject the READ_STATUS if only rev 1 had been negotiated... regardless
of that, I don't think we should change QEMU's behaviour, as it would
affect existing guests (they would lose access to the status bits as
observed by the device, including DEVICE_NEEDS_RESET.)
> everything works as intended; a device rejecting the
> command should also be handled gracefully. For correctness, we
> should really limit the command to revision 2 or higher, though.
> 
> We also negotiated the revision to at most 1, as we never bumped
> the maximum revision; let's do that now.
> 
> Fixes: 7d3ce5ab9430 ("virtio/s390: support READ_STATUS command for
virtio-ccw")
> Signed-off-by: Cornelia Huck <cohuck at redhat.com>
> ---
> 
> QEMU does not fence off READ_STATUS for revisions < 2, which is probably
> why we never noticed this. I'm not aware of other hypervisors that do
> fence it off, nor any that cannot deal properly with an unknown command.
> 
> Not sure whether this is stable worthy?
Maybe it is, given the MUST reject clause in the standard?
> 
> ---
>  drivers/s390/virtio/virtio_ccw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/virtio/virtio_ccw.c
b/drivers/s390/virtio/virtio_ccw.c
> index 5730572b52cd..54e686dca6de 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -117,7 +117,7 @@ struct virtio_rev_info {
>  };
>  
>  /* the highest virtio-ccw revision we support */
> -#define VIRTIO_CCW_REV_MAX 1
> +#define VIRTIO_CCW_REV_MAX 2
>  
>  struct virtio_ccw_vq_info {
>  	struct virtqueue *vq;
> @@ -952,7 +952,7 @@ static u8 virtio_ccw_get_status(struct virtio_device
*vdev)
>  	u8 old_status = vcdev->dma_area->status;
>  	struct ccw1 *ccw;
>  
> -	if (vcdev->revision < 1)
> +	if (vcdev->revision < 2)
>  		return vcdev->dma_area->status;
>  
>  	ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));