Re: [Qemu-block] [PATCH v2 3/3] virtio-blk: switch off scsi-passthrough by default

[Cornelia Huck]

On Mon, 19 Oct 2015 14:57:31 +0300
"Michael S. Tsirkin"  wrote:

> On Mon, Oct 19, 2015 at 01:53:50PM +0200, Cornelia Huck wrote:
> > On Sun, 18 Oct 2015 10:59:59 +0300
> > "Michael S. Tsirkin"  wrote:
> > 
> > > On Fri, Oct 16, 2015 at 01:07:28PM +0200, Christian Borntraeger wrote:
> > 
> > > > Lets keep this patch as is to have scsi=off as default for virtio 1.0
> > > > 
> > > > (some iotests do fail because of this)
> > > > 
> > > > Christian
> > > 
> > > What fails, exactly?
> > 
> > For example, testcase 068 (on s390x, since we default virtio-1 to on):
> 
> I see, thanks.
> So it's just the assertion that we have in code that fires.
> Sure, scsi must be off by default before virtio 1 is on.

And -hda always assumes defaults...

Re: [Qemu-block] [PATCH v2 3/3] virtio-blk: switch off scsi-passthrough by default

[Cornelia Huck]

On Sun, 18 Oct 2015 10:59:59 +0300
"Michael S. Tsirkin"  wrote:

> On Fri, Oct 16, 2015 at 01:07:28PM +0200, Christian Borntraeger wrote:

> > Lets keep this patch as is to have scsi=off as default for virtio 1.0
> > 
> > (some iotests do fail because of this)
> > 
> > Christian
> 
> What fails, exactly?

For example, testcase 068 (on s390x, since we default virtio-1 to on):

--- /data/git/yyy/qemu/tests/qemu-iotests/068.out   2015-03-09 
12:32:35.245444052 +0100
+++ 068.out.bad 2015-10-19 13:48:00.023772655 +0200
@@ -3,9 +3,8 @@
 === Saving and reloading a VM state to/from a qcow2 image ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+qemu-system-s390x: -hda 
/data/git/yyy/qemu/build/tests/qemu-iotests/scratch/t.qcow2: Please set 
scsi=off for virtio-blk devices in order to use virtio 1.0
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) savevm 0
-(qemu) quit
+(qemu) qemu-system-s390x: -hda 
/data/git/yyy/qemu/build/tests/qemu-iotests/scratch/t.qcow2: Please set 
scsi=off for virtio-blk devices in order to use virtio 1.0
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) quit
-*** done
+(qemu) *** done

Re: [Qemu-block] [PATCH v2 3/3] virtio-blk: switch off scsi-passthrough by default

[Michael S. Tsirkin]

On Mon, Oct 19, 2015 at 01:53:50PM +0200, Cornelia Huck wrote:
> On Sun, 18 Oct 2015 10:59:59 +0300
> "Michael S. Tsirkin"  wrote:
> 
> > On Fri, Oct 16, 2015 at 01:07:28PM +0200, Christian Borntraeger wrote:
> 
> > > Lets keep this patch as is to have scsi=off as default for virtio 1.0
> > > 
> > > (some iotests do fail because of this)
> > > 
> > > Christian
> > 
> > What fails, exactly?
> 
> For example, testcase 068 (on s390x, since we default virtio-1 to on):

I see, thanks.
So it's just the assertion that we have in code that fires.
Sure, scsi must be off by default before virtio 1 is on.


> --- /data/git/yyy/qemu/tests/qemu-iotests/068.out 2015-03-09 
> 12:32:35.245444052 +0100
> +++ 068.out.bad   2015-10-19 13:48:00.023772655 +0200
> @@ -3,9 +3,8 @@
>  === Saving and reloading a VM state to/from a qcow2 image ===
>  
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
> +qemu-system-s390x: -hda 
> /data/git/yyy/qemu/build/tests/qemu-iotests/scratch/t.qcow2: Please set 
> scsi=off for virtio-blk devices in order to use virtio 1.0
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) savevm 0
> -(qemu) quit
> +(qemu) qemu-system-s390x: -hda 
> /data/git/yyy/qemu/build/tests/qemu-iotests/scratch/t.qcow2: Please set 
> scsi=off for virtio-blk devices in order to use virtio 1.0
>  QEMU X.Y.Z monitor - type 'help' for more information
> -(qemu) quit
> -*** done
> +(qemu) *** done

