[Qemu-devel] Commit 622b520f changed -drive if=scsi, index=N, intentional?

[Qemu-devel] Commit 622b520f changed -drive if=scsi, index=N, intentional?

[Markus Armbruster]

Consider -drive if=scsi,index=12,...

Before the commit, index=12 meant bus=1,unit=5.  Example:

    $ qemu-system-x86_64 -nodefaults -vnc :0 -S -monitor stdio -drive if=scsi,index=12,media=cdrom
    QEMU 0.13.50 monitor - type 'help' for more information
    (qemu) info block
    scsi1-cd5: type=cdrom removable=1 locked=0 [not inserted]
    (qemu) info qtree
    [...]
        bus: pci.0
          type PCI
          dev: lsi53c895a, id ""
            bus-prop: addr = 03.0
            bus-prop: romfile = <null>
            bus-prop: rombar = 1
            bus-prop: multifunction = off
            class SCSI controller, addr 00:03.0, pci id 1000:0012 (sub 1af4:1000)
            bar 0: i/o at 0xffffffffffffffff [0xfe]
            bar 1: mem at 0xffffffffffffffff [0x3fe]
            bar 2: mem at 0xffffffffffffffff [0x1ffe]
            bus: scsi.0
              type SCSI
              dev: scsi-disk, id ""
                dev-prop: drive = scsi1-cd5
                dev-prop: logical_block_size = 512
                dev-prop: physical_block_size = 512
                dev-prop: min_io_size = 0
                dev-prop: opt_io_size = 0
                dev-prop: ver = "0.13.50"
                dev-prop: serial = "0"
                bus-prop: scsi-id = 5
          dev: lsi53c895a, id ""
            bus-prop: addr = 02.0
            bus-prop: romfile = <null>
            bus-prop: rombar = 1
            bus-prop: multifunction = off
            class SCSI controller, addr 00:02.0, pci id 1000:0012 (sub 1af4:1000)
            bar 0: i/o at 0xffffffffffffffff [0xfe]
            bar 1: mem at 0xffffffffffffffff [0x3fe]
            bar 2: mem at 0xffffffffffffffff [0x1ffe]
            bus: scsi.0
              type SCSI
    [...]

    Two scsi-buses, and scsi1-cd5 with scsi-id 5 is on the second one,
    i.e. bus=1, unit=5.

After the commit, it means bus=0,unit=12.  The drive is created, but not
the guest device.  That's because lsi53c895a supports only 7 units
(LSI_MAX_DEVS), and scsi_bus_legacy_handle_cmdline() ignores drives with
unit numbers exceeding that limit.  Example:

    $ ~/work/qemu/bld-x86/x86_64-softmmu/qemu-system-x86_64 -nodefaults -usb -m 384 -vnc :0 -enable-kvm -S -readconfig ~/work/qemu/test.cfg -drive if=scsi,index=12,media=cdrom
    QEMU 0.13.50 monitor - type 'help' for more information
    (qemu) info block
    hda: type=hd removable=0 file=/home/armbru/work/images/test.img ro=0 drv=raw encrypted=0
    scsi0-cd12: type=cdrom removable=1 locked=0 [not inserted]
    (qemu) info qtree
    [...]
        bus: pci.0
          type PCI
    [...]
          dev: lsi53c895a, id ""
            bus-prop: addr = 02.0
            bus-prop: romfile = <null>
            bus-prop: rombar = 1
            bus-prop: multifunction = off
            bus-prop: command_serr_enable = on
            class SCSI controller, addr 00:02.0, pci id 1000:0012 (sub 1af4:1000)
            bar 0: i/o at 0xffffffffffffffff [0xfe]
            bar 1: mem at 0xffffffffffffffff [0x3fe]
            bar 2: mem at 0xffffffffffffffff [0x1ffe]
            bus: scsi.0
              type SCSI
    [...]

    One scsi-bus, and scsi1-cd5 nowhere to be found.

I'd call this a regression.

What now?



commit 622b520fb4ca50b5028485f1d225317ece0a42b9
Author: Hannes Reinecke <hare@suse.de>
Date:   Wed Nov 24 12:15:56 2010 +0100

    scsi: Increase the number of possible devices
    
    The SCSI parallel interface has a limit of 8 devices, but
    not the SCSI stack in general. So we should be removing the
    hard-coded limit and use MAX_SCSI_DEVS instead.
    And we only need to scan those devices which are allocated
    by the bus.
    
    Signed-off-by: Hannes Reinecke <hare@suse.de>
    Acked-by: Christoph Hellwig <hch@lst.de>
    Signed-off-by: Kevin Wolf <kwolf@redhat.com>

