Re: [Qemu-block] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Paolo Bonzini]

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

Re: [Qemu-block] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Cornelia Huck]

On Wed, 15 Jul 2015 16:45:52 +0200
Paolo Bonzini  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 
> >>> Cc: Kevin Wolf 
> >>> Cc: qemu-block@nongnu.org
> >>> Signed-off-by: Jason Wang 
> > 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).

Re: [Qemu-block] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Paolo Bonzini]

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 stefa...@redhat.com
  Cc: Kevin Wolf kw...@redhat.com
  Cc: qemu-block@nongnu.org
  Signed-off-by: Jason Wang jasow...@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

Re: [Qemu-block] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Michael S. Tsirkin]

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 stefa...@redhat.com
 Cc: Kevin Wolf kw...@redhat.com
 Cc: qemu-block@nongnu.org
 Signed-off-by: Jason Wang jasow...@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

Re: [Qemu-block] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Michael S. Tsirkin]

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 stefa...@redhat.com
   Cc: Kevin Wolf kw...@redhat.com
   Cc: qemu-block@nongnu.org
   Signed-off-by: Jason Wang jasow...@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?

-- 
MST

Re: [Qemu-block] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Paolo Bonzini]

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 stefa...@redhat.com
 Cc: Kevin Wolf kw...@redhat.com
 Cc: qemu-block@nongnu.org
 Signed-off-by: Jason Wang jasow...@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

[Qemu-block] [PATCH V2 3/5] virtio-blk: disable scsi passthrough by default

[Jason Wang]

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 stefa...@redhat.com
Cc: Kevin Wolf kw...@redhat.com
Cc: qemu-block@nongnu.org
Signed-off-by: Jason Wang jasow...@redhat.com
---
 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