Re: [PATCH v5 2/2] tests: add virtio-scsi and virtio-blk seg_max_adjust test

[Cornelia Huck]

On Wed, 22 Jan 2020 22:47:42 +0100
Philippe Mathieu-Daudé <address@hidden> wrote:


> > This test is failing on OSX:
> > 
> > TestFail: machine type pc-i440fx-2.0: <class 'TypeError'>
> > 
> > Looking at my job-results/job-2020-01-22T17.54-92b7fae/job.log:
> > 
> > Unexpected error in object_property_find() at qom/object.c:1201:
> > qemu-system-x86_64: -device virtio-blk-pci,id=scsi0,drive=drive0: can't 
> > apply global virtio-blk-device.scsi=true: Property '.scsi' not found
> > 
> > Which makes sense looking at hw/block/virtio-blk.c:
> > 
> > 1261 static Property virtio_blk_properties[] = {
> > 1262     DEFINE_BLOCK_PROPERTIES(VirtIOBlock, conf.conf),
> > ...
> > 1268 #ifdef __linux__
> > 1269     DEFINE_PROP_BIT64("scsi", VirtIOBlock, host_features,
> > 1270                       VIRTIO_BLK_F_SCSI, false),
> > 1271 #endif
> > 
> > Except code moved around, origin is:
> > 
> > $ git show 1063b8b15
> > commit 1063b8b15fb49fcf88ffa282b19aaaf7ca9c678c
> > Author: Christoph Hellwig <address@hidden>
> > Date:   Mon Apr 27 10:29:14 2009 +0200
> > 
> >      virtio-blk: add SGI_IO passthru support
> > 
> >      Add support for SG_IO passthru (packet commands) to the virtio-blk
> >      backend.  Conceptually based on an older patch from Hannes Reinecke
> >      but largely rewritten to match the code structure and layering in
> >      virtio-blk.
> > 
> >      Note that currently we issue the hose SG_IO synchronously.  We could
> >      easily switch to async I/O, but that would required either bloating
> >      the VirtIOBlockReq by the size of struct sg_io_hdr or an additional
> >      memory allocation for each SG_IO request.
> > 
> > I'm not sure what is the correct way to fix this.  
> 
> ... because of:
> 
> $ git show ed65fd1a27
> commit ed65fd1a2750d24290354cc7ea49caec7c13e30b
> Author: Cornelia Huck <address@hidden>
> Date:   Fri Oct 16 12:25:54 2015 +0200
> 
>      virtio-blk: switch off scsi-passthrough by default
> 
>      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 <address@hidden>
>      Message-id: address@hidden
>      Signed-off-by: Stefan Hajnoczi <address@hidden>
> 
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 095de5d12f..93e71afb4a 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",\
> +        },

This code has changed a lot in the meantime...

> 
>   #define HW_COMPAT_2_3 \
>           {\
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 3e230debb8..45a24e4fa6 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -972,7 +972,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),
> 
> Should this HW_COMPAT_2_4 entry be guarded with ifdef __linux__?

... so something like the following might do the trick:

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 3e288bfceb7f..d8e30e4895d8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -148,7 +148,8 @@ GlobalProperty hw_compat_2_5[] = {
 const size_t hw_compat_2_5_len = G_N_ELEMENTS(hw_compat_2_5);
 
 GlobalProperty hw_compat_2_4[] = {
-    { "virtio-blk-device", "scsi", "true" },
+    /* Optional because the 'scsi' property is Linux-only */
+    { "virtio-blk-device", "scsi", "true", .optional = true },
     { "e1000", "extra_mac_registers", "off" },
     { "virtio-pci", "x-disable-pcie", "on" },
     { "virtio-pci", "migrate-extra", "off" },


> 
> Probably nobody ran a pre-2.4 machine out of Linux =)
> 

Yeah. I'm wondering if there's more compat stuff in there that should
be optional. Devices that simply do not exist are not a problem, but
properties that not always exist are.