[PATCH 0/3] hw/scsi: Cleanup around scsi_bus_legacy_handle_cmdline()

[PATCH 0/3] hw/scsi: Cleanup around scsi_bus_legacy_handle_cmdline()

[Philippe Mathieu-Daudé]

When a device model requires legacy command line handling,
call scsi_bus_legacy_handle_cmdline() in its realize handler
instead of having each user call it.

This applies to:
 - spapr_vscsi
 - lsi53c810 / lsi53c895a
 - sysbus_esp

Note, scsi_bus_legacy_handle_cmdline() prototype could be
made private to hw/scsi/ to restrict its use to scsi device
implementations.

Philippe Mathieu-Daudé (3):
  hw/scsi/spapr_vscsi: Call scsi_bus_legacy_handle_cmdline() in REALIZE
  hw/scsi/lsi53c895a: Call scsi_bus_legacy_handle_cmdline() once
  hw/scsi/esp: Call scsi_bus_legacy_handle_cmdline() once

 include/hw/pci/pci.h  | 2 --
 hw/arm/realview.c     | 3 +--
 hw/arm/versatilepb.c  | 3 +--
 hw/hppa/machine.c     | 3 +--
 hw/m68k/next-cube.c   | 2 --
 hw/m68k/q800.c        | 2 --
 hw/mips/jazz.c        | 2 --
 hw/ppc/prep.c         | 1 -
 hw/scsi/esp.c         | 1 +
 hw/scsi/lsi53c895a.c  | 8 +-------
 hw/scsi/spapr_vscsi.c | 4 ++--
 hw/sparc/sun4m.c      | 1 -
 12 files changed, 7 insertions(+), 25 deletions(-)

-- 
2.45.2

[PATCH 1/3] hw/scsi/spapr_vscsi: Call scsi_bus_legacy_handle_cmdline() in REALIZE

[Philippe Mathieu-Daudé]

Call scsi_bus_legacy_handle_cmdline() in the DeviceRealize
handler, just after scsi_bus_init().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/spapr_vscsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index c75a6c8807..8e76bfd7ce 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -1218,6 +1218,7 @@ static void spapr_vscsi_realize(SpaprVioDevice *dev, Error **errp)
     dev->crq.SendFunc = vscsi_do_crq;
 
     scsi_bus_init(&s->bus, sizeof(s->bus), DEVICE(dev), &vscsi_scsi_info);
+    scsi_bus_legacy_handle_cmdline(&s->bus);
 
     /* ibmvscsi SCSI bus does not allow hotplug. */
     qbus_set_hotplug_handler(BUS(&s->bus), NULL);
@@ -1227,10 +1228,9 @@ void spapr_vscsi_create(SpaprVioBus *bus)
 {
     DeviceState *dev;
 
-    dev = qdev_new("spapr-vscsi");
+    dev = qdev_new(TYPE_VIO_SPAPR_VSCSI_DEVICE);
 
     qdev_realize_and_unref(dev, &bus->bus, &error_fatal);
-    scsi_bus_legacy_handle_cmdline(&VIO_SPAPR_VSCSI_DEVICE(dev)->bus);
 }
 
 static int spapr_vscsi_devnode(SpaprVioDevice *dev, void *fdt, int node_off)
-- 
2.45.2

[PATCH 2/3] hw/scsi/lsi53c895a: Call scsi_bus_legacy_handle_cmdline() once

[Philippe Mathieu-Daudé]

Call scsi_bus_legacy_handle_cmdline() once in the DeviceRealize
handler, just after scsi_bus_init().
No need for lsi53c8xx_handle_legacy_cmdline(), remove it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/pci/pci.h | 2 --
 hw/arm/realview.c    | 3 +--
 hw/arm/versatilepb.c | 3 +--
 hw/hppa/machine.c    | 3 +--
 hw/ppc/prep.c        | 1 -
 hw/scsi/lsi53c895a.c | 8 +-------
 6 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index c0717e3121..7a69f0368c 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -655,8 +655,6 @@ PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
                                            const char *name);
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
 
