| Message ID | 1437544792-3949-3-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 22 Jul 2015 13:59:51 +0800 Jason Wang <jasowang@redhat.com> wrote: > SCSI passthrough was no longer supported in virtio 1.0, so this patch > fail the get_features() when both 1.0 and scsi is set. And also only > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/block/virtio-blk.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 4c27974..4716c3e 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > + if (s->conf.scsi) { > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > + return 0; > + } > + } else { > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > + } > > if (s->conf.config_wce) { > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); Do we advertise F_SCSI even if scsi is not configured in order to keep the same bits as before? I'm afraid I don't remember, that thread was long :/ I'm asking because I'd like to depend on that bit to decide whether I can negotiate revision 1 for ccw and subsequently offer VERSION_1. It would be an easy thing to do, and I'd like to avoid mucking around with device-specific configuration from the transport. To illustrate what I'm talking about, my current patchset for virtio-1 on ccw is here: git://github.com/cohuck/qemu virtio-1-ccw-2.5
On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > On Wed, 22 Jul 2015 13:59:51 +0800 > Jason Wang <jasowang@redhat.com> wrote: > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > fail the get_features() when both 1.0 and scsi is set. And also only > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > --- > > hw/block/virtio-blk.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > index 4c27974..4716c3e 100644 > > --- a/hw/block/virtio-blk.c > > +++ b/hw/block/virtio-blk.c > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > + if (s->conf.scsi) { > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > + return 0; > > + } > > + } else { > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > + } > > > > if (s->conf.config_wce) { > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > Do we advertise F_SCSI even if scsi is not configured in order to keep > the same bits as before? I'm afraid I don't remember, that thread was > long :/ > > I'm asking because I'd like to depend on that bit to decide whether I > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > would be an easy thing to do, and I'd like to avoid mucking around with > device-specific configuration from the transport. > > To illustrate what I'm talking about, my current patchset for virtio-1 > on ccw is here: > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 I still think you are over-engineering it. Just add a property to disable the modern interface. Anyone using scsi passthrough will have to set that, if not - above patch will make initialization fail.
On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote: > SCSI passthrough was no longer supported in virtio 1.0, so this patch > fail the get_features() when both 1.0 and scsi is set. And also only > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > Signed-off-by: Jason Wang <jasowang@redhat.com> > --- > hw/block/virtio-blk.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 4c27974..4716c3e 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > + if (s->conf.scsi) { > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); A bit better: "Virtio modern does not support scsi passthrough - please set disable-modern=on". It might be even better if we could specify the device that failed the initialization, but I don't know how to do that in a way that's helpful to the user. Markus? > + return 0; > + } > + } else { > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > + } > > if (s->conf.config_wce) { > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > -- > 2.1.4
On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote: > SCSI passthrough was no longer supported in virtio 1.0, so this patch > fail the get_features() when both 1.0 and scsi is set. And also only > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. Why is SCSI passthrough support not available in virtio 1.0 ? This will cause a regression for any users of that as & when QEMU changes to use virtio 1.0 by default. Can we not fix this regression instead. Regards, Daniel
On 07/22/2015 04:58 PM, Cornelia Huck wrote: > On Wed, 22 Jul 2015 13:59:51 +0800 > Jason Wang <jasowang@redhat.com> wrote: > >> SCSI passthrough was no longer supported in virtio 1.0, so this patch >> fail the get_features() when both 1.0 and scsi is set. And also only >> advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> hw/block/virtio-blk.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index 4c27974..4716c3e 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, >> virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); >> virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); >> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); >> - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); >> + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { >> + if (s->conf.scsi) { >> + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); >> + return 0; >> + } >> + } else { >> + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); >> + } >> >> if (s->conf.config_wce) { >> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > Do we advertise F_SCSI even if scsi is not configured in order to keep > the same bits as before? I'm afraid I don't remember, that thread was > long :/ > > I'm asking because I'd like to depend on that bit to decide whether I > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > would be an easy thing to do, and I'd like to avoid mucking around with > device-specific configuration from the transport. I don't see much difference. Both of our patches needs to set scsi to off to have 1.0 device. And I don't see advantages that did it from transport. If it has, we probably need something similar in virtio-pci. > > To illustrate what I'm talking about, my current patchset for virtio-1 > on ccw is here: > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > Looks like patch "virtio-blk: scsi is legacy only" breaks backward compatibility because VIRTIO_BLK_F_SCSI was not notified when scsi is off. Thanks
On 07/22/2015 05:31 PM, Daniel P. Berrange wrote: > On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote: >> SCSI passthrough was no longer supported in virtio 1.0, so this patch >> fail the get_features() when both 1.0 and scsi is set. And also only >> advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > Why is SCSI passthrough support not available in virtio 1.0 ? This > will cause a regression for any users of that as & when QEMU changes > to use virtio 1.0 by default. Can we not fix this regression instead. > > Regards, > Daniel I can find some discussion here: http://comments.gmane.org/gmane.comp.emulators.virtio.devel/865 Paolo may know more about this.
On 07/22/2015 05:25 PM, Michael S. Tsirkin wrote: > On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote: >> SCSI passthrough was no longer supported in virtio 1.0, so this patch >> fail the get_features() when both 1.0 and scsi is set. And also only >> advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. >> >> Signed-off-by: Jason Wang <jasowang@redhat.com> >> --- >> hw/block/virtio-blk.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c >> index 4c27974..4716c3e 100644 >> --- a/hw/block/virtio-blk.c >> +++ b/hw/block/virtio-blk.c >> @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, >> virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); >> virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); >> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); >> - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); >> + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { >> + if (s->conf.scsi) { >> + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > A bit better: > "Virtio modern does not support scsi passthrough - please set disable-modern=on". > > It might be even better if we could specify the device that failed the > initialization, but I don't know how to do that in a way that's > helpful to the user. Looks like current error prompt has contained the device? qemu-system-x86_64: -device virtio-blk-pci,id=v0,scsi=on,disable-modern=off,drive=img1: Virtio 1.0 does not support scsi passthrough! > > Markus? > > >> + return 0; >> + } >> + } else { >> + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); >> + } >> >> if (s->conf.config_wce) { >> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); >> -- >> 2.1.4
On Wed, Jul 22, 2015 at 05:52:29PM +0800, Jason Wang wrote: > > > On 07/22/2015 05:25 PM, Michael S. Tsirkin wrote: > > On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote: > >> SCSI passthrough was no longer supported in virtio 1.0, so this patch > >> fail the get_features() when both 1.0 and scsi is set. And also only > >> advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > >> > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > >> --- > >> hw/block/virtio-blk.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > >> index 4c27974..4716c3e 100644 > >> --- a/hw/block/virtio-blk.c > >> +++ b/hw/block/virtio-blk.c > >> @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > >> virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > >> virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > >> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > >> - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > >> + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > >> + if (s->conf.scsi) { > >> + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > A bit better: > > "Virtio modern does not support scsi passthrough - please set disable-modern=on". > > > > It might be even better if we could specify the device that failed the > > initialization, but I don't know how to do that in a way that's > > helpful to the user. > > Looks like current error prompt has contained the device? > > qemu-system-x86_64: -device > virtio-blk-pci,id=v0,scsi=on,disable-modern=off,drive=img1: Virtio 1.0 > does not support scsi passthrough! Cool, then the message I suggested will contain all the necessary information. > > > > Markus? > > > > > >> + return 0; > >> + } > >> + } else { > >> + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > >> + } > >> > >> if (s->conf.config_wce) { > >> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > >> -- > >> 2.1.4
On Wed, Jul 22, 2015 at 01:12:45PM +0300, Michael S. Tsirkin wrote: > On Wed, Jul 22, 2015 at 05:52:29PM +0800, Jason Wang wrote: > > > > > > On 07/22/2015 05:25 PM, Michael S. Tsirkin wrote: > > > On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote: > > >> SCSI passthrough was no longer supported in virtio 1.0, so this patch > > >> fail the get_features() when both 1.0 and scsi is set. And also only > > >> advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > >> > > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > >> --- > > >> hw/block/virtio-blk.c | 9 ++++++++- > > >> 1 file changed, 8 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > >> index 4c27974..4716c3e 100644 > > >> --- a/hw/block/virtio-blk.c > > >> +++ b/hw/block/virtio-blk.c > > >> @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > >> virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > >> virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > >> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > >> - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > >> + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > >> + if (s->conf.scsi) { > > >> + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > A bit better: > > > "Virtio modern does not support scsi passthrough - please set disable-modern=on". > > > > > > It might be even better if we could specify the device that failed the > > > initialization, but I don't know how to do that in a way that's > > > helpful to the user. > > > > Looks like current error prompt has contained the device? > > > > qemu-system-x86_64: -device > > virtio-blk-pci,id=v0,scsi=on,disable-modern=off,drive=img1: Virtio 1.0 > > does not support scsi passthrough! > > Cool, then the message I suggested will contain all the > necessary information. Maybe add "or switch to virtio-scsi". > > > > > > Markus? > > > > > > > > >> + return 0; > > >> + } > > >> + } else { > > >> + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > >> + } > > >> > > >> if (s->conf.config_wce) { > > >> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > >> -- > > >> 2.1.4
On Wed, Jul 22, 2015 at 10:31:45AM +0100, Daniel P. Berrange wrote: > On Wed, Jul 22, 2015 at 01:59:51PM +0800, Jason Wang wrote: > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > fail the get_features() when both 1.0 and scsi is set. And also only > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > Why is SCSI passthrough support not available in virtio 1.0 ? This > will cause a regression for any users of that as & when QEMU changes > to use virtio 1.0 by default. Can we not fix this regression instead. > > Regards, > Daniel If we wanted to, we might be able to fix this but not for 2.4: we'd have to extend the spec and guest drivers, in some way TBD. Paolo would be best placed to answer whether this feature is desirable in the future, I think the argument made when the spec was written was that the feature is not widely used, and virtio scsi is available as a replacement for people who need it. > -- > |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| > |: http://libvirt.org -o- http://virt-manager.org :| > |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| > |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, 22 Jul 2015 17:35:07 +0800 Jason Wang <jasowang@redhat.com> wrote: > > > On 07/22/2015 04:58 PM, Cornelia Huck wrote: > > On Wed, 22 Jul 2015 13:59:51 +0800 > > Jason Wang <jasowang@redhat.com> wrote: > > > >> SCSI passthrough was no longer supported in virtio 1.0, so this patch > >> fail the get_features() when both 1.0 and scsi is set. And also only > >> advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > >> > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > >> --- > >> hw/block/virtio-blk.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > >> index 4c27974..4716c3e 100644 > >> --- a/hw/block/virtio-blk.c > >> +++ b/hw/block/virtio-blk.c > >> @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > >> virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > >> virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > >> virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > >> - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > >> + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > >> + if (s->conf.scsi) { > >> + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > >> + return 0; > >> + } > >> + } else { > >> + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > >> + } > >> > >> if (s->conf.config_wce) { > >> virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > the same bits as before? I'm afraid I don't remember, that thread was > > long :/ > > > > I'm asking because I'd like to depend on that bit to decide whether I > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > would be an easy thing to do, and I'd like to avoid mucking around with > > device-specific configuration from the transport. > > I don't see much difference. Both of our patches needs to set scsi to > off to have 1.0 device. And I don't see advantages that did it from > transport. If it has, we probably need something similar in virtio-pci. The crux of the problem seems to be that pci and ccw have different approaches on supporting legacy and modern devices... > > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > on ccw is here: > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > > Looks like patch "virtio-blk: scsi is legacy only" breaks backward > compatibility because VIRTIO_BLK_F_SCSI was not notified when scsi is off. Yeah, that's what I feared.
On Wed, 22 Jul 2015 12:21:32 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > > On Wed, 22 Jul 2015 13:59:51 +0800 > > Jason Wang <jasowang@redhat.com> wrote: > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > --- > > > hw/block/virtio-blk.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > index 4c27974..4716c3e 100644 > > > --- a/hw/block/virtio-blk.c > > > +++ b/hw/block/virtio-blk.c > > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > + if (s->conf.scsi) { > > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > + return 0; > > > + } > > > + } else { > > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > + } > > > > > > if (s->conf.config_wce) { > > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > the same bits as before? I'm afraid I don't remember, that thread was > > long :/ > > > > I'm asking because I'd like to depend on that bit to decide whether I > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > would be an easy thing to do, and I'd like to avoid mucking around with > > device-specific configuration from the transport. > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > on ccw is here: > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > I still think you are over-engineering it. > > Just add a property to disable the modern interface. > Anyone using scsi passthrough will have to set that, > if not - above patch will make initialization fail. And I still think requiring user action and not having this work transparently is a bad idea... Moreover, I will need a revision-fencing mechanism in any case, when we introduce further revisions.
On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote: > On Wed, 22 Jul 2015 12:21:32 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > > > On Wed, 22 Jul 2015 13:59:51 +0800 > > > Jason Wang <jasowang@redhat.com> wrote: > > > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > --- > > > > hw/block/virtio-blk.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > index 4c27974..4716c3e 100644 > > > > --- a/hw/block/virtio-blk.c > > > > +++ b/hw/block/virtio-blk.c > > > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > > + if (s->conf.scsi) { > > > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > > + return 0; > > > > + } > > > > + } else { > > > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > + } > > > > > > > > if (s->conf.config_wce) { > > > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > > the same bits as before? I'm afraid I don't remember, that thread was > > > long :/ > > > > > > I'm asking because I'd like to depend on that bit to decide whether I > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > > would be an easy thing to do, and I'd like to avoid mucking around with > > > device-specific configuration from the transport. > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > > on ccw is here: > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > > > > I still think you are over-engineering it. > > > > Just add a property to disable the modern interface. > > Anyone using scsi passthrough will have to set that, > > if not - above patch will make initialization fail. > > And I still think requiring user action and not having this work > transparently is a bad idea... Having what work transparently? SCSI passthrough? Look, either you agree with Paolo or not. Paolo thinks it's a deprecated hack not really worth supporting long term. If you agree, I don't see why is asking for an extra property such a bit deal. If you don't agree - please open a new thread and argue about that. > Moreover, I will need a revision-fencing mechanism in any case, when we > introduce further revisions. Why? Assuming we drop more features in the future?
On Wed, 22 Jul 2015 13:32:17 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote: > > On Wed, 22 Jul 2015 12:21:32 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > > > > On Wed, 22 Jul 2015 13:59:51 +0800 > > > > Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > --- > > > > > hw/block/virtio-blk.c | 9 ++++++++- > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > index 4c27974..4716c3e 100644 > > > > > --- a/hw/block/virtio-blk.c > > > > > +++ b/hw/block/virtio-blk.c > > > > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > > > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > > > + if (s->conf.scsi) { > > > > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > > > + return 0; > > > > > + } > > > > > + } else { > > > > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > + } > > > > > > > > > > if (s->conf.config_wce) { > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > > > the same bits as before? I'm afraid I don't remember, that thread was > > > > long :/ > > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > > > would be an easy thing to do, and I'd like to avoid mucking around with > > > > device-specific configuration from the transport. > > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > > > on ccw is here: > > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > > > > > > > I still think you are over-engineering it. > > > > > > Just add a property to disable the modern interface. > > > Anyone using scsi passthrough will have to set that, > > > if not - above patch will make initialization fail. > > > > And I still think requiring user action and not having this work > > transparently is a bad idea... > > Having what work transparently? SCSI passthrough? > Look, either you agree with Paolo or not. > Paolo thinks it's a deprecated hack not really worth supporting long term. > If you agree, I don't see why is asking for an extra property > such a bit deal. If you don't agree - please open a new thread > and argue about that. I sometimes wonder whether we're arguing about the same thing :( Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is not. If I upgrade the host, I want the guests to be able to continue using scsi without needing to fence virtio-1 off manually. > > > > Moreover, I will need a revision-fencing mechanism in any case, when we > > introduce further revisions. > > Why? Assuming we drop more features in the future? Revisions != features. Think new or changed channel commands, for example.
On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote: > On Wed, 22 Jul 2015 13:32:17 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote: > > > On Wed, 22 Jul 2015 12:21:32 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > > > > > On Wed, 22 Jul 2015 13:59:51 +0800 > > > > > Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > --- > > > > > > hw/block/virtio-blk.c | 9 ++++++++- > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > > index 4c27974..4716c3e 100644 > > > > > > --- a/hw/block/virtio-blk.c > > > > > > +++ b/hw/block/virtio-blk.c > > > > > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > > > > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > > > > + if (s->conf.scsi) { > > > > > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > > > > + return 0; > > > > > > + } > > > > > > + } else { > > > > > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > + } > > > > > > > > > > > > if (s->conf.config_wce) { > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > > > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > > > > the same bits as before? I'm afraid I don't remember, that thread was > > > > > long :/ > > > > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > > > > would be an easy thing to do, and I'd like to avoid mucking around with > > > > > device-specific configuration from the transport. > > > > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > > > > on ccw is here: > > > > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > > > > > > > > > > I still think you are over-engineering it. > > > > > > > > Just add a property to disable the modern interface. > > > > Anyone using scsi passthrough will have to set that, > > > > if not - above patch will make initialization fail. > > > > > > And I still think requiring user action and not having this work > > > transparently is a bad idea... > > > > Having what work transparently? SCSI passthrough? > > Look, either you agree with Paolo or not. > > Paolo thinks it's a deprecated hack not really worth supporting long term. > > If you agree, I don't see why is asking for an extra property > > such a bit deal. If you don't agree - please open a new thread > > and argue about that. > > I sometimes wonder whether we're arguing about the same thing :( > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is > not. If I upgrade the host, I want the guests to be able to continue > using scsi without needing to fence virtio-1 off manually. Paolo's argument is that no one should be using scsi passthrough. If the feature has users, we should bring it back into virtio 1. If almost one uses it, then no one will suffer too much from getting an error message saying "please set disable-modern=on". And there's no reason to make it behave differently between ccw and pci. > > > > > > > Moreover, I will need a revision-fencing mechanism in any case, when we > > > introduce further revisions. > > > > Why? Assuming we drop more features in the future? > > Revisions != features. Think new or changed channel commands, for > example. You likely can just add these unconditionally. How is this related to virtio blk at all?
On Wed, 22 Jul 2015 13:44:14 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote: > > On Wed, 22 Jul 2015 13:32:17 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote: > > > > On Wed, 22 Jul 2015 12:21:32 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > > > > > > On Wed, 22 Jul 2015 13:59:51 +0800 > > > > > > Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > --- > > > > > > > hw/block/virtio-blk.c | 9 ++++++++- > > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > > > index 4c27974..4716c3e 100644 > > > > > > > --- a/hw/block/virtio-blk.c > > > > > > > +++ b/hw/block/virtio-blk.c > > > > > > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > > > > > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > > > > > + if (s->conf.scsi) { > > > > > > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > > > > > + return 0; > > > > > > > + } > > > > > > > + } else { > > > > > > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > + } > > > > > > > > > > > > > > if (s->conf.config_wce) { > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > > > > > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > > > > > the same bits as before? I'm afraid I don't remember, that thread was > > > > > > long :/ > > > > > > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I > > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > > > > > would be an easy thing to do, and I'd like to avoid mucking around with > > > > > > device-specific configuration from the transport. > > > > > > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > > > > > on ccw is here: > > > > > > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > > > > > > > > > > > > > I still think you are over-engineering it. > > > > > > > > > > Just add a property to disable the modern interface. > > > > > Anyone using scsi passthrough will have to set that, > > > > > if not - above patch will make initialization fail. > > > > > > > > And I still think requiring user action and not having this work > > > > transparently is a bad idea... > > > > > > Having what work transparently? SCSI passthrough? > > > Look, either you agree with Paolo or not. > > > Paolo thinks it's a deprecated hack not really worth supporting long term. > > > If you agree, I don't see why is asking for an extra property > > > such a bit deal. If you don't agree - please open a new thread > > > and argue about that. > > > > I sometimes wonder whether we're arguing about the same thing :( > > > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is > > not. If I upgrade the host, I want the guests to be able to continue > > using scsi without needing to fence virtio-1 off manually. > > Paolo's argument is that no one should be using scsi passthrough. > > If the feature has users, we should bring it back into virtio 1. > > If almost one uses it, then no one will suffer too much from getting > an error message saying "please set disable-modern=on". And here's where we disagree. Even if it's exotic, I don't want to break existing users. > And there's no reason to make it behave differently > between ccw and pci. > > > > > > > > > > > > Moreover, I will need a revision-fencing mechanism in any case, when we > > > > introduce further revisions. > > > > > > Why? Assuming we drop more features in the future? > > > > Revisions != features. Think new or changed channel commands, for > > example. > > You likely can just add these unconditionally. Backwards compatibility? > How is this related to virtio blk at all? It isn't, revision handling is generic. It's just the scsi bit that triggered it.
On 22/07/2015 12:19, Michael S. Tsirkin wrote: > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > Why is SCSI passthrough support not available in virtio 1.0 ? This > > will cause a regression for any users of that as & when QEMU changes > > to use virtio 1.0 by default. Can we not fix this regression instead. > > If we wanted to, we might be able to fix this but not for 2.4: we'd have > to extend the spec and guest drivers, in some way TBD. > > Paolo would be best placed to answer whether this feature is desirable > in the future, I think the argument made when the spec was written was that > the feature is not widely used, and virtio scsi is available as > a replacement for people who need it. No, the feature is not desirable in the future. There is no reason really not to use virtio-scsi passthrough instead, since virtio-scsi has been out for about 3 years now and is stable. In addition, the implementation would either not be compatible with virtio 0.9, or would be different from everything else in the spec because it requires a particular framing for the buffers. Paolo
On Wed, Jul 22, 2015 at 01:40:25PM +0200, Paolo Bonzini wrote: > > > On 22/07/2015 12:19, Michael S. Tsirkin wrote: > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > Why is SCSI passthrough support not available in virtio 1.0 ? This > > > will cause a regression for any users of that as & when QEMU changes > > > to use virtio 1.0 by default. Can we not fix this regression instead. > > > > If we wanted to, we might be able to fix this but not for 2.4: we'd have > > to extend the spec and guest drivers, in some way TBD. > > > > Paolo would be best placed to answer whether this feature is desirable > > in the future, I think the argument made when the spec was written was that > > the feature is not widely used, and virtio scsi is available as > > a replacement for people who need it. > > No, the feature is not desirable in the future. There is no reason > really not to use virtio-scsi passthrough instead, since virtio-scsi has > been out for about 3 years now and is stable. > > In addition, the implementation would either not be compatible with > virtio 0.9, or would be different from everything else in the spec > because it requires a particular framing for the buffers. IIUC, the SCSI passthrough feature for virtio-blk is enabled by setting the 'scsi=on' property on the virtio-blk device, which is exposed by libvirt with XML: <disk type='block' device='lun'> <driver name='qemu' type='raw'/> <source dev='/dev/sda'/> <target dev='vda' bus='virtio'/> </disk> (For use with virtio-scsi you'd just change the <target> element) So if the guest is using virtio-1.0, then this will now fail to boot, or cause an error from monitor hotplug. This is not too bad, but I'm just wondering if there's anything else we ought to think about doing in libvirt in this situation. Normally we'd try to detect unsupported things upfront so we can report VIR_ERR_CONFIG_UNSUPPORTED, instead of the generic error code VIR_ERR_INTERNAL_ERROR, but perhaps this is sufficiently niche to not worry about it and its fine to just delegate error reporting to QEMU ? Regards, Daniel
On 22/07/2015 13:46, Daniel P. Berrange wrote: > IIUC, the SCSI passthrough feature for virtio-blk is enabled by > setting the 'scsi=on' property on the virtio-blk device, which is > exposed by libvirt with XML: > > <disk type='block' device='lun'> > <driver name='qemu' type='raw'/> > <source dev='/dev/sda'/> > <target dev='vda' bus='virtio'/> > </disk> > > (For use with virtio-scsi you'd just change the <target> element) > > So if the guest is using virtio-1.0, then this will now fail to boot, or > cause an error from monitor hotplug. This is not too bad, but I'm just > wondering if there's anything else we ought to think about doing in libvirt > in this situation. Normally we'd try to detect unsupported things upfront > so we can report VIR_ERR_CONFIG_UNSUPPORTED, instead of the generic error > code VIR_ERR_INTERNAL_ERROR, but perhaps this is sufficiently niche to > not worry about it and its fine to just delegate error reporting to QEMU ? Probably. Note that it will be a long time before the default is changed to 1.0 (if it ever will). Perhaps you can start warning now about <disk type='block' device='lun'>, and suggest using virtio-scsi instead? Paolo
On Wed, Jul 22, 2015 at 12:55:22PM +0200, Cornelia Huck wrote: > On Wed, 22 Jul 2015 13:44:14 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote: > > > On Wed, 22 Jul 2015 13:32:17 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote: > > > > > On Wed, 22 Jul 2015 12:21:32 +0300 > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > > > > > > > On Wed, 22 Jul 2015 13:59:51 +0800 > > > > > > > Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > --- > > > > > > > > hw/block/virtio-blk.c | 9 ++++++++- > > > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > > > > index 4c27974..4716c3e 100644 > > > > > > > > --- a/hw/block/virtio-blk.c > > > > > > > > +++ b/hw/block/virtio-blk.c > > > > > > > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > > > > > > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > > > > > > + if (s->conf.scsi) { > > > > > > > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > > > > > > + return 0; > > > > > > > > + } > > > > > > > > + } else { > > > > > > > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > > + } > > > > > > > > > > > > > > > > if (s->conf.config_wce) { > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > > > > > > > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > > > > > > the same bits as before? I'm afraid I don't remember, that thread was > > > > > > > long :/ > > > > > > > > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I > > > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > > > > > > would be an easy thing to do, and I'd like to avoid mucking around with > > > > > > > device-specific configuration from the transport. > > > > > > > > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > > > > > > on ccw is here: > > > > > > > > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > > > > > > > > > > > > > > > > I still think you are over-engineering it. > > > > > > > > > > > > Just add a property to disable the modern interface. > > > > > > Anyone using scsi passthrough will have to set that, > > > > > > if not - above patch will make initialization fail. > > > > > > > > > > And I still think requiring user action and not having this work > > > > > transparently is a bad idea... > > > > > > > > Having what work transparently? SCSI passthrough? > > > > Look, either you agree with Paolo or not. > > > > Paolo thinks it's a deprecated hack not really worth supporting long term. > > > > If you agree, I don't see why is asking for an extra property > > > > such a bit deal. If you don't agree - please open a new thread > > > > and argue about that. > > > > > > I sometimes wonder whether we're arguing about the same thing :( > > > > > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is > > > not. If I upgrade the host, I want the guests to be able to continue > > > using scsi without needing to fence virtio-1 off manually. > > > > Paolo's argument is that no one should be using scsi passthrough. > > > > If the feature has users, we should bring it back into virtio 1. > > > > If almost one uses it, then no one will suffer too much from getting > > an error message saying "please set disable-modern=on". > > And here's where we disagree. Even if it's exotic, I don't want to > break existing users. You should take this disagreement to the virtio TC. QEMU merely implements what the spec voted by TC says. > > And there's no reason to make it behave differently > > between ccw and pci. > > > > > > > > > > > > > > > > > Moreover, I will need a revision-fencing mechanism in any case, when we > > > > > introduce further revisions. > > > > > > > > Why? Assuming we drop more features in the future? > > > > > > Revisions != features. Think new or changed channel commands, for > > > example. > > > > You likely can just add these unconditionally. > > Backwards compatibility? Compatibility is built-in to revision negotiation, isn't it? > > How is this related to virtio blk at all? > > It isn't, revision handling is generic. It's just the scsi bit that > triggered it.
On Wed, Jul 22, 2015 at 01:53:14PM +0200, Paolo Bonzini wrote: > > > On 22/07/2015 13:46, Daniel P. Berrange wrote: > > IIUC, the SCSI passthrough feature for virtio-blk is enabled by > > setting the 'scsi=on' property on the virtio-blk device, which is > > exposed by libvirt with XML: > > > > <disk type='block' device='lun'> > > <driver name='qemu' type='raw'/> > > <source dev='/dev/sda'/> > > <target dev='vda' bus='virtio'/> > > </disk> > > > > (For use with virtio-scsi you'd just change the <target> element) > > > > So if the guest is using virtio-1.0, then this will now fail to boot, or > > cause an error from monitor hotplug. This is not too bad, but I'm just > > wondering if there's anything else we ought to think about doing in libvirt > > in this situation. Normally we'd try to detect unsupported things upfront > > so we can report VIR_ERR_CONFIG_UNSUPPORTED, instead of the generic error > > code VIR_ERR_INTERNAL_ERROR, but perhaps this is sufficiently niche to > > not worry about it and its fine to just delegate error reporting to QEMU ? > > Probably. Note that it will be a long time before the default is > changed to 1.0 (if it ever will). I fully expect we'll enable modern by default in 2.5. We'll also disable legacy if the bus used is pcie. We can disable modern if bus is pci and scsi passthrough was requested. > Perhaps you can start warning now > about <disk type='block' device='lun'>, and suggest using virtio-scsi > instead? > > Paolo
On Wed, 22 Jul 2015 17:53:47 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 22, 2015 at 12:55:22PM +0200, Cornelia Huck wrote: > > On Wed, 22 Jul 2015 13:44:14 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote: > > > > On Wed, 22 Jul 2015 13:32:17 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote: > > > > > > On Wed, 22 Jul 2015 12:21:32 +0300 > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > > > > > > > > On Wed, 22 Jul 2015 13:59:51 +0800 > > > > > > > > Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > > > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > > > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > --- > > > > > > > > > hw/block/virtio-blk.c | 9 ++++++++- > > > > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > > > > > index 4c27974..4716c3e 100644 > > > > > > > > > --- a/hw/block/virtio-blk.c > > > > > > > > > +++ b/hw/block/virtio-blk.c > > > > > > > > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > > > > > > > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > > > > > > > + if (s->conf.scsi) { > > > > > > > > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > > > > > > > + return 0; > > > > > > > > > + } > > > > > > > > > + } else { > > > > > > > > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > > > + } > > > > > > > > > > > > > > > > > > if (s->conf.config_wce) { > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > > > > > > > > > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > > > > > > > the same bits as before? I'm afraid I don't remember, that thread was > > > > > > > > long :/ > > > > > > > > > > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I > > > > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > > > > > > > would be an easy thing to do, and I'd like to avoid mucking around with > > > > > > > > device-specific configuration from the transport. > > > > > > > > > > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > > > > > > > on ccw is here: > > > > > > > > > > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > > > > > > > > > > > > > > > > > > > I still think you are over-engineering it. > > > > > > > > > > > > > > Just add a property to disable the modern interface. > > > > > > > Anyone using scsi passthrough will have to set that, > > > > > > > if not - above patch will make initialization fail. > > > > > > > > > > > > And I still think requiring user action and not having this work > > > > > > transparently is a bad idea... > > > > > > > > > > Having what work transparently? SCSI passthrough? > > > > > Look, either you agree with Paolo or not. > > > > > Paolo thinks it's a deprecated hack not really worth supporting long term. > > > > > If you agree, I don't see why is asking for an extra property > > > > > such a bit deal. If you don't agree - please open a new thread > > > > > and argue about that. > > > > > > > > I sometimes wonder whether we're arguing about the same thing :( > > > > > > > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is > > > > not. If I upgrade the host, I want the guests to be able to continue > > > > using scsi without needing to fence virtio-1 off manually. > > > > > > Paolo's argument is that no one should be using scsi passthrough. > > > > > > If the feature has users, we should bring it back into virtio 1. > > > > > > If almost one uses it, then no one will suffer too much from getting > > > an error message saying "please set disable-modern=on". > > > > And here's where we disagree. Even if it's exotic, I don't want to > > break existing users. > > You should take this disagreement to the virtio TC. QEMU merely > implements what the spec voted by TC says. I fail to see why this is a spec issue. It sounds like a qemu implementation question to me. > > > > And there's no reason to make it behave differently > > > between ccw and pci. > > > > > > > > > > > > > > > > > > > > > > Moreover, I will need a revision-fencing mechanism in any case, when we > > > > > > introduce further revisions. > > > > > > > > > > Why? Assuming we drop more features in the future? > > > > > > > > Revisions != features. Think new or changed channel commands, for > > > > example. > > > > > > You likely can just add these unconditionally. > > > > Backwards compatibility? > > Compatibility is built-in to revision negotiation, isn't it? Yes, but we still want to support migration to older releases. > > > > How is this related to virtio blk at all? > > > > It isn't, revision handling is generic. It's just the scsi bit that > > triggered it. >
On Wed, Jul 22, 2015 at 01:40:25PM +0200, Paolo Bonzini wrote: > > > On 22/07/2015 12:19, Michael S. Tsirkin wrote: > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > Why is SCSI passthrough support not available in virtio 1.0 ? This > > > will cause a regression for any users of that as & when QEMU changes > > > to use virtio 1.0 by default. Can we not fix this regression instead. > > > > If we wanted to, we might be able to fix this but not for 2.4: we'd have > > to extend the spec and guest drivers, in some way TBD. > > > > Paolo would be best placed to answer whether this feature is desirable > > in the future, I think the argument made when the spec was written was that > > the feature is not widely used, and virtio scsi is available as > > a replacement for people who need it. > > No, the feature is not desirable in the future. There is no reason > really not to use virtio-scsi passthrough instead, since virtio-scsi has > been out for about 3 years now and is stable. > Given the amount of work we are spending handling this corner case, I'm beginning to think we should just bring it back in. > In addition, the implementation would either not be compatible with > virtio 0.9, or would be different from everything else in the spec > because it requires a particular framing for the buffers. > > Paolo Of course we'll need some kind of change to avoid depending on framing of buffers, but how hard is it? Just stick the header size somewhere in the buffer or in the config space. Not compatible is probably not the right term to use here, since when using the legacy interface we can make the old framing assumption.
On Wed, Jul 22, 2015 at 06:11:16PM +0200, Cornelia Huck wrote: > On Wed, 22 Jul 2015 17:53:47 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jul 22, 2015 at 12:55:22PM +0200, Cornelia Huck wrote: > > > On Wed, 22 Jul 2015 13:44:14 +0300 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote: > > > > > On Wed, 22 Jul 2015 13:32:17 +0300 > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote: > > > > > > > On Wed, 22 Jul 2015 12:21:32 +0300 > > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > > > > > > > > > On Wed, 22 Jul 2015 13:59:51 +0800 > > > > > > > > > Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > > > > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > > > > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > --- > > > > > > > > > > hw/block/virtio-blk.c | 9 ++++++++- > > > > > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > > > > > > index 4c27974..4716c3e 100644 > > > > > > > > > > --- a/hw/block/virtio-blk.c > > > > > > > > > > +++ b/hw/block/virtio-blk.c > > > > > > > > > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > > > > > > > > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > > > > > > > > + if (s->conf.scsi) { > > > > > > > > > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > > > > > > > > + return 0; > > > > > > > > > > + } > > > > > > > > > > + } else { > > > > > > > > > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > > > if (s->conf.config_wce) { > > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > > > > > > > > > > > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > > > > > > > > the same bits as before? I'm afraid I don't remember, that thread was > > > > > > > > > long :/ > > > > > > > > > > > > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I > > > > > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > > > > > > > > would be an easy thing to do, and I'd like to avoid mucking around with > > > > > > > > > device-specific configuration from the transport. > > > > > > > > > > > > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > > > > > > > > on ccw is here: > > > > > > > > > > > > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > > > > > > > > > > > > > > > > > > > > > > I still think you are over-engineering it. > > > > > > > > > > > > > > > > Just add a property to disable the modern interface. > > > > > > > > Anyone using scsi passthrough will have to set that, > > > > > > > > if not - above patch will make initialization fail. > > > > > > > > > > > > > > And I still think requiring user action and not having this work > > > > > > > transparently is a bad idea... > > > > > > > > > > > > Having what work transparently? SCSI passthrough? > > > > > > Look, either you agree with Paolo or not. > > > > > > Paolo thinks it's a deprecated hack not really worth supporting long term. > > > > > > If you agree, I don't see why is asking for an extra property > > > > > > such a bit deal. If you don't agree - please open a new thread > > > > > > and argue about that. > > > > > > > > > > I sometimes wonder whether we're arguing about the same thing :( > > > > > > > > > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is > > > > > not. If I upgrade the host, I want the guests to be able to continue > > > > > using scsi without needing to fence virtio-1 off manually. > > > > > > > > Paolo's argument is that no one should be using scsi passthrough. > > > > > > > > If the feature has users, we should bring it back into virtio 1. > > > > > > > > If almost one uses it, then no one will suffer too much from getting > > > > an error message saying "please set disable-modern=on". > > > > > > And here's where we disagree. Even if it's exotic, I don't want to > > > break existing users. > > > > You should take this disagreement to the virtio TC. QEMU merely > > implements what the spec voted by TC says. > > I fail to see why this is a spec issue. It sounds like a qemu > implementation question to me. Pls re-read the discussion around removing this issue then. virtio 1 should be a super-set of virtio functionality-wise. Paolo said this specific feature has been deprecated for years, no one should be using it, so we dropped it from spec. > > > > > > And there's no reason to make it behave differently > > > > between ccw and pci. > > > > > > > > > > > > > > > > > > > > > > > > > > > Moreover, I will need a revision-fencing mechanism in any case, when we > > > > > > > introduce further revisions. > > > > > > > > > > > > Why? Assuming we drop more features in the future? > > > > > > > > > > Revisions != features. Think new or changed channel commands, for > > > > > example. > > > > > > > > You likely can just add these unconditionally. > > > > > > Backwards compatibility? > > > > Compatibility is built-in to revision negotiation, isn't it? > > Yes, but we still want to support migration to older releases. I don't think you can do anything in virtio to make this work for PPC. As long as you don't version machine types, cross version migration will be broken. > > > > > > How is this related to virtio blk at all? > > > > > > It isn't, revision handling is generic. It's just the scsi bit that > > > triggered it. > >
On 22/07/2015 18:15, Michael S. Tsirkin wrote: > > No, the feature is not desirable in the future. There is no reason > > really not to use virtio-scsi passthrough instead, since virtio-scsi has > > been out for about 3 years now and is stable. > > Given the amount of work we are spending handling this corner case, > I'm beginning to think we should just bring it back in. Please don't. Perhaps it made sense when Rusty was thinking of virtio-blk as simply "struct request over virtio", but I'm not even sure what the use case is. For virtio-scsi, for example, one reason to use passthrough is that you can use the same /dev/disk/by-id path on physical and virtual machines. But that doesn't work with virtio-blk's pseudo passthrough. Another is to pass "weird" devices (tapes, media changers, etc.) to backup appliances, but that also doesn't work with virtio-blk. So what is it used for? That's what is needed to make an informed decision. > > In addition, the implementation would either not be compatible with > > virtio 0.9, or would be different from everything else in the spec > > because it requires a particular framing for the buffers. > > Of course we'll need some kind of change to avoid depending on framing > of buffers, but how hard is it? Just stick the header size > somewhere in the buffer or in the config space. > > Not compatible is probably not the right term to use here, > since when using the legacy interface we can make the old > framing assumption. Not compatible in the sense that it requires work in the drivers too (unlike e.g. CONFIG_WCE, where the old code just works with the proposed modifications to 1.0). Paolo
On Wed, 22 Jul 2015 19:34:31 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 22, 2015 at 06:11:16PM +0200, Cornelia Huck wrote: > > On Wed, 22 Jul 2015 17:53:47 +0300 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Jul 22, 2015 at 12:55:22PM +0200, Cornelia Huck wrote: > > > > On Wed, 22 Jul 2015 13:44:14 +0300 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Wed, Jul 22, 2015 at 12:38:40PM +0200, Cornelia Huck wrote: > > > > > > On Wed, 22 Jul 2015 13:32:17 +0300 > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > > > On Wed, Jul 22, 2015 at 12:25:31PM +0200, Cornelia Huck wrote: > > > > > > > > On Wed, 22 Jul 2015 12:21:32 +0300 > > > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > > On Wed, Jul 22, 2015 at 10:58:43AM +0200, Cornelia Huck wrote: > > > > > > > > > > On Wed, 22 Jul 2015 13:59:51 +0800 > > > > > > > > > > Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > > > SCSI passthrough was no longer supported in virtio 1.0, so this patch > > > > > > > > > > > fail the get_features() when both 1.0 and scsi is set. And also only > > > > > > > > > > > advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > > --- > > > > > > > > > > > hw/block/virtio-blk.c | 9 ++++++++- > > > > > > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > > > > > > > > > > > index 4c27974..4716c3e 100644 > > > > > > > > > > > --- a/hw/block/virtio-blk.c > > > > > > > > > > > +++ b/hw/block/virtio-blk.c > > > > > > > > > > > @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, > > > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); > > > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); > > > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); > > > > > > > > > > > - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > > > > > + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { > > > > > > > > > > > + if (s->conf.scsi) { > > > > > > > > > > > + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); > > > > > > > > > > > + return 0; > > > > > > > > > > > + } > > > > > > > > > > > + } else { > > > > > > > > > > > + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); > > > > > > > > > > > + } > > > > > > > > > > > > > > > > > > > > > > if (s->conf.config_wce) { > > > > > > > > > > > virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE); > > > > > > > > > > > > > > > > > > > > Do we advertise F_SCSI even if scsi is not configured in order to keep > > > > > > > > > > the same bits as before? I'm afraid I don't remember, that thread was > > > > > > > > > > long :/ > > > > > > > > > > > > > > > > > > > > I'm asking because I'd like to depend on that bit to decide whether I > > > > > > > > > > can negotiate revision 1 for ccw and subsequently offer VERSION_1. It > > > > > > > > > > would be an easy thing to do, and I'd like to avoid mucking around with > > > > > > > > > > device-specific configuration from the transport. > > > > > > > > > > > > > > > > > > > > To illustrate what I'm talking about, my current patchset for virtio-1 > > > > > > > > > > on ccw is here: > > > > > > > > > > > > > > > > > > > > git://github.com/cohuck/qemu virtio-1-ccw-2.5 > > > > > > > > > > > > > > > > > > > > > > > > > > > I still think you are over-engineering it. > > > > > > > > > > > > > > > > > > Just add a property to disable the modern interface. > > > > > > > > > Anyone using scsi passthrough will have to set that, > > > > > > > > > if not - above patch will make initialization fail. > > > > > > > > > > > > > > > > And I still think requiring user action and not having this work > > > > > > > > transparently is a bad idea... > > > > > > > > > > > > > > Having what work transparently? SCSI passthrough? > > > > > > > Look, either you agree with Paolo or not. > > > > > > > Paolo thinks it's a deprecated hack not really worth supporting long term. > > > > > > > If you agree, I don't see why is asking for an extra property > > > > > > > such a bit deal. If you don't agree - please open a new thread > > > > > > > and argue about that. > > > > > > > > > > > > I sometimes wonder whether we're arguing about the same thing :( > > > > > > > > > > > > Dropping scsi for virtio-1 is fine. Dropping backwards-compatibility is > > > > > > not. If I upgrade the host, I want the guests to be able to continue > > > > > > using scsi without needing to fence virtio-1 off manually. > > > > > > > > > > Paolo's argument is that no one should be using scsi passthrough. > > > > > > > > > > If the feature has users, we should bring it back into virtio 1. > > > > > > > > > > If almost one uses it, then no one will suffer too much from getting > > > > > an error message saying "please set disable-modern=on". > > > > > > > > And here's where we disagree. Even if it's exotic, I don't want to > > > > break existing users. > > > > > > You should take this disagreement to the virtio TC. QEMU merely > > > implements what the spec voted by TC says. > > > > I fail to see why this is a spec issue. It sounds like a qemu > > implementation question to me. > > Pls re-read the discussion around removing this issue then. > > > virtio 1 should be a super-set of virtio functionality-wise. Perhaps this assumption is the problem then? scsi seems to be the only odd one, but still... > > > Paolo said this specific feature has been deprecated for years, no > one should be using it, so we dropped it from spec. Nothing wrong with that. But "it's deprecated" does not mean "nobody's using it", I fear. The question is: Should qemu accommodate those users? > > > > > > > > > > And there's no reason to make it behave differently > > > > > between ccw and pci. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Moreover, I will need a revision-fencing mechanism in any case, when we > > > > > > > > introduce further revisions. > > > > > > > > > > > > > > Why? Assuming we drop more features in the future? > > > > > > > > > > > > Revisions != features. Think new or changed channel commands, for > > > > > > example. > > > > > > > > > > You likely can just add these unconditionally. > > > > > > > > Backwards compatibility? > > > > > > Compatibility is built-in to revision negotiation, isn't it? > > > > Yes, but we still want to support migration to older releases. > > I don't think you can do anything in virtio to make this work > for PPC. As long as you don't version machine types, cross version > migration will be broken. ccw is s390x :) And we added versioned machines for 2.4, so yes, we care. Which brings me to the next problem: Assuming we want to make blocking VERSION_1 devices user-configurable, we'll need a max_revision property or something like that. (pci's legacy/modern approach just won't cut it, I fear, since we'll need to handle higher revisions for later cross-version migration purposes as well.) This implies we need to add this max_rev field to virtio-ccw's migration stream _now_ (for 2.4). I've started hacking up a patch, but I'm no way finished. So: - checking scsi flag in features for revision fencing won't work (we always need to advertise it for !VERSION_1) - checking if it's a blk device with scsi configured for revision fencing is so ugly that I won't even try to code it - which leaves setting VERSION_1 from the start and have virtio-blk fence VERSION_1 + scsi. For which I need the max_revision property with the issues I outlined above. Not sure how to get out of this pickle.
On 23/07/2015 11:07, Cornelia Huck wrote: > > Paolo said this specific feature has been deprecated for years, no > > one should be using it, so we dropped it from spec. > > Nothing wrong with that. But "it's deprecated" does not mean "nobody's > using it", I fear. The question is: Should qemu accommodate those users? "It's deprecated" does mean (or at least can mean) "it's going to go away in relatively exceptional circumstances". virtio 1 counts as relatively exceptional, I think. Paolo
On Thu, 23 Jul 2015 11:37:44 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 23/07/2015 11:07, Cornelia Huck wrote: > > > Paolo said this specific feature has been deprecated for years, no > > > one should be using it, so we dropped it from spec. > > > > Nothing wrong with that. But "it's deprecated" does not mean "nobody's > > using it", I fear. The question is: Should qemu accommodate those users? > > "It's deprecated" does mean (or at least can mean) "it's going to go > away in relatively exceptional circumstances". virtio 1 counts as > relatively exceptional, I think. OK, if the consensus is "you should not be using it; but if you really need it, you can get it back with a config switch", I won't oppose that.
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 4c27974..4716c3e 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -731,7 +731,14 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features, virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY); virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY); virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE); - virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); + if (__virtio_has_feature(features, VIRTIO_F_VERSION_1)) { + if (s->conf.scsi) { + error_setg(errp, "Virtio 1.0 does not support scsi passthrough!"); + return 0; + } + } else { + virtio_add_feature(&features, VIRTIO_BLK_F_SCSI); + } if (s->conf.config_wce) { virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
SCSI passthrough was no longer supported in virtio 1.0, so this patch fail the get_features() when both 1.0 and scsi is set. And also only advertise VIRTIO_BLK_F_SCSI for legacy virtio-blk device. Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/block/virtio-blk.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)