Linux Virtualization - Nov 2014 - [PATCH v4 04/42] virtio: disable virtio 1.0 in transports

On Tue, Nov 25, 2014 at 06:29:42PM +0100, Cornelia Huck
wrote:
> On Tue, 25 Nov 2014 18:41:35 +0200
> "Michael S. Tsirkin" <mst at redhat.com> wrote:
> 
> > disable virtio 1.0 in transports that don't
> > support it yet.
> 
> I'd prefer if you disabled it for _every_ transport in this patch,
> until the needed infrastructure is in place. Else this is a bit
> confusing.
Well the only transports left are pci and rpoc, and these only
read the low 32 bit of the features from the device -
so there's nothing to clear.

E.g. the following would be even more confusing, would it not:

	u32 features;
	....
	features &= ~BIT_ULL(VIRTIO_F_VERSION_1);

Agree?
> > We will gradually re-enable as support is added.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> > ---
> >  drivers/lguest/lguest_device.c     | 3 ++-
> >  drivers/misc/mic/card/mic_virtio.c | 2 ++
> >  drivers/s390/kvm/virtio_ccw.c      | 3 ++-
> >  drivers/virtio/virtio_mmio.c       | 2 ++
> >  4 files changed, 8 insertions(+), 2 deletions(-)
> 
> Why do you disable ccw but not pci? (Doesn't pci need any changes
> transport-side?)
No because the register is 32 bit wide there.
> And you missed the old s390 virtio transport in
> drivers/s390/kvm/kvm_virtio.c :)
Good catch. I can fix that for v5, but see below.
> > diff --git a/drivers/s390/kvm/virtio_ccw.c
b/drivers/s390/kvm/virtio_ccw.c
> > index abba04d..08536f0 100644
> > --- a/drivers/s390/kvm/virtio_ccw.c
> > +++ b/drivers/s390/kvm/virtio_ccw.c
> > @@ -704,7 +704,8 @@ static u64 virtio_ccw_get_features(struct
virtio_device *vdev)
> >  out_free:
> >  	kfree(features);
> >  	kfree(ccw);
> > -	return rc;
> > +	/* TODO: enable virtio 1.0 */
> > +	return rc & ~BIT_ULL(VIRTIO_F_VERSION_1);;
> 
> double ';'
> 
> FWIW, as negotiating a revision >= 1 is a pre-req for virtio 1.0
> support on ccw, virtio 1.0 is already implicitly disabled.
Ah, you mean device guarantees that VIRTIO_F_VERSION_1 isn't set
if guest sets revision to 0?
In that case it's probably best to drop this from both ccw
devices.

It is here temporarily anyway, only in order to avoid
using transitional devices incorrectly when bisecting.

> >  }
> > 
> >  static void virtio_ccw_finalize_features(struct virtio_device *vdev)

On Tue, 25 Nov 2014 23:20:11 +0200
"Michael S. Tsirkin" <mst at redhat.com> wrote:
> On Tue, Nov 25, 2014 at 06:29:42PM +0100, Cornelia Huck wrote:
> > On Tue, 25 Nov 2014 18:41:35 +0200
> > "Michael S. Tsirkin" <mst at redhat.com> wrote:
> > 
> > > disable virtio 1.0 in transports that don't
> > > support it yet.
> > 
> > I'd prefer if you disabled it for _every_ transport in this patch,
> > until the needed infrastructure is in place. Else this is a bit
> > confusing.
> 
> Well the only transports left are pci and rpoc, and these only
> read the low 32 bit of the features from the device -
> so there's nothing to clear.
> 
> E.g. the following would be even more confusing, would it not:
> 
> 	u32 features;
> 	....
> 	features &= ~BIT_ULL(VIRTIO_F_VERSION_1);
> 
> Agree?
Maybe you should tweak the patch description a bit and mention that you
only disable virtio 1.0 for transports where it is actually needed?

(...)
> > FWIW, as negotiating a revision >= 1 is a pre-req for virtio 1.0
> > support on ccw, virtio 1.0 is already implicitly disabled.
> 
> Ah, you mean device guarantees that VIRTIO_F_VERSION_1 isn't set
> if guest sets revision to 0?
Yes, the bit will not be offered if the revision is 0 or has not been
set at all.
> In that case it's probably best to drop this from both ccw
> devices.
There's only one ccw transport :)

The old s390 virtio transport in kvm_virtio.c is not part of virtio 1.0.

On Wed, Nov 26, 2014 at 10:09:54AM +0100, Cornelia Huck
wrote:
> On Tue, 25 Nov 2014 23:20:11 +0200
> "Michael S. Tsirkin" <mst at redhat.com> wrote:
> 
> > On Tue, Nov 25, 2014 at 06:29:42PM +0100, Cornelia Huck wrote:
> > > On Tue, 25 Nov 2014 18:41:35 +0200
> > > "Michael S. Tsirkin" <mst at redhat.com> wrote:
> > > 
> > > > disable virtio 1.0 in transports that don't
> > > > support it yet.
> > > 
> > > I'd prefer if you disabled it for _every_ transport in this
patch,
> > > until the needed infrastructure is in place. Else this is a bit
> > > confusing.
> > 
> > Well the only transports left are pci and rpoc, and these only
> > read the low 32 bit of the features from the device -
> > so there's nothing to clear.
> > 
> > E.g. the following would be even more confusing, would it not:
> > 
> > 	u32 features;
> > 	....
> > 	features &= ~BIT_ULL(VIRTIO_F_VERSION_1);
> > 
> > Agree?
> 
> Maybe you should tweak the patch description a bit and mention that you
> only disable virtio 1.0 for transports where it is actually needed?
> 
> (...)
Yes, I did that now.
> > > FWIW, as negotiating a revision >= 1 is a pre-req for virtio
1.0
> > > support on ccw, virtio 1.0 is already implicitly disabled.
> > 
> > Ah, you mean device guarantees that VIRTIO_F_VERSION_1 isn't set
> > if guest sets revision to 0?
> 
> Yes, the bit will not be offered if the revision is 0 or has not been
> set at all.
> 
> > In that case it's probably best to drop this from both ccw
> > devices.
> 
> There's only one ccw transport :)
> 
> The old s390 virtio transport in kvm_virtio.c is not part of virtio 1.0.
It might or might not be a good idea to add code in kvm_virtio.c
blacklisting VIRTIO_F_VERSION_1, just in case there's a buggy device
that sets it.
As correct devices won't set it, I don't think we need to
worry about it too much. We can make it a patch on top later
if we want.

-- 
MST