Re: [Qemu-block] [PATCH v2 3/3] virtio-blk: switch off scsi-passthrough by default

[Michael S. Tsirkin]

On Fri, Oct 16, 2015 at 01:07:28PM +0200, Christian Borntraeger wrote:
> Am 16.10.2015 um 12:44 schrieb Cornelia Huck:
> > On Fri, 16 Oct 2015 12:32:52 +0200
> > Christian Borntraeger  wrote:
> > 
> >> Am 16.10.2015 um 12:25 schrieb Cornelia Huck:
> >>> Devices that are compliant with virtio-1 do not support scsi
> >>> passthrough any more (and it has not been a recommended setup
> >>> anyway for quite some time). To avoid having to switch it off
> >>> explicitly in newer qemus that turn on virtio-1 by default, let's
> >>> switch the default to scsi=false for 2.5.
> >>>
> >>> Signed-off-by: Cornelia Huck 
> >>> ---
> >>>  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 8beb26b..999dbd7 100644
> >>> --- a/hw/block/virtio-blk.c
> >>> +++ b/hw/block/virtio-blk.c
> >>> @@ -975,7 +975,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 095de5d..93e71af 100644
> >>> --- a/include/hw/compat.h
> >>> +++ b/include/hw/compat.h
> >>> @@ -2,7 +2,11 @@
> >>>  #define HW_COMPAT_H
> >>>
> >>>  #define HW_COMPAT_2_4 \
> >>> -/* empty */
> >>> +{\
> >>> +.driver   = "virtio-blk-device",\
> >>> +.property = "scsi",\
> >>> +.value= "true",\
> >>
> >> does that work?
> > 
> > It did for me :)
> > 
> >>
> >> If yes, would it make sense to convert the things in HW_COMPAT_2_3 from
> >> pci to device, e.g.
> >>
> >>
> >> {\
> >> -   .driver   = "virtio-blk-pci",\
> >> +   .driver   = "virtio-blk-device",\
> >> .property = "any_layout",\
> >> .value= "off",\
> >> ...
> > 
> > Not sure: We don't have 2.3 compat for ccw... but would give a better
> > template for later changes.
> 
> Yes. But this can be an addon patch. 
> 
> Lets keep this patch as is to have scsi=off as default for virtio 1.0
> 
> (some iotests do fail because of this)
> 
> Christian

What fails, exactly?

Re: [Qemu-block] [PATCH v2 3/3] virtio-blk: switch off scsi-passthrough by default

[Christian Borntraeger]

Am 16.10.2015 um 12:25 schrieb Cornelia Huck:
> Devices that are compliant with virtio-1 do not support scsi
> passthrough any more (and it has not been a recommended setup
> anyway for quite some time). To avoid having to switch it off
> explicitly in newer qemus that turn on virtio-1 by default, let's
> switch the default to scsi=false for 2.5.
> 
> Signed-off-by: Cornelia Huck 
> ---
>  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 8beb26b..999dbd7 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -975,7 +975,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 095de5d..93e71af 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -2,7 +2,11 @@
>  #define HW_COMPAT_H
> 
>  #define HW_COMPAT_2_4 \
> -/* empty */
> +{\
> +.driver   = "virtio-blk-device",\
> +.property = "scsi",\
> +.value= "true",\

does that work?

If yes, would it make sense to convert the things in HW_COMPAT_2_3 from
pci to device, e.g.


{\
-   .driver   = "virtio-blk-pci",\
+   .driver   = "virtio-blk-device",\
.property = "any_layout",\
.value= "off",\
...

Re: [Qemu-block] [PATCH v2 3/3] virtio-blk: switch off scsi-passthrough by default

[Cornelia Huck]

On Fri, 16 Oct 2015 12:32:52 +0200
Christian Borntraeger  wrote:

> Am 16.10.2015 um 12:25 schrieb Cornelia Huck:
> > Devices that are compliant with virtio-1 do not support scsi
> > passthrough any more (and it has not been a recommended setup
> > anyway for quite some time). To avoid having to switch it off
> > explicitly in newer qemus that turn on virtio-1 by default, let's
> > switch the default to scsi=false for 2.5.
> > 
> > Signed-off-by: Cornelia Huck 
> > ---
> >  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 8beb26b..999dbd7 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -975,7 +975,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 095de5d..93e71af 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -2,7 +2,11 @@
> >  #define HW_COMPAT_H
> > 
> >  #define HW_COMPAT_2_4 \
> > -/* empty */
> > +{\
> > +.driver   = "virtio-blk-device",\
> > +.property = "scsi",\
> > +.value= "true",\
> 
> does that work?

It did for me :)

> 
> If yes, would it make sense to convert the things in HW_COMPAT_2_3 from
> pci to device, e.g.
> 
> 
> {\
> -   .driver   = "virtio-blk-pci",\
> +   .driver   = "virtio-blk-device",\
> .property = "any_layout",\
> .value= "off",\
> ...

Not sure: We don't have 2.3 compat for ccw... but would give a better
template for later changes.

[Qemu-block] [PATCH v2 3/3] virtio-blk: switch off scsi-passthrough by default

[Cornelia Huck]

Devices that are compliant with virtio-1 do not support scsi
passthrough any more (and it has not been a recommended setup
anyway for quite some time). To avoid having to switch it off
explicitly in newer qemus that turn on virtio-1 by default, let's
switch the default to scsi=false for 2.5.

Signed-off-by: Cornelia Huck 
---
 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 8beb26b..999dbd7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -975,7 +975,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 095de5d..93e71af 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,11 @@
 #define HW_COMPAT_H
 
 #define HW_COMPAT_2_4 \
-/* empty */
+{\
+.driver   = "virtio-blk-device",\
+.property = "scsi",\
+.value= "true",\
+},
 
 #define HW_COMPAT_2_3 \
 {\
-- 
2.3.9

Re: [Qemu-block] [PATCH v2 3/3] virtio-blk: switch off scsi-passthrough by default

[Christian Borntraeger]

Am 16.10.2015 um 12:44 schrieb Cornelia Huck:
> On Fri, 16 Oct 2015 12:32:52 +0200
> Christian Borntraeger  wrote:
> 
>> Am 16.10.2015 um 12:25 schrieb Cornelia Huck:
>>> Devices that are compliant with virtio-1 do not support scsi
>>> passthrough any more (and it has not been a recommended setup
>>> anyway for quite some time). To avoid having to switch it off
>>> explicitly in newer qemus that turn on virtio-1 by default, let's
>>> switch the default to scsi=false for 2.5.
>>>
>>> Signed-off-by: Cornelia Huck 
>>> ---
>>>  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 8beb26b..999dbd7 100644
>>> --- a/hw/block/virtio-blk.c
>>> +++ b/hw/block/virtio-blk.c
>>> @@ -975,7 +975,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 095de5d..93e71af 100644
>>> --- a/include/hw/compat.h
>>> +++ b/include/hw/compat.h
>>> @@ -2,7 +2,11 @@
>>>  #define HW_COMPAT_H
>>>
>>>  #define HW_COMPAT_2_4 \
>>> -/* empty */
>>> +{\
>>> +.driver   = "virtio-blk-device",\
>>> +.property = "scsi",\
>>> +.value= "true",\
>>
>> does that work?
> 
> It did for me :)
> 
>>
>> If yes, would it make sense to convert the things in HW_COMPAT_2_3 from
>> pci to device, e.g.
>>
>>
>> {\
>> -   .driver   = "virtio-blk-pci",\
>> +   .driver   = "virtio-blk-device",\
>> .property = "any_layout",\
>> .value= "off",\
>> ...
> 
> Not sure: We don't have 2.3 compat for ccw... but would give a better
> template for later changes.

Yes. But this can be an addon patch. 

Lets keep this patch as is to have scsi=off as default for virtio 1.0

(some iotests do fail because of this)

Christian