-void lsi53c8xx_handle_legacy_cmdline(DeviceState *lsi_dev);
-
 qemu_irq pci_allocate_irq(PCIDevice *pci_dev);
 void pci_set_irq(PCIDevice *pci_dev, int level);
 
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index b186f965c6..1042c1a1a3 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -294,8 +294,7 @@ static void realview_init(MachineState *machine,
         }
         n = drive_get_max_bus(IF_SCSI);
         while (n >= 0) {
-            dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
-            lsi53c8xx_handle_legacy_cmdline(dev);
+            pci_create_simple(pci_bus, -1, "lsi53c895a");
             n--;
         }
     }
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index d48235453e..716ed951bc 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -271,8 +271,7 @@ static void versatile_init(MachineState *machine, int board_id)
     }
     n = drive_get_max_bus(IF_SCSI);
     while (n >= 0) {
-        dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
-        lsi53c8xx_handle_legacy_cmdline(dev);
+        pci_create_simple(pci_bus, -1, "lsi53c895a");
         n--;
     }
 
diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index a31dc32a9f..69b822bfc8 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -349,8 +349,7 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus,
 
     /* SCSI disk setup. */
     if (drive_get_max_bus(IF_SCSI) >= 0) {
-        dev = DEVICE(pci_create_simple(pci_bus, -1, "lsi53c895a"));
-        lsi53c8xx_handle_legacy_cmdline(dev);
+        pci_create_simple(pci_bus, -1, "lsi53c895a");
     }
 
     /* Graphics setup. */
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index fb58c312ac..0e5ff2a75c 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -328,7 +328,6 @@ static void ibm_40p_init(MachineState *machine)
 
         dev = DEVICE(pci_create_simple(pci_bus, PCI_DEVFN(1, 0),
                                        "lsi53c810"));
-        lsi53c8xx_handle_legacy_cmdline(dev);
         qdev_connect_gpio_out(dev, 0, qdev_get_gpio_in(i82378_dev, 13));
 
         /* XXX: s3-trio at PCI_DEVFN(2, 0) */
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 1f728416e2..d632789434 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -2365,6 +2365,7 @@ static void lsi_scsi_realize(PCIDevice *dev, Error **errp)
     QTAILQ_INIT(&s->queue);
 
     scsi_bus_init(&s->bus, sizeof(s->bus), d, &lsi_scsi_info);
+    scsi_bus_legacy_handle_cmdline(&s->bus);
 }
 
 static void lsi_scsi_exit(PCIDevice *dev)
@@ -2422,10 +2423,3 @@ static void lsi53c895a_register_types(void)
 }
 
 type_init(lsi53c895a_register_types)
-
-void lsi53c8xx_handle_legacy_cmdline(DeviceState *lsi_dev)
-{
-    LSIState *s = LSI53C895A(lsi_dev);
-
-    scsi_bus_legacy_handle_cmdline(&s->bus);
-}
-- 
2.45.2

[PATCH 3/3] hw/scsi/esp: Call scsi_bus_legacy_handle_cmdline() once

[Philippe Mathieu-Daudé]

Call scsi_bus_legacy_handle_cmdline() once in the DeviceRealize
handler, just after scsi_bus_init().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/m68k/next-cube.c | 2 --
 hw/m68k/q800.c      | 2 --
 hw/mips/jazz.c      | 2 --
 hw/scsi/esp.c       | 1 +
 hw/sparc/sun4m.c    | 1 -
 5 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/m68k/next-cube.c b/hw/m68k/next-cube.c
index 08886d432c..f0fdbef7ae 100644
--- a/hw/m68k/next-cube.c
+++ b/hw/m68k/next-cube.c
@@ -851,8 +851,6 @@ static void next_scsi_init(DeviceState *pcdev)
 
     next_pc->scsi_reset = qdev_get_gpio_in(dev, 0);
     next_pc->scsi_dma = qdev_get_gpio_in(dev, 1);
-
-    scsi_bus_legacy_handle_cmdline(&esp->bus);
 }
 
 static void next_escc_init(DeviceState *pcdev)
diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 556604e1dc..816a662fd9 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -487,8 +487,6 @@ static void q800_machine_init(MachineState *machine)
     memory_region_add_subregion(&m->macio, ESP_PDMA - IO_BASE,
                                 sysbus_mmio_get_region(sysbus, 1));
 
-    scsi_bus_legacy_handle_cmdline(&esp->bus);
-
     /* Apple Sound Chip */
 
     object_initialize_child(OBJECT(machine), "asc", &m->asc, TYPE_ASC);
diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
index 0e43c9f0ba..71badb0616 100644
--- a/hw/mips/jazz.c
+++ b/hw/mips/jazz.c
@@ -347,8 +347,6 @@ static void mips_jazz_init(MachineState *machine,
     sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(rc4030, 5));
     sysbus_mmio_map(sysbus, 0, 0x80002000);
 
-    scsi_bus_legacy_handle_cmdline(&esp->bus);
-
     /* Floppy */
     for (n = 0; n < MAX_FD; n++) {
         fds[n] = drive_get(IF_FLOPPY, 0, n);
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ac841dc32e..a1d2f6f380 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1542,6 +1542,7 @@ static void sysbus_esp_realize(DeviceState *dev, Error **errp)
     qdev_init_gpio_in(dev, sysbus_esp_gpio_demux, 2);
 
     scsi_bus_init(&s->bus, sizeof(s->bus), dev, &esp_scsi_info);
+    scsi_bus_legacy_handle_cmdline(&s->bus);
 }
 
 static void sysbus_esp_hard_reset(DeviceState *dev)
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index d52e6a7213..26e0af43bf 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -338,7 +338,6 @@ static void *sparc32_dma_init(hwaddr dma_base,
     sysbus_mmio_map(SYS_BUS_DEVICE(dma), 0, dma_base);
 
     sysbus_mmio_map(SYS_BUS_DEVICE(esp), 0, esp_base);
-    scsi_bus_legacy_handle_cmdline(&esp->esp.bus);
 
     sysbus_mmio_map(SYS_BUS_DEVICE(lance), 0, le_base);
 
-- 
2.45.2

Re: [PATCH 0/3] hw/scsi: Cleanup around scsi_bus_legacy_handle_cmdline()

[Thomas Huth]

On 22/11/2024 12.19, Philippe Mathieu-Daudé wrote:
> When a device model requires legacy command line handling,
> call scsi_bus_legacy_handle_cmdline() in its realize handler
> instead of having each user call it.
> 
> This applies to:
>   - spapr_vscsi
>   - lsi53c810 / lsi53c895a
>   - sysbus_esp
> 
> Note, scsi_bus_legacy_handle_cmdline() prototype could be
> made private to hw/scsi/ to restrict its use to scsi device
> implementations.

Not sure whether this is the right way to go ... shouldn't the handling of 
the legacy command line be rather part of the machine than being part of the 
SCSI controller device? Imagine for example a machine that has multiple, 
different SCSI controllers - I think you'd rather want to let the machine 
decide where the legacy devices should be grabbed from than having the SCSI 
controller devices fight for them...?

  Thomas

Re: [PATCH 0/3] hw/scsi: Cleanup around scsi_bus_legacy_handle_cmdline()

[Paolo Bonzini]

On 11/22/24 13:37, Thomas Huth wrote:
> On 22/11/2024 12.19, Philippe Mathieu-Daudé wrote:
>> When a device model requires legacy command line handling,
>> call scsi_bus_legacy_handle_cmdline() in its realize handler
>> instead of having each user call it.
>>
>> This applies to:
>>   - spapr_vscsi
>>   - lsi53c810 / lsi53c895a
>>   - sysbus_esp
>>
>> Note, scsi_bus_legacy_handle_cmdline() prototype could be
>> made private to hw/scsi/ to restrict its use to scsi device
>> implementations.
> 
> Not sure whether this is the right way to go ... shouldn't the handling 
> of the legacy command line be rather part of the machine than being part 
> of the SCSI controller device? Imagine for example a machine that has 
> multiple, different SCSI controllers - I think you'd rather want to let 
> the machine decide where the legacy devices should be grabbed from than 
> having the SCSI controller devices fight for them...?

I agree that it should be done in the machines generally:

1) if the machine creates a SCSI controller, it should then call
scsi_bus_legacy_handle_cmdline().  This is the case for esp and for
spapr-vscsi (so spapr_vscsi_create() could be inlined in its caller).

2) lsi53c* is the odd one out because it was "the" way to add "-drive
if=scsi" to the PC machine.

For case (2) it's okay to call it in the realize function, I guess.
However, let's only process -drive if=scsi for devices added on the
command-line.

The LSI HBA should not call  scsi_bus_legacy_handle_cmdline() if
dev->hotplugged.  I think we can do it without a deprecation period,
and in fact assert that !phase_check(PHASE_MACHINE_READY) in
scsi_bus_legacy_handle_cmdline().

Paolo