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

Message

Philippe Mathieu-Daudé Nov. 22, 2024, 11:19 a.m. UTC

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(-)

Comments

Thomas Huth Nov. 22, 2024, 12:37 p.m. UTC | #1

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

Paolo Bonzini Nov. 22, 2024, 12:53 p.m. UTC | #2

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