| Message ID | 1436938201-16766-4-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 15, 2015 at 01:29:59PM +0800, Jason Wang wrote: > Disable scsi passthrough by default since it was incompatible with > virtio 1.0. For legacy machine types, keep this on by default. > > Cc: Stefan Hajnoczi <stefanha@redhat.com> > Cc: Kevin Wolf <kwolf@redhat.com> > Cc: qemu-block@nongnu.org > Signed-off-by: Jason Wang <jasowang@redhat.com> Seems risky for 2.4. modern is off by default for now. Can't we limit the change to when modern is enabled? I suggested changing this from bool to on/off/auto, and make auto mean !modern. > --- > hw/block/virtio-blk.c | 2 +- > include/hw/compat.h | 6 +++++- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index 761d763..362fe53 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -964,7 +964,7 @@ static Property virtio_blk_properties[] = { > DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial), > DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true), > #ifdef __linux__ > - DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, true), > + DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, false), > #endif > DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, > true), > diff --git a/include/hw/compat.h b/include/hw/compat.h > index 4a43466..56039d8 100644 > --- a/include/hw/compat.h > +++ b/include/hw/compat.h > @@ -2,7 +2,11 @@ > #define HW_COMPAT_H > > #define HW_COMPAT_2_3 \ > - /* empty */ > + {\ > + .driver = "virtio-blk-pci",\ > + .property = "scsi",\ > + .value = "on",\ > + }, > > #define HW_COMPAT_2_2 \ > /* empty */ > -- > 2.1.4
On 15/07/2015 14:21, Michael S. Tsirkin wrote: >> > Disable scsi passthrough by default since it was incompatible with >> > virtio 1.0. For legacy machine types, keep this on by default. >> > >> > Cc: Stefan Hajnoczi <stefanha@redhat.com> >> > Cc: Kevin Wolf <kwolf@redhat.com> >> > Cc: qemu-block@nongnu.org >> > Signed-off-by: Jason Wang <jasowang@redhat.com> > Seems risky for 2.4. modern is off by default for now. Can't we limit > the change to when modern is enabled? That would have the effect of disabling a feature when you turn on modern. > I suggested changing this from bool to on/off/auto, and > make auto mean !modern. No, please do it like Jason did. The SCSI feature effectively had to be enabled explicitly already, the requests were marked as unsupported. Paolo
On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote: > > > On 15/07/2015 14:21, Michael S. Tsirkin wrote: > >> > Disable scsi passthrough by default since it was incompatible with > >> > virtio 1.0. For legacy machine types, keep this on by default. > >> > > >> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > >> > Cc: Kevin Wolf <kwolf@redhat.com> > >> > Cc: qemu-block@nongnu.org > >> > Signed-off-by: Jason Wang <jasowang@redhat.com> > > Seems risky for 2.4. modern is off by default for now. Can't we limit > > the change to when modern is enabled? > > That would have the effect of disabling a feature when you turn on modern. What's wrong with that? > > I suggested changing this from bool to on/off/auto, and > > make auto mean !modern. > > No, please do it like Jason did. The SCSI feature effectively had to be > enabled explicitly already, the requests were marked as unsupported. > > Paolo I didn't know. How is it enabled?
On 15/07/2015 16:14, Michael S. Tsirkin wrote: > On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote: >> >> >> On 15/07/2015 14:21, Michael S. Tsirkin wrote: >>>>> Disable scsi passthrough by default since it was incompatible with >>>>> virtio 1.0. For legacy machine types, keep this on by default. >>>>> >>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com> >>>>> Cc: Kevin Wolf <kwolf@redhat.com> >>>>> Cc: qemu-block@nongnu.org >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> Seems risky for 2.4. modern is off by default for now. Can't we limit >>> the change to when modern is enabled? >> >> That would have the effect of disabling a feature when you turn on modern. > > What's wrong with that? Weren't you complaining about it a few hours ago? :) >>> I suggested changing this from bool to on/off/auto, and >>> make auto mean !modern. >> >> No, please do it like Jason did. The SCSI feature effectively had to be >> enabled explicitly already, the requests were marked as unsupported. > > I didn't know. How is it enabled? It's enabled by default in QEMU, but disabled by default in libvirt. And it only works if you pass a whole _disk_ (not a partition or logical volume) to QEMU, which is definitely not the common case. It can just be documented in the release notes; the feature is still available, and libvirt won't be broken because it adds explicitly both scsi=on and scsi=off. Paolo
On Wed, Jul 15, 2015 at 04:18:49PM +0200, Paolo Bonzini wrote: > > > On 15/07/2015 16:14, Michael S. Tsirkin wrote: > > On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote: > >> > >> > >> On 15/07/2015 14:21, Michael S. Tsirkin wrote: > >>>>> Disable scsi passthrough by default since it was incompatible with > >>>>> virtio 1.0. For legacy machine types, keep this on by default. > >>>>> > >>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com> > >>>>> Cc: Kevin Wolf <kwolf@redhat.com> > >>>>> Cc: qemu-block@nongnu.org > >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> > >>> Seems risky for 2.4. modern is off by default for now. Can't we limit > >>> the change to when modern is enabled? > >> > >> That would have the effect of disabling a feature when you turn on modern. > > > > What's wrong with that? > > Weren't you complaining about it a few hours ago? :) No, I complained about guest driver update disabling it. > >>> I suggested changing this from bool to on/off/auto, and > >>> make auto mean !modern. > >> > >> No, please do it like Jason did. The SCSI feature effectively had to be > >> enabled explicitly already, the requests were marked as unsupported. > > > > I didn't know. How is it enabled? > > It's enabled by default in QEMU, but disabled by default in libvirt. > And it only works if you pass a whole _disk_ (not a partition or logical > volume) to QEMU, which is definitely not the common case. > > It can just be documented in the release notes; the feature is still > available, and libvirt won't be broken because it adds explicitly both > scsi=on and scsi=off. > > Paolo So for libvirt, we don't really care about the default, right? For command line, would it not be friendlier to make it follow the modern flag automatically?
On 15/07/2015 16:28, Michael S. Tsirkin wrote: > On Wed, Jul 15, 2015 at 04:18:49PM +0200, Paolo Bonzini wrote: >> >> >> On 15/07/2015 16:14, Michael S. Tsirkin wrote: >>> On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote: >>>> >>>> >>>> On 15/07/2015 14:21, Michael S. Tsirkin wrote: >>>>>>> Disable scsi passthrough by default since it was incompatible with >>>>>>> virtio 1.0. For legacy machine types, keep this on by default. >>>>>>> >>>>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com> >>>>>>> Cc: Kevin Wolf <kwolf@redhat.com> >>>>>>> Cc: qemu-block@nongnu.org >>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>> Seems risky for 2.4. modern is off by default for now. Can't we limit >>>>> the change to when modern is enabled? >>>> >>>> That would have the effect of disabling a feature when you turn on modern. >>> >>> What's wrong with that? >> >> Weren't you complaining about it a few hours ago? :) > > No, I complained about guest driver update disabling it. Ah, sorry for the confusion. >>>>> I suggested changing this from bool to on/off/auto, and >>>>> make auto mean !modern. >>>> >>>> No, please do it like Jason did. The SCSI feature effectively had to be >>>> enabled explicitly already, the requests were marked as unsupported. >>> >>> I didn't know. How is it enabled? >> >> It's enabled by default in QEMU, but disabled by default in libvirt. >> And it only works if you pass a whole _disk_ (not a partition or logical >> volume) to QEMU, which is definitely not the common case. >> >> It can just be documented in the release notes; the feature is still >> available, and libvirt won't be broken because it adds explicitly both >> scsi=on and scsi=off. > > So for libvirt, we don't really care about the default, right? > For command line, would it not be friendlier to make it follow the > modern flag automatically? I wouldn't mind if we grabbed the occasion to disable it altogether. However, that would indeed work as well. Paolo
<dragging up an old thread again> On Wed, 15 Jul 2015 16:45:52 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 15/07/2015 16:28, Michael S. Tsirkin wrote: > > On Wed, Jul 15, 2015 at 04:18:49PM +0200, Paolo Bonzini wrote: > >> > >> > >> On 15/07/2015 16:14, Michael S. Tsirkin wrote: > >>> On Wed, Jul 15, 2015 at 02:47:24PM +0200, Paolo Bonzini wrote: > >>>> > >>>> > >>>> On 15/07/2015 14:21, Michael S. Tsirkin wrote: > >>>>>>> Disable scsi passthrough by default since it was incompatible with > >>>>>>> virtio 1.0. For legacy machine types, keep this on by default. > >>>>>>> > >>>>>>> Cc: Stefan Hajnoczi <stefanha@redhat.com> > >>>>>>> Cc: Kevin Wolf <kwolf@redhat.com> > >>>>>>> Cc: qemu-block@nongnu.org > >>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> > >>>>> Seems risky for 2.4. modern is off by default for now. Can't we limit > >>>>> the change to when modern is enabled? > >>>> > >>>> That would have the effect of disabling a feature when you turn on modern. > >>> > >>> What's wrong with that? > >> > >> Weren't you complaining about it a few hours ago? :) > > > > No, I complained about guest driver update disabling it. > > Ah, sorry for the confusion. > > >>>>> I suggested changing this from bool to on/off/auto, and > >>>>> make auto mean !modern. > >>>> > >>>> No, please do it like Jason did. The SCSI feature effectively had to be > >>>> enabled explicitly already, the requests were marked as unsupported. > >>> > >>> I didn't know. How is it enabled? > >> > >> It's enabled by default in QEMU, but disabled by default in libvirt. > >> And it only works if you pass a whole _disk_ (not a partition or logical > >> volume) to QEMU, which is definitely not the common case. > >> > >> It can just be documented in the release notes; the feature is still > >> available, and libvirt won't be broken because it adds explicitly both > >> scsi=on and scsi=off. > > > > So for libvirt, we don't really care about the default, right? > > For command line, would it not be friendlier to make it follow the > > modern flag automatically? > > I wouldn't mind if we grabbed the occasion to disable it altogether. > However, that would indeed work as well. Do we want to change anything for 2.5 about the default? Currently, we still default scsi to true, and you have to disable it explicitly if you want to use virtio-1 compliant virtio-blk devices (which is a bit annoying, as scsi passthrough is not something people usually want).
On 14/10/2015 12:29, Cornelia Huck wrote: > Do we want to change anything for 2.5 about the default? > > Currently, we still default scsi to true, and you have to disable it > explicitly if you want to use virtio-1 compliant virtio-blk devices > (which is a bit annoying, as scsi passthrough is not something people > usually want). I think disabling it is best. Can you send a patch? Paolo
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 761d763..362fe53 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -964,7 +964,7 @@ static Property virtio_blk_properties[] = { DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial), DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true), #ifdef __linux__ - DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, true), + DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, false), #endif DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, true), diff --git a/include/hw/compat.h b/include/hw/compat.h index 4a43466..56039d8 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -2,7 +2,11 @@ #define HW_COMPAT_H #define HW_COMPAT_2_3 \ - /* empty */ + {\ + .driver = "virtio-blk-pci",\ + .property = "scsi",\ + .value = "on",\ + }, #define HW_COMPAT_2_2 \ /* empty */
Disable scsi passthrough by default since it was incompatible with virtio 1.0. For legacy machine types, keep this on by default. Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: qemu-block@nongnu.org Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/block/virtio-blk.c | 2 +- include/hw/compat.h | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-)