Re: [PATCH] virtio/s390: implement virtio-ccw revision 2 correctly

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 15 Feb 2021 19:51:44 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 15 Feb 2021 12:47:02 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri, 12 Feb 2021 18:04:11 +0100
> > Cornelia Huck <cohuck@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.)  
> 
> Not only that, without READ_STATUS, we can't do device reset which
> is a prerequisite for a proper cleanup, as required by the spec.
> 
> You certainly remember, the driver has may not assume the reset
> was performed (and thus virtqueues are not live) until it reads
> back status 0. But without READ_STATUS virtio_ccw_get_status() will
> keep returning the status the driver last set via
> virtio_ccw_set_status(). And CCW_CMD_VDEV_RESET is of course
> revision 1 material. This looks ugly!

Yes, that problem kind of cascades down.

> 
> >   
> > > 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@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?
> >   
> 
> Yes, IMHO this must be backported. A device that ain't violating the
> spec would currently reject READ_STATUS. Which would break RESET_VDEV
> like I described above.
> 
> Can we change that MUST to should? There are now good reasons for not
> doing like the spec says in case of READ_STATUS.

Yes. I'm not so sure forcing the device to reject the command was such
a good idea anyway, and relaxing the requirement keeps existing
implementations in compliance.

I've opened https://github.com/oasis-tcs/virtio-spec/issues/96 and will
send a patch for the spec later.

> 
> > > 
> > > ---
> > >  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;  
> 
> I don't think our faking of the status read (i.e. returning the old one)
> is contributing to spec compliance. Especially not if the inability to
> READ is not transient.
> 
> Also return old_status; would tell the story better, but on the
> other hand, that would be an unrelated cosmetic change. Maybe
> a separate patch?

We would also need to actively check for success or failure of the
channel program in that case.

I'm currently looking at the virtio-ccw code anyway, so I can put that
on my list as well.

> 
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

Thanks!

I'll do a v2 with a tweaked commit message and cc:stable.

> 
> Regards,
> Halil
> 
> > >  
> > >  	ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw));    
> >   
>