Patchwork Re: Commit 622b520f changed -drive if=scsi, index=N, intentional?

login
register
mail settings
Submitter Kevin Wolf
Date Jan. 27, 2011, 12:21 p.m.
Message ID <4D416342.6080001@redhat.com>
Download mbox | patch
Permalink /patch/80657/
State New
Headers show

Comments

Kevin Wolf - Jan. 27, 2011, 12:21 p.m.
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:

             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
Markus Armbruster - Jan. 27, 2011, 3:11 p.m.
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.
Gerd Hoffmann - Jan. 27, 2011, 7:26 p.m.
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
Hannes Reinecke - Jan. 28, 2011, 7:27 a.m.
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

Patch

--- 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;