diff --git a/blockdev.h b/blockdev.h
index 2a0559e..4cb8ca9 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -32,7 +32,7 @@ struct DriveInfo {
 };
 
 #define MAX_IDE_DEVS	2
-#define MAX_SCSI_DEVS	7
+#define MAX_SCSI_DEVS	255
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 int drive_get_max_bus(BlockInterfaceType type);
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 5a3fd4b..74a08b7 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -108,7 +108,7 @@ int scsi_bus_legacy_handle_cmdline(SCSIBus *bus)
     int res = 0, unit;
 
     loc_push_none(&loc);
-    for (unit = 0; unit < MAX_SCSI_DEVS; unit++) {
+    for (unit = 0; unit < bus->ndev; unit++) {
         dinfo = drive_get(IF_SCSI, bus->busnr, unit);
         if (dinfo == NULL) {
             continue;
diff --git a/hw/scsi.h b/hw/scsi.h
index cb06d6d..9c798ae 100644
--- a/hw/scsi.h
+++ b/hw/scsi.h
@@ -3,6 +3,7 @@
 
 #include "qdev.h"
 #include "block.h"
+#include "blockdev.h"
 #include "block_int.h"
 
 #define SCSI_CMD_BUF_SIZE     16
@@ -86,7 +87,7 @@ struct SCSIBus {
     int tcq, ndev;
     scsi_completionfn complete;
 
-    SCSIDevice *devs[8];
+    SCSIDevice *devs[MAX_SCSI_DEVS];
 };
 
 void scsi_bus_new(SCSIBus *bus, DeviceState *host, int tcq, int ndev,

[Qemu-devel] Re: Commit 622b520f changed -drive if=scsi, index=N, intentional?

[Kevin Wolf]

Am 27.01.2011 13:10, schrieb Markus Armbruster:
> Consider -drive if=scsi,index=12,...
> 
> Before the commit, index=12 meant bus=1,unit=5.  Example:
> 
>     [...]
> 
>     Two scsi-buses, and scsi1-cd5 with scsi-id 5 is on the second one,
>     i.e. bus=1, unit=5.
> 
> After the commit, it means bus=0,unit=12.  The drive is created, but not
> the guest device.  That's because lsi53c895a supports only 7 units
> (LSI_MAX_DEVS), and scsi_bus_legacy_handle_cmdline() ignores drives with
> unit numbers exceeding that limit.  Example:
> 
>     [...]
> 
>     One scsi-bus, and scsi1-cd5 nowhere to be found.
> 
> I'd call this a regression.
> 
> What now?

That's a really good question. We could do something like this:

--- a/blockdev.c
+++ b/blockdev.c
@@ -192,7 +192,7 @@ DriveInfo *drive_init(QemuOpts *opts, int
default_to_scsi)
             max_devs = MAX_IDE_DEVS;
         } else if (!strcmp(buf, "scsi")) {
            type = IF_SCSI;
-            max_devs = MAX_SCSI_DEVS;
+            max_devs = 7;
         } else if (!strcmp(buf, "floppy")) {
            type = IF_FLOPPY;
             max_devs = 0;

That's very obviously not much more than a hack, but I don't think
blockdev.c can get the real number easily (please prove me wrong). With
this hack, we would get the old behaviour for -drive (which doesn't use
any other controller anyway) and you can still use -device to attach
more devices to a non-lsi bus.

Kevin

Re: [Qemu-devel] Commit 622b520f changed -drive if=scsi, index=N, intentional?

[Daniel P. Berrange]

On Thu, Jan 27, 2011 at 01:10:50PM +0100, Markus Armbruster wrote:
> Consider -drive if=scsi,index=12,...
> 
> Before the commit, index=12 meant bus=1,unit=5.  Example:
> 
>     $ qemu-system-x86_64 -nodefaults -vnc :0 -S -monitor stdio -drive if=scsi,index=12,media=cdrom
>     QEMU 0.13.50 monitor - type 'help' for more information
>     (qemu) info block
>     scsi1-cd5: type=cdrom removable=1 locked=0 [not inserted]
>     (qemu) info qtree
>     [...]
>         bus: pci.0
>           type PCI
>           dev: lsi53c895a, id ""
>             bus-prop: addr = 03.0
>             bus-prop: romfile = <null>
>             bus-prop: rombar = 1
>             bus-prop: multifunction = off
>             class SCSI controller, addr 00:03.0, pci id 1000:0012 (sub 1af4:1000)
>             bar 0: i/o at 0xffffffffffffffff [0xfe]
>             bar 1: mem at 0xffffffffffffffff [0x3fe]
>             bar 2: mem at 0xffffffffffffffff [0x1ffe]
>             bus: scsi.0
>               type SCSI
>               dev: scsi-disk, id ""
>                 dev-prop: drive = scsi1-cd5
>                 dev-prop: logical_block_size = 512
>                 dev-prop: physical_block_size = 512
>                 dev-prop: min_io_size = 0
>                 dev-prop: opt_io_size = 0
>                 dev-prop: ver = "0.13.50"
>                 dev-prop: serial = "0"
>                 bus-prop: scsi-id = 5
>           dev: lsi53c895a, id ""
>             bus-prop: addr = 02.0
>             bus-prop: romfile = <null>
>             bus-prop: rombar = 1
>             bus-prop: multifunction = off
>             class SCSI controller, addr 00:02.0, pci id 1000:0012 (sub 1af4:1000)
>             bar 0: i/o at 0xffffffffffffffff [0xfe]
>             bar 1: mem at 0xffffffffffffffff [0x3fe]
>             bar 2: mem at 0xffffffffffffffff [0x1ffe]
>             bus: scsi.0
>               type SCSI
>     [...]
> 
>     Two scsi-buses, and scsi1-cd5 with scsi-id 5 is on the second one,
>     i.e. bus=1, unit=5.
> 
> After the commit, it means bus=0,unit=12.  The drive is created, but not
> the guest device.  That's because lsi53c895a supports only 7 units
> (LSI_MAX_DEVS), and scsi_bus_legacy_handle_cmdline() ignores drives with
> unit numbers exceeding that limit.  Example:
> 
>     $ ~/work/qemu/bld-x86/x86_64-softmmu/qemu-system-x86_64 -nodefaults -usb -m 384 -vnc :0 -enable-kvm -S -readconfig ~/work/qemu/test.cfg -drive if=scsi,index=12,media=cdrom
>     QEMU 0.13.50 monitor - type 'help' for more information
>     (qemu) info block
>     hda: type=hd removable=0 file=/home/armbru/work/images/test.img ro=0 drv=raw encrypted=0
>     scsi0-cd12: type=cdrom removable=1 locked=0 [not inserted]
>     (qemu) info qtree
>     [...]
>         bus: pci.0
>           type PCI
>     [...]
>           dev: lsi53c895a, id ""
>             bus-prop: addr = 02.0
>             bus-prop: romfile = <null>
>             bus-prop: rombar = 1
>             bus-prop: multifunction = off
>             bus-prop: command_serr_enable = on
>             class SCSI controller, addr 00:02.0, pci id 1000:0012 (sub 1af4:1000)
>             bar 0: i/o at 0xffffffffffffffff [0xfe]
>             bar 1: mem at 0xffffffffffffffff [0x3fe]
>             bar 2: mem at 0xffffffffffffffff [0x1ffe]
>             bus: scsi.0
>               type SCSI
>     [...]
> 
>     One scsi-bus, and scsi1-cd5 nowhere to be found.
> 
> I'd call this a regression.
> 
> What now?

Not answering your question, but it occurs to me that you've
demonstrated a easy way to provide some unit tests for QEMU
to validate that we're not changing the device model in
ways like this that could upset guests. eg in the source
tree have a set of files each containing a list of command
line arguments, and a corresponding set of files containing
the 'info qdev' output. The unit tests run the command lines
and compare the 'info qdev' output against what's recorded
in the expect data files and complain if they differ.

Daniel

[Qemu-devel] Re: Commit 622b520f changed -drive if=scsi, index=N, intentional?

[Paolo Bonzini]

On 01/27/2011 01:10 PM, Markus Armbruster wrote:
> Consider -drive if=scsi,index=12,...
> 
> Before the commit, index=12 meant bus=1,unit=5.  Example:
> 
>      $ qemu-system-x86_64 -nodefaults -vnc :0 -S -monitor stdio -drive if=scsi,index=12,media=cdrom
>      QEMU 0.13.50 monitor - type 'help' for more information
>      (qemu) info block
>      scsi1-cd5: type=cdrom removable=1 locked=0 [not inserted]
>      (qemu) info qtree
>      [...]
>          bus: pci.0
>            type PCI
>            dev: lsi53c895a, id ""
>              bus-prop: addr = 03.0
>              bus-prop: romfile =<null>
>              bus-prop: rombar = 1
>              bus-prop: multifunction = off
>              class SCSI controller, addr 00:03.0, pci id 1000:0012 (sub 1af4:1000)
>              bar 0: i/o at 0xffffffffffffffff [0xfe]
>              bar 1: mem at 0xffffffffffffffff [0x3fe]
>              bar 2: mem at 0xffffffffffffffff [0x1ffe]
>              bus: scsi.0
>                type SCSI
>                dev: scsi-disk, id ""
>                  dev-prop: drive = scsi1-cd5
>                  dev-prop: logical_block_size = 512
>                  dev-prop: physical_block_size = 512
>                  dev-prop: min_io_size = 0
>                  dev-prop: opt_io_size = 0
>                  dev-prop: ver = "0.13.50"
>                  dev-prop: serial = "0"
>                  bus-prop: scsi-id = 5
>            dev: lsi53c895a, id ""
>              bus-prop: addr = 02.0
>              bus-prop: romfile =<null>
>              bus-prop: rombar = 1
>              bus-prop: multifunction = off
>              class SCSI controller, addr 00:02.0, pci id 1000:0012 (sub 1af4:1000)
>              bar 0: i/o at 0xffffffffffffffff [0xfe]
>              bar 1: mem at 0xffffffffffffffff [0x3fe]
>              bar 2: mem at 0xffffffffffffffff [0x1ffe]
>              bus: scsi.0
>                type SCSI
>      [...]
> 
>      Two scsi-buses, and scsi1-cd5 with scsi-id 5 is on the second one,
>      i.e. bus=1, unit=5.
> 
> After the commit, it means bus=0,unit=12.  The drive is created, but not
> the guest device.  That's because lsi53c895a supports only 7 units
> (LSI_MAX_DEVS), and scsi_bus_legacy_handle_cmdline() ignores drives with
> unit numbers exceeding that limit.

The culprit is

        type = IF_SCSI;
        max_devs = MAX_SCSI_DEVS;
        pstrcpy(devname, sizeof(devname), "scsi");

...

        } else if (!strcmp(buf, "scsi")) {
            type = IF_SCSI;
            max_devs = MAX_SCSI_DEVS;

Here is a patch, untested but trivial.


-------------- --8< ----------------
>From f64236fa1df4cfecaa2e5b91ce5eeb47ce97e97c Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Thu, 27 Jan 2011 13:45:16 +0100
Subject: [PATCH] change behavior of index=N to be the same as 0.13 and earlier

Even though we support theoretically more than 7 devices per target,
the controllers we have do not and they ignore LUNs above 6.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 blockdev.c |    4 ++--
 blockdev.h |    5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index f7f591f..c5078f8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -163,7 +163,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
 
     if (default_to_scsi) {
         type = IF_SCSI;
-        max_devs = MAX_SCSI_DEVS;
+        max_devs = DEFAULT_SCSI_DEVS_PER_BUS;
         pstrcpy(devname, sizeof(devname), "scsi");
     } else {
         type = IF_IDE;
@@ -194,7 +194,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi, int *fatal_error)
             max_devs = MAX_IDE_DEVS;
         } else if (!strcmp(buf, "scsi")) {
 	    type = IF_SCSI;
-            max_devs = MAX_SCSI_DEVS;
+            max_devs = DEFAULT_SCSI_DEVS_PER_BUS;
         } else if (!strcmp(buf, "floppy")) {
 	    type = IF_FLOPPY;
             max_devs = 0;
diff --git a/blockdev.h b/blockdev.h
index 4536b5c..1cfc013 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -31,8 +31,9 @@ struct DriveInfo {
     QTAILQ_ENTRY(DriveInfo) next;
 };
 
-#define MAX_IDE_DEVS	2
-#define MAX_SCSI_DEVS	255
+#define MAX_IDE_DEVS			2
+#define MAX_SCSI_DEVS			255
+#define DEFAULT_SCSI_DEVS_PER_BUS	7
 
 DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
 int drive_get_max_bus(BlockInterfaceType type);
-- 
1.7.3.4

Paolo

[Qemu-devel] Re: Commit 622b520f changed -drive if=scsi, index=N, intentional?

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 27.01.2011 13:10, schrieb Markus Armbruster:
>> Consider -drive if=scsi,index=12,...
>> 
>> Before the commit, index=12 meant bus=1,unit=5.  Example:
>> 
>>     [...]
>> 
>>     Two scsi-buses, and scsi1-cd5 with scsi-id 5 is on the second one,
>>     i.e. bus=1, unit=5.
>> 
>> After the commit, it means bus=0,unit=12.  The drive is created, but not
>> the guest device.  That's because lsi53c895a supports only 7 units
>> (LSI_MAX_DEVS), and scsi_bus_legacy_handle_cmdline() ignores drives with
>> unit numbers exceeding that limit.  Example:
>> 
>>     [...]
>> 
>>     One scsi-bus, and scsi1-cd5 nowhere to be found.
>> 
>> I'd call this a regression.
>> 
>> What now?
>
> That's a really good question. We could do something like this:
>
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -192,7 +192,7 @@ DriveInfo *drive_init(QemuOpts *opts, int
> default_to_scsi)
>              max_devs = MAX_IDE_DEVS;
>          } else if (!strcmp(buf, "scsi")) {
>             type = IF_SCSI;
> -            max_devs = MAX_SCSI_DEVS;
> +            max_devs = 7;
>          } else if (!strcmp(buf, "floppy")) {
>             type = IF_FLOPPY;
>              max_devs = 0;
>
> That's very obviously not much more than a hack, but I don't think
> blockdev.c can get the real number easily (please prove me wrong). With
> this hack, we would get the old behaviour for -drive (which doesn't use
> any other controller anyway) and you can still use -device to attach
> more devices to a non-lsi bus.

The real number is 7, because it's always been 7 :)

Okay, back to serious.  drive_init() with IF_SCSI is tied to
scsi_bus_legacy_handle_cmdline() and its callers.  The callers are SCSI
controller device initialization functions.  The controller has a per
bus unit limit.  And that's the real number for that bus.

But drive_init() doesn't know which device is going to provide a given
bus number!  It shouldn't know.  Perhaps it even can't know.

We have two callers now: lsi_scsi_init(), limit 7 (LSI_MAX_DEVS), and
esp_init1(), limit 7 (ESP_MAX_DEVS).

Let's worry about it when we add a caller with a different limit.

Re: [Qemu-devel] Re: Commit 622b520f changed -drive if=scsi, index=N, intentional?

[Gerd Hoffmann]

   Hi,

>           } else if (!strcmp(buf, "scsi")) {
>              type = IF_SCSI;
> -            max_devs = MAX_SCSI_DEVS;
> +            max_devs = 7;

> That's very obviously not much more than a hack, but I don't think
> blockdev.c can get the real number easily (please prove me wrong). With
> this hack, we would get the old behaviour for -drive (which doesn't use
> any other controller anyway) and you can still use -device to attach
> more devices to a non-lsi bus.

Looks sensible to me.  scsi controllers with more than 7 devs (megasas 
and whatever else might be coming, virtio?) can't be added via if=scsi 
legacy syntax and must use -blockdev and -device anyway.

We probably want add a comment explaining this though.

cheers,
   Gerd

Re: [Qemu-devel] Re: Commit 622b520f changed -drive if=scsi, index=N, intentional?

[Hannes Reinecke]

On 01/27/2011 08:26 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>           } else if (!strcmp(buf, "scsi")) {
>>              type = IF_SCSI;
>> -            max_devs = MAX_SCSI_DEVS;
>> +            max_devs = 7;
> 
>> That's very obviously not much more than a hack, but I don't think
>> blockdev.c can get the real number easily (please prove me wrong). With
>> this hack, we would get the old behaviour for -drive (which doesn't use
>> any other controller anyway) and you can still use -device to attach
>> more devices to a non-lsi bus.
> 
> Looks sensible to me.  scsi controllers with more than 7 devs (megasas
> and whatever else might be coming, virtio?) can't be added via if=scsi
> legacy syntax and must use -blockdev and -device anyway.
> 
> We probably want add a comment explaining this though.
> 
Yes please. It's a bit non-obvious that 'scsi' means in fact 'lsi'.

As long as there a way of specifying more than 8 devs
I'm fine with that patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)