drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Explicit is better than implicit: use drive_get() directly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/sysemu/blockdev.h           |  1 -
 blockdev.c                          | 10 ----------
 hw/arm/aspeed.c                     | 21 +++++++++++++--------
 hw/arm/cubieboard.c                 |  2 +-
 hw/arm/imx25_pdk.c                  |  2 +-
 hw/arm/integratorcp.c               |  2 +-
 hw/arm/mcimx6ul-evk.c               |  2 +-
 hw/arm/mcimx7d-sabre.c              |  2 +-
 hw/arm/msf2-som.c                   |  2 +-
 hw/arm/npcm7xx_boards.c             |  6 +++---
 hw/arm/orangepi.c                   |  2 +-
 hw/arm/raspi.c                      |  2 +-
 hw/arm/realview.c                   |  2 +-
 hw/arm/sabrelite.c                  |  2 +-
 hw/arm/versatilepb.c                |  4 ++--
 hw/arm/vexpress.c                   |  6 +++---
 hw/arm/xilinx_zynq.c                | 16 +++++++++-------
 hw/arm/xlnx-versal-virt.c           |  3 ++-
 hw/arm/xlnx-zcu102.c                |  6 +++---
 hw/microblaze/petalogix_ml605_mmu.c |  2 +-
 hw/misc/sifive_u_otp.c              |  2 +-
 hw/riscv/microchip_pfsoc.c          |  2 +-
 hw/sparc64/niagara.c                |  2 +-
 23 files changed, 49 insertions(+), 52 deletions(-)

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 32c2d6023c..a750f99b79 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -50,7 +50,6 @@ void drive_check_orphaned(void);
 DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
 int drive_get_max_bus(BlockInterfaceType type);
 int drive_get_max_devs(BlockInterfaceType type);
-DriveInfo *drive_get_next(BlockInterfaceType type);
 
 QemuOpts *drive_def(const char *optstr);
 QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
diff --git a/blockdev.c b/blockdev.c
index b35072644e..0eb2823b1b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -303,16 +303,6 @@ int drive_get_max_bus(BlockInterfaceType type)
     return max_bus;
 }
 
-/* Get a block device.  This should only be used for single-drive devices
-   (e.g. SD/Floppy/MTD).  Multi-disk devices (scsi/ide) should use the
-   appropriate bus.  */
-DriveInfo *drive_get_next(BlockInterfaceType type)
-{
-    static int next_block_unit[IF_COUNT];
-
-    return drive_get(type, 0, next_block_unit[type]++);
-}
-
 static void bdrv_format_print(void *opaque, const char *name)
 {
     qemu_printf(" %s", name);
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a77f46b3ad..cf20ae0db5 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -284,12 +284,13 @@ static void write_boot_rom(DriveInfo *dinfo, hwaddr addr, size_t rom_size,
 }
 
 static void aspeed_board_init_flashes(AspeedSMCState *s,
-                                      const char *flashtype)
+                                      const char *flashtype,
+                                      int unit0)
 {
     int i ;
 
     for (i = 0; i < s->num_cs; ++i) {
-        DriveInfo *dinfo = drive_get_next(IF_MTD);
+        DriveInfo *dinfo = drive_get(IF_MTD, 0, unit0 + i);
         qemu_irq cs_line;
         DeviceState *dev;
 
@@ -382,10 +383,12 @@ static void aspeed_machine_init(MachineState *machine)
                           "max_ram", max_ram_size  - machine->ram_size);
     memory_region_add_subregion(&bmc->ram_container, machine->ram_size, &bmc->max_ram);
 
-    aspeed_board_init_flashes(&bmc->soc.fmc, bmc->fmc_model ?
-                              bmc->fmc_model : amc->fmc_model);
-    aspeed_board_init_flashes(&bmc->soc.spi[0], bmc->spi_model ?
-                              bmc->spi_model : amc->spi_model);
+    aspeed_board_init_flashes(&bmc->soc.fmc,
+                              bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
+                              0);
+    aspeed_board_init_flashes(&bmc->soc.spi[0],
+                              bmc->spi_model ? bmc->spi_model : amc->spi_model,
+                              bmc->soc.fmc.num_cs);
 
     /* Install first FMC flash content as a boot rom. */
     if (drive0) {
@@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
     }
 
     for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
-        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
+        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
+                           drive_get(IF_SD, 0, i));
     }
 
     if (bmc->soc.emmc.num_slots) {
-        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
+        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
+                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
     }
 
     arm_load_kernel(ARM_CPU(first_cpu), machine, &aspeed_board_binfo);
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index 294ba5de6e..5e3372a3c7 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -81,7 +81,7 @@ static void cubieboard_init(MachineState *machine)
     }
 
     /* Retrieve SD bus */
-    di = drive_get_next(IF_SD);
+    di = drive_get(IF_SD, 0, 0);
     blk = di ? blk_by_legacy_dinfo(di) : NULL;
     bus = qdev_get_child_bus(DEVICE(a10), "sd-bus");
 
diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
index bd16acd4d9..6dff000163 100644
--- a/hw/arm/imx25_pdk.c
+++ b/hw/arm/imx25_pdk.c
@@ -123,7 +123,7 @@ static void imx25_pdk_init(MachineState *machine)
         DriveInfo *di;
         BlockBackend *blk;
 
-        di = drive_get_next(IF_SD);
+        di = drive_get(IF_SD, 0, i);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
         bus = qdev_get_child_bus(DEVICE(&s->soc.esdhc[i]), "sd-bus");
         carddev = qdev_new(TYPE_SD_CARD);
diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 16e8985953..b109ece3ae 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -649,7 +649,7 @@ static void integratorcp_init(MachineState *machine)
                           qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_WPROT, 0));
     qdev_connect_gpio_out_named(dev, "card-inserted", 0,
                           qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_CARDIN, 0));
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 0);
     if (dinfo) {
         DeviceState *card;
 
diff --git a/hw/arm/mcimx6ul-evk.c b/hw/arm/mcimx6ul-evk.c
index 77fae874b1..28b4886f48 100644
--- a/hw/arm/mcimx6ul-evk.c
+++ b/hw/arm/mcimx6ul-evk.c
@@ -52,7 +52,7 @@ static void mcimx6ul_evk_init(MachineState *machine)
         DriveInfo *di;
         BlockBackend *blk;
 
-        di = drive_get_next(IF_SD);
+        di = drive_get(IF_SD, 0, i);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
         bus = qdev_get_child_bus(DEVICE(&s->usdhc[i]), "sd-bus");
         carddev = qdev_new(TYPE_SD_CARD);
diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
index 935d4b0f1c..50a5ecde31 100644
--- a/hw/arm/mcimx7d-sabre.c
+++ b/hw/arm/mcimx7d-sabre.c
@@ -52,7 +52,7 @@ static void mcimx7d_sabre_init(MachineState *machine)
         DriveInfo *di;
         BlockBackend *blk;
 
-        di = drive_get_next(IF_SD);
+        di = drive_get(IF_SD, 0, i);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
         bus = qdev_get_child_bus(DEVICE(&s->usdhc[i]), "sd-bus");
         carddev = qdev_new(TYPE_SD_CARD);
diff --git a/hw/arm/msf2-som.c b/hw/arm/msf2-som.c
index 396e8b9913..d9f881690e 100644
--- a/hw/arm/msf2-som.c
+++ b/hw/arm/msf2-som.c
@@ -45,7 +45,7 @@ static void emcraft_sf2_s2s010_init(MachineState *machine)
     DeviceState *spi_flash;
     MSF2State *soc;
     MachineClass *mc = MACHINE_GET_CLASS(machine);
-    DriveInfo *dinfo = drive_get_next(IF_MTD);
+    DriveInfo *dinfo = drive_get(IF_MTD, 0, 0);
     qemu_irq cs_line;
     BusState *spi_bus;
     MemoryRegion *sysmem = get_system_memory();
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index dec7d16ae5..d8a49e4e85 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -84,9 +84,9 @@ static void npcm7xx_connect_dram(NPCM7xxState *soc, MemoryRegion *dram)
                              &error_abort);
 }
 
-static void sdhci_attach_drive(SDHCIState *sdhci)
+static void sdhci_attach_drive(SDHCIState *sdhci, int unit)
 {
-        DriveInfo *di = drive_get_next(IF_SD);
+        DriveInfo *di = drive_get(IF_SD, 0, unit);
         BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
 
         BusState *bus = qdev_get_child_bus(DEVICE(sdhci), "sd-bus");
@@ -374,7 +374,7 @@ static void quanta_gbs_init(MachineState *machine)
                           drive_get(IF_MTD, 0, 0));
 
     quanta_gbs_i2c_init(soc);
-    sdhci_attach_drive(&soc->mmc.sdhci);
+    sdhci_attach_drive(&soc->mmc.sdhci, 0);
     npcm7xx_load_kernel(machine, soc);
 }
 
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index 0cf9895ce7..e796382236 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -85,7 +85,7 @@ static void orangepi_init(MachineState *machine)
     qdev_realize(DEVICE(h3), NULL, &error_abort);
 
     /* Retrieve SD bus */
-    di = drive_get_next(IF_SD);
+    di = drive_get(IF_SD, 0, 0);
     blk = di ? blk_by_legacy_dinfo(di) : NULL;
     bus = qdev_get_child_bus(DEVICE(h3), "sd-bus");
 
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 146d35382b..b4dd6c1e99 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -284,7 +284,7 @@ static void raspi_machine_init(MachineState *machine)
     qdev_realize(DEVICE(&s->soc), NULL, &error_fatal);
 
     /* Create and plug in the SD cards */
-    di = drive_get_next(IF_SD);
+    di = drive_get(IF_SD, 0, 0);
     blk = di ? blk_by_legacy_dinfo(di) : NULL;
     bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
     if (bus == NULL) {
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index 1c54316ba3..ddc70b54a5 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -237,7 +237,7 @@ static void realview_init(MachineState *machine,
         qemu_irq_invert(qdev_get_gpio_in(gpio2, 0)));
     qdev_connect_gpio_out_named(dev, "card-read-only", 0, mmc_irq[0]);
     qdev_connect_gpio_out_named(dev, "card-inserted", 0, mmc_irq[1]);
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 0);
     if (dinfo) {
         DeviceState *card;
 
diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
index 553608e583..cce49aa25c 100644
--- a/hw/arm/sabrelite.c
+++ b/hw/arm/sabrelite.c
@@ -76,7 +76,7 @@ static void sabrelite_init(MachineState *machine)
             if (spi_bus) {
                 DeviceState *flash_dev;
                 qemu_irq cs_line;
-                DriveInfo *dinfo = drive_get_next(IF_MTD);
+                DriveInfo *dinfo = drive_get(IF_MTD, 0, 0);
 
                 flash_dev = qdev_new("sst25vf016b");
                 if (dinfo) {
diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
index 575399c4fc..ecc1f6cf74 100644
--- a/hw/arm/versatilepb.c
+++ b/hw/arm/versatilepb.c
@@ -310,7 +310,7 @@ static void versatile_init(MachineState *machine, int board_id)
     qdev_connect_gpio_out(sysctl, 0, qdev_get_gpio_in(dev, 0));
 
     dev = sysbus_create_varargs("pl181", 0x10005000, sic[22], sic[1], NULL);
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 0);
     if (dinfo) {
         DeviceState *card;
 
@@ -322,7 +322,7 @@ static void versatile_init(MachineState *machine, int board_id)
     }
 
     dev = sysbus_create_varargs("pl181", 0x1000b000, sic[23], sic[2], NULL);
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 1);
     if (dinfo) {
         DeviceState *card;
 
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 58481c0762..966758cf82 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -625,7 +625,7 @@ static void vexpress_common_init(MachineState *machine)
                           qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_WPROT));
     qdev_connect_gpio_out_named(dev, "card-inserted", 0,
                           qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_CARDIN));
-    dinfo = drive_get_next(IF_SD);
+    dinfo = drive_get(IF_SD, 0, 0);
     if (dinfo) {
         DeviceState *card;
 
@@ -657,7 +657,7 @@ static void vexpress_common_init(MachineState *machine)
 
     sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
 
-    dinfo = drive_get_next(IF_PFLASH);
+    dinfo = drive_get(IF_PFLASH, 0, 0);
     pflash0 = ve_pflash_cfi01_register(map[VE_NORFLASH0], "vexpress.flash0",
                                        dinfo);
     if (!pflash0) {
@@ -673,7 +673,7 @@ static void vexpress_common_init(MachineState *machine)
         memory_region_add_subregion(sysmem, map[VE_NORFLASHALIAS], flashalias);
     }
 
-    dinfo = drive_get_next(IF_PFLASH);
+    dinfo = drive_get(IF_PFLASH, 0, 1);
     if (!ve_pflash_cfi01_register(map[VE_NORFLASH1], "vexpress.flash1",
                                   dinfo)) {
         error_report("vexpress: error registering flash 1");
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 69c333e91b..50e7268396 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -125,9 +125,10 @@ static void gem_init(NICInfo *nd, uint32_t base, qemu_irq irq)
     sysbus_connect_irq(s, 0, irq);
 }
 
-static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
-                                         bool is_qspi)
+static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
+                                        bool is_qspi, int unit0)
 {
+    int unit = unit0;
     DeviceState *dev;
     SysBusDevice *busdev;
     SSIBus *spi;
@@ -156,7 +157,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
         spi = (SSIBus *)qdev_get_child_bus(dev, bus_name);
 
         for (j = 0; j < num_ss; ++j) {
-            DriveInfo *dinfo = drive_get_next(IF_MTD);
+            DriveInfo *dinfo = drive_get(IF_MTD, 0, unit++);
             flash_dev = qdev_new("n25q128");
             if (dinfo) {
                 qdev_prop_set_drive_err(flash_dev, "drive",
@@ -170,6 +171,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
         }
     }
 
+    return unit;
 }
 
 static void zynq_init(MachineState *machine)
@@ -247,9 +249,9 @@ static void zynq_init(MachineState *machine)
         pic[n] = qdev_get_gpio_in(dev, n);
     }
 
-    zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET], false);
-    zynq_init_spi_flashes(0xE0007000, pic[81-IRQ_OFFSET], false);
-    zynq_init_spi_flashes(0xE000D000, pic[51-IRQ_OFFSET], true);
+    n = zynq_init_spi_flashes(0xE0006000, pic[58 - IRQ_OFFSET], false, 0);
+    n = zynq_init_spi_flashes(0xE0007000, pic[81 - IRQ_OFFSET], false, n);
+    n = zynq_init_spi_flashes(0xE000D000, pic[51 - IRQ_OFFSET], true, n);
 
     sysbus_create_simple(TYPE_CHIPIDEA, 0xE0002000, pic[53 - IRQ_OFFSET]);
     sysbus_create_simple(TYPE_CHIPIDEA, 0xE0003000, pic[76 - IRQ_OFFSET]);
@@ -298,7 +300,7 @@ static void zynq_init(MachineState *machine)
         sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, hci_addr);
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[hci_irq - IRQ_OFFSET]);
 
-        di = drive_get_next(IF_SD);
+        di = drive_get(IF_SD, 0, n);
         blk = di ? blk_by_legacy_dinfo(di) : NULL;
         carddev = qdev_new(TYPE_SD_CARD);
         qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal);
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index d2f55e29b6..0c5edc898e 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -669,7 +669,8 @@ static void versal_virt_init(MachineState *machine)
 
     /* Plugin SD cards.  */
     for (i = 0; i < ARRAY_SIZE(s->soc.pmc.iou.sd); i++) {
-        sd_plugin_card(&s->soc.pmc.iou.sd[i], drive_get_next(IF_SD));
+        sd_plugin_card(&s->soc.pmc.iou.sd[i],
+                       drive_get(IF_SD, 0, i));
     }
 
     s->binfo.ram_size = machine->ram_size;
diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index 3dc2b5e8ca..45eb19ab3b 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -169,7 +169,7 @@ static void xlnx_zcu102_init(MachineState *machine)
     /* Create and plug in the SD cards */
     for (i = 0; i < XLNX_ZYNQMP_NUM_SDHCI; i++) {
         BusState *bus;
-        DriveInfo *di = drive_get_next(IF_SD);
+        DriveInfo *di = drive_get(IF_SD, 0, i);
         BlockBackend *blk = di ? blk_by_legacy_dinfo(di) : NULL;
         DeviceState *carddev;
         char *bus_name;
@@ -190,7 +190,7 @@ static void xlnx_zcu102_init(MachineState *machine)
         BusState *spi_bus;
         DeviceState *flash_dev;
         qemu_irq cs_line;
-        DriveInfo *dinfo = drive_get_next(IF_MTD);
+        DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
         gchar *bus_name = g_strdup_printf("spi%d", i);
 
         spi_bus = qdev_get_child_bus(DEVICE(&s->soc), bus_name);
@@ -212,7 +212,7 @@ static void xlnx_zcu102_init(MachineState *machine)
         BusState *spi_bus;
         DeviceState *flash_dev;
         qemu_irq cs_line;
-        DriveInfo *dinfo = drive_get_next(IF_MTD);
+        DriveInfo *dinfo = drive_get(IF_MTD, 0, XLNX_ZYNQMP_NUM_SPIS + i);
         int bus = i / XLNX_ZYNQMP_NUM_QSPI_BUS_CS;
         gchar *bus_name = g_strdup_printf("qspi%d", bus);
 
diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index 159db6cbe2..a24fadddca 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -183,7 +183,7 @@ petalogix_ml605_init(MachineState *machine)
         spi = (SSIBus *)qdev_get_child_bus(dev, "spi");
 
         for (i = 0; i < NUM_SPI_FLASHES; i++) {
-            DriveInfo *dinfo = drive_get_next(IF_MTD);
+            DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
             qemu_irq cs_line;
 
             dev = qdev_new("n25q128");
diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
index 18aa0bd55d..5d5a8c8a90 100644
--- a/hw/misc/sifive_u_otp.c
+++ b/hw/misc/sifive_u_otp.c
@@ -209,7 +209,7 @@ static void sifive_u_otp_realize(DeviceState *dev, Error **errp)
                           TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
 
-    dinfo = drive_get_next(IF_NONE);
+    dinfo = drive_get(IF_NONE, 0, 0);
     if (dinfo) {
         int ret;
         uint64_t perm;
diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
index 57d779fb55..d1d065efbc 100644
--- a/hw/riscv/microchip_pfsoc.c
+++ b/hw/riscv/microchip_pfsoc.c
@@ -458,7 +458,7 @@ static void microchip_icicle_kit_machine_init(MachineState *machine)
     target_ulong firmware_end_addr, kernel_start_addr;
     uint64_t kernel_entry;
     uint32_t fdt_load_addr;
-    DriveInfo *dinfo = drive_get_next(IF_SD);
+    DriveInfo *dinfo = drive_get(IF_SD, 0, 0);
 
     /* Sanity check on RAM size */
     if (machine->ram_size < mc->default_ram_size) {
diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
index f3e42d0326..ccad2c43a3 100644
--- a/hw/sparc64/niagara.c
+++ b/hw/sparc64/niagara.c
@@ -98,7 +98,7 @@ static void add_rom_or_fail(const char *file, const hwaddr addr)
 static void niagara_init(MachineState *machine)
 {
     NiagaraBoardState *s = g_new(NiagaraBoardState, 1);
-    DriveInfo *dinfo = drive_get_next(IF_PFLASH);
+    DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
     MemoryRegion *sysmem = get_system_memory();
 
     /* init CPUs */
-- 
2.31.1

On Mon, 15 Nov 2021 at 12:55, Markus Armbruster <armbru@redhat.com> wrote:
>
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> Explicit is better than implicit: use drive_get() directly.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/sysemu/blockdev.h           |  1 -
>  blockdev.c                          | 10 ----------
>  hw/arm/aspeed.c                     | 21 +++++++++++++--------
>  hw/arm/cubieboard.c                 |  2 +-
>  hw/arm/imx25_pdk.c                  |  2 +-
>  hw/arm/integratorcp.c               |  2 +-
>  hw/arm/mcimx6ul-evk.c               |  2 +-
>  hw/arm/mcimx7d-sabre.c              |  2 +-
>  hw/arm/msf2-som.c                   |  2 +-
>  hw/arm/npcm7xx_boards.c             |  6 +++---
>  hw/arm/orangepi.c                   |  2 +-
>  hw/arm/raspi.c                      |  2 +-
>  hw/arm/realview.c                   |  2 +-
>  hw/arm/sabrelite.c                  |  2 +-
>  hw/arm/versatilepb.c                |  4 ++--
>  hw/arm/vexpress.c                   |  6 +++---
>  hw/arm/xilinx_zynq.c                | 16 +++++++++-------
>  hw/arm/xlnx-versal-virt.c           |  3 ++-
>  hw/arm/xlnx-zcu102.c                |  6 +++---
>  hw/microblaze/petalogix_ml605_mmu.c |  2 +-
>  hw/misc/sifive_u_otp.c              |  2 +-
>  hw/riscv/microchip_pfsoc.c          |  2 +-
>  hw/sparc64/niagara.c                |  2 +-
>  23 files changed, 49 insertions(+), 52 deletions(-)

This would be easier to review if it didn't try to change all of
these board/SoC models at once. Each one of them is entirely
separate review work.

-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 15 Nov 2021 at 12:55, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> drive_get_next() is basically a bad idea.  It returns the "next" block
>> backend of a certain interface type.  "Next" means bus=0,unit=N, where
>> subsequent calls count N up from zero, per interface type.
>>
>> This lets you define unit numbers implicitly by execution order.  If the
>> order changes, or new calls appear "in the middle", unit numbers change.
>> ABI break.  Hard to spot in review.
>>
>> Explicit is better than implicit: use drive_get() directly.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/sysemu/blockdev.h           |  1 -
>>  blockdev.c                          | 10 ----------
>>  hw/arm/aspeed.c                     | 21 +++++++++++++--------
>>  hw/arm/cubieboard.c                 |  2 +-
>>  hw/arm/imx25_pdk.c                  |  2 +-
>>  hw/arm/integratorcp.c               |  2 +-
>>  hw/arm/mcimx6ul-evk.c               |  2 +-
>>  hw/arm/mcimx7d-sabre.c              |  2 +-
>>  hw/arm/msf2-som.c                   |  2 +-
>>  hw/arm/npcm7xx_boards.c             |  6 +++---
>>  hw/arm/orangepi.c                   |  2 +-
>>  hw/arm/raspi.c                      |  2 +-
>>  hw/arm/realview.c                   |  2 +-
>>  hw/arm/sabrelite.c                  |  2 +-
>>  hw/arm/versatilepb.c                |  4 ++--
>>  hw/arm/vexpress.c                   |  6 +++---
>>  hw/arm/xilinx_zynq.c                | 16 +++++++++-------
>>  hw/arm/xlnx-versal-virt.c           |  3 ++-
>>  hw/arm/xlnx-zcu102.c                |  6 +++---
>>  hw/microblaze/petalogix_ml605_mmu.c |  2 +-
>>  hw/misc/sifive_u_otp.c              |  2 +-
>>  hw/riscv/microchip_pfsoc.c          |  2 +-
>>  hw/sparc64/niagara.c                |  2 +-
>>  23 files changed, 49 insertions(+), 52 deletions(-)
>
> This would be easier to review if it didn't try to change all of
> these board/SoC models at once. Each one of them is entirely
> separate review work.

Happy to split, but first I'd like to get some advice on the part I'm
unsure about; see my cover letter.

On 11/15/21 13:55, Markus Armbruster wrote:
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
> 
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
> 
> Explicit is better than implicit: use drive_get() directly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/sysemu/blockdev.h           |  1 -
>  blockdev.c                          | 10 ----------
>  hw/arm/aspeed.c                     | 21 +++++++++++++--------
>  hw/arm/cubieboard.c                 |  2 +-
>  hw/arm/imx25_pdk.c                  |  2 +-
>  hw/arm/integratorcp.c               |  2 +-
>  hw/arm/mcimx6ul-evk.c               |  2 +-
>  hw/arm/mcimx7d-sabre.c              |  2 +-
>  hw/arm/msf2-som.c                   |  2 +-
>  hw/arm/npcm7xx_boards.c             |  6 +++---
>  hw/arm/orangepi.c                   |  2 +-
>  hw/arm/raspi.c                      |  2 +-
>  hw/arm/realview.c                   |  2 +-
>  hw/arm/sabrelite.c                  |  2 +-
>  hw/arm/versatilepb.c                |  4 ++--
>  hw/arm/vexpress.c                   |  6 +++---
>  hw/arm/xilinx_zynq.c                | 16 +++++++++-------
>  hw/arm/xlnx-versal-virt.c           |  3 ++-
>  hw/arm/xlnx-zcu102.c                |  6 +++---
>  hw/microblaze/petalogix_ml605_mmu.c |  2 +-
>  hw/misc/sifive_u_otp.c              |  2 +-
>  hw/riscv/microchip_pfsoc.c          |  2 +-
>  hw/sparc64/niagara.c                |  2 +-
>  23 files changed, 49 insertions(+), 52 deletions(-)

> @@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
>      }
>  
>      for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
> -        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
> +                           drive_get(IF_SD, 0, i));

If we put SD on bus #0, ...

>      }
>  
>      if (bmc->soc.emmc.num_slots) {
> -        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
> +                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));

... we'd want to put eMMC on bus #1, but I see having eMMC cards on a
IF_SD bus as a bug, since these cards are soldered on the board.

> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -625,7 +625,7 @@ static void vexpress_common_init(MachineState *machine)
>                            qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_WPROT));
>      qdev_connect_gpio_out_named(dev, "card-inserted", 0,
>                            qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_CARDIN));
> -    dinfo = drive_get_next(IF_SD);
> +    dinfo = drive_get(IF_SD, 0, 0);

Can we have one interface refactor per patch (IF_SD, IF_PFLASH, IF_MTD...)?

> @@ -657,7 +657,7 @@ static void vexpress_common_init(MachineState *machine)
>  
>      sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
>  
> -    dinfo = drive_get_next(IF_PFLASH);
> +    dinfo = drive_get(IF_PFLASH, 0, 0);

> -static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
> -                                         bool is_qspi)
> +static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
> +                                        bool is_qspi, int unit0)
>  {
> +    int unit = unit0;
>      DeviceState *dev;
>      SysBusDevice *busdev;
>      SSIBus *spi;
> @@ -156,7 +157,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>          spi = (SSIBus *)qdev_get_child_bus(dev, bus_name);
>  
>          for (j = 0; j < num_ss; ++j) {
> -            DriveInfo *dinfo = drive_get_next(IF_MTD);
> +            DriveInfo *dinfo = drive_get(IF_MTD, 0, unit++);

> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 3dc2b5e8ca..45eb19ab3b 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -190,7 +190,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>          BusState *spi_bus;
>          DeviceState *flash_dev;
>          qemu_irq cs_line;
> -        DriveInfo *dinfo = drive_get_next(IF_MTD);
> +        DriveInfo *dinfo = drive_get(IF_MTD, 0, i);

If this is bus #0, ...

>          gchar *bus_name = g_strdup_printf("spi%d", i);
>  
>          spi_bus = qdev_get_child_bus(DEVICE(&s->soc), bus_name);
> @@ -212,7 +212,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>          BusState *spi_bus;
>          DeviceState *flash_dev;
>          qemu_irq cs_line;
> -        DriveInfo *dinfo = drive_get_next(IF_MTD);
> +        DriveInfo *dinfo = drive_get(IF_MTD, 0, XLNX_ZYNQMP_NUM_SPIS + i);

... I'd expect we use bus #1 here (different connector on the board).

>          int bus = i / XLNX_ZYNQMP_NUM_QSPI_BUS_CS;
>          gchar *bus_name = g_strdup_printf("qspi%d", bus);

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 11/15/21 13:55, Markus Armbruster wrote:
>> drive_get_next() is basically a bad idea.  It returns the "next" block
>> backend of a certain interface type.  "Next" means bus=0,unit=N, where
>> subsequent calls count N up from zero, per interface type.
>> 
>> This lets you define unit numbers implicitly by execution order.  If the
>> order changes, or new calls appear "in the middle", unit numbers change.
>> ABI break.  Hard to spot in review.
>> 
>> Explicit is better than implicit: use drive_get() directly.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/sysemu/blockdev.h           |  1 -
>>  blockdev.c                          | 10 ----------
>>  hw/arm/aspeed.c                     | 21 +++++++++++++--------
>>  hw/arm/cubieboard.c                 |  2 +-
>>  hw/arm/imx25_pdk.c                  |  2 +-
>>  hw/arm/integratorcp.c               |  2 +-
>>  hw/arm/mcimx6ul-evk.c               |  2 +-
>>  hw/arm/mcimx7d-sabre.c              |  2 +-
>>  hw/arm/msf2-som.c                   |  2 +-
>>  hw/arm/npcm7xx_boards.c             |  6 +++---
>>  hw/arm/orangepi.c                   |  2 +-
>>  hw/arm/raspi.c                      |  2 +-
>>  hw/arm/realview.c                   |  2 +-
>>  hw/arm/sabrelite.c                  |  2 +-
>>  hw/arm/versatilepb.c                |  4 ++--
>>  hw/arm/vexpress.c                   |  6 +++---
>>  hw/arm/xilinx_zynq.c                | 16 +++++++++-------
>>  hw/arm/xlnx-versal-virt.c           |  3 ++-
>>  hw/arm/xlnx-zcu102.c                |  6 +++---
>>  hw/microblaze/petalogix_ml605_mmu.c |  2 +-
>>  hw/misc/sifive_u_otp.c              |  2 +-
>>  hw/riscv/microchip_pfsoc.c          |  2 +-
>>  hw/sparc64/niagara.c                |  2 +-
>>  23 files changed, 49 insertions(+), 52 deletions(-)
>
>> @@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
>>      }
>>  
>>      for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
>> -        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
>> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
>> +                           drive_get(IF_SD, 0, i));
>
> If we put SD on bus #0, ...
>
>>      }
>>  
>>      if (bmc->soc.emmc.num_slots) {
>> -        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
>> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
>> +                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
>
> ... we'd want to put eMMC on bus #1

Using separate buses for different kinds of devices would be neater, but
it also would be an incompatible change.  This patch keeps existing
bus/unit numbers working.  drive_get_next() can only use bus 0.

>                                      but I see having eMMC cards on a
> IF_SD bus as a bug, since these cards are soldered on the board.

IF_SD is not a bus, it's an "block interface type", which is really just
a user interface thing.

>> --- a/hw/arm/vexpress.c
>> +++ b/hw/arm/vexpress.c
>> @@ -625,7 +625,7 @@ static void vexpress_common_init(MachineState *machine)
>>                            qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_WPROT));
>>      qdev_connect_gpio_out_named(dev, "card-inserted", 0,
>>                            qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_CARDIN));
>> -    dinfo = drive_get_next(IF_SD);
>> +    dinfo = drive_get(IF_SD, 0, 0);
>
> Can we have one interface refactor per patch (IF_SD, IF_PFLASH, IF_MTD...)?

Peter asked for one patch per "board/SoC model".  I'll do whatever helps
reviewers.

>> @@ -657,7 +657,7 @@ static void vexpress_common_init(MachineState *machine)
>>  
>>      sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
>>  
>> -    dinfo = drive_get_next(IF_PFLASH);
>> +    dinfo = drive_get(IF_PFLASH, 0, 0);
>
>> -static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>> -                                         bool is_qspi)
>> +static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>> +                                        bool is_qspi, int unit0)
>>  {
>> +    int unit = unit0;
>>      DeviceState *dev;
>>      SysBusDevice *busdev;
>>      SSIBus *spi;
>> @@ -156,7 +157,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>>          spi = (SSIBus *)qdev_get_child_bus(dev, bus_name);
>>  
>>          for (j = 0; j < num_ss; ++j) {
>> -            DriveInfo *dinfo = drive_get_next(IF_MTD);
>> +            DriveInfo *dinfo = drive_get(IF_MTD, 0, unit++);
>
>> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
>> index 3dc2b5e8ca..45eb19ab3b 100644
>> --- a/hw/arm/xlnx-zcu102.c
>> +++ b/hw/arm/xlnx-zcu102.c
>> @@ -190,7 +190,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>>          BusState *spi_bus;
>>          DeviceState *flash_dev;
>>          qemu_irq cs_line;
>> -        DriveInfo *dinfo = drive_get_next(IF_MTD);
>> +        DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
>
> If this is bus #0, ...
>
>>          gchar *bus_name = g_strdup_printf("spi%d", i);
>>  
>>          spi_bus = qdev_get_child_bus(DEVICE(&s->soc), bus_name);
>> @@ -212,7 +212,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>>          BusState *spi_bus;
>>          DeviceState *flash_dev;
>>          qemu_irq cs_line;
>> -        DriveInfo *dinfo = drive_get_next(IF_MTD);
>> +        DriveInfo *dinfo = drive_get(IF_MTD, 0, XLNX_ZYNQMP_NUM_SPIS + i);
>
> ... I'd expect we use bus #1 here (different connector on the board).

See above.

>>          int bus = i / XLNX_ZYNQMP_NUM_QSPI_BUS_CS;
>>          gchar *bus_name = g_strdup_printf("qspi%d", bus);

On 11/15/21 16:57, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> On 11/15/21 13:55, Markus Armbruster wrote:
>>> drive_get_next() is basically a bad idea.  It returns the "next" block
>>> backend of a certain interface type.  "Next" means bus=0,unit=N, where
>>> subsequent calls count N up from zero, per interface type.
>>>
>>> This lets you define unit numbers implicitly by execution order.  If the
>>> order changes, or new calls appear "in the middle", unit numbers change.
>>> ABI break.  Hard to spot in review.
>>>
>>> Explicit is better than implicit: use drive_get() directly.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---

>>> @@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
>>>      }
>>>  
>>>      for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
>>> -        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
>>> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
>>> +                           drive_get(IF_SD, 0, i));
>>
>> If we put SD on bus #0, ...
>>
>>>      }
>>>  
>>>      if (bmc->soc.emmc.num_slots) {
>>> -        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
>>> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
>>> +                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
>>
>> ... we'd want to put eMMC on bus #1
> 
> Using separate buses for different kinds of devices would be neater, but
> it also would be an incompatible change.  This patch keeps existing
> bus/unit numbers working.  drive_get_next() can only use bus 0.
> 
>>                                      but I see having eMMC cards on a
>> IF_SD bus as a bug, since these cards are soldered on the board.
> 
> IF_SD is not a bus, it's an "block interface type", which is really just
> a user interface thing.

Why are we discriminating by "block interface type" then?

What is the difference between "block interfaces"? I see a block drive
as a generic unit, usable on multiple hardware devices.

I never really understood how this "block interface type" helps
developers and users. I thought BlockInterfaceType and DriveInfo
were legacy / deprecated APIs we want to get rid of; and we would
come up with a replacement API using BlockDeviceInfo or providing
a BlockFrontend state of the art object.
Anyway, I suppose the explanation is buried in the git history
before the last 8 years. I need to keep reading.

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 11/15/21 16:57, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>> On 11/15/21 13:55, Markus Armbruster wrote:
>>>> drive_get_next() is basically a bad idea.  It returns the "next" block
>>>> backend of a certain interface type.  "Next" means bus=0,unit=N, where
>>>> subsequent calls count N up from zero, per interface type.
>>>>
>>>> This lets you define unit numbers implicitly by execution order.  If the
>>>> order changes, or new calls appear "in the middle", unit numbers change.
>>>> ABI break.  Hard to spot in review.
>>>>
>>>> Explicit is better than implicit: use drive_get() directly.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>
>>>> @@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
>>>>      }
>>>>  
>>>>      for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
>>>> -        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
>>>> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
>>>> +                           drive_get(IF_SD, 0, i));
>>>
>>> If we put SD on bus #0, ...
>>>
>>>>      }
>>>>  
>>>>      if (bmc->soc.emmc.num_slots) {
>>>> -        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
>>>> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
>>>> +                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
>>>
>>> ... we'd want to put eMMC on bus #1
>> 
>> Using separate buses for different kinds of devices would be neater, but
>> it also would be an incompatible change.  This patch keeps existing
>> bus/unit numbers working.  drive_get_next() can only use bus 0.
>> 
>>>                                      but I see having eMMC cards on a
>>> IF_SD bus as a bug, since these cards are soldered on the board.
>> 
>> IF_SD is not a bus, it's an "block interface type", which is really just
>> a user interface thing.
>
> Why are we discriminating by "block interface type" then?
>
> What is the difference between "block interfaces"? I see a block drive
> as a generic unit, usable on multiple hardware devices.
>
> I never really understood how this "block interface type" helps
> developers and users. I thought BlockInterfaceType and DriveInfo
> were legacy / deprecated APIs we want to get rid of; and we would
> come up with a replacement API using BlockDeviceInfo or providing
> a BlockFrontend state of the art object.
> Anyway, I suppose the explanation is buried in the git history
> before the last 8 years. I need to keep reading.

In the beginning (v0.4.2), there was -hda and -hdb, and life was simple.

Then there was -hdc, -hdd, -cdrom (v0.5.1), -fda, -fdb (v0.6.0),
-mtdblock, -sd, -pflash (v0.9.1).

All these options do two things: they create a block backend, and they
request the board to create a certain block frontend for it, similar to
other options of this vintage, like -serial, -parallel, and -net.
Boards generally ignore requests they don't understand, but that's just
sloppiness.

For each set of related options, there was a global variable holding the
requests: bs_table[] for -hda, -hdb, -hdc, -hdd, -cdrom; fd_table[]
-fda, -fdb; mtd_bdrv for -mtd; sd_drv for -ds; pflash_table[] for
-pflash.

The options replaced prior ones, except for -pflash, which appended to
its table.

bs_table[]'s index had a peculiar meaning: it's bus * MAX_IDE_DEVS +
unit.  This ensures that -hda (index 0) goes on IDE bus 0 as unit 0;
-hdb on bus 0, unit 1; -hdc on 1, 0; -hdc on 1, 1.

Life was now complicated enough for a generalization (v0.9.1), so there
was -drive (v0.9.1).  All the variables holding requests were fused into
drives_table[].  Table elements are identified by (type, bus, unit),
where type is an enum whose members correspond to the old global
variables: IF_IDE for bs_table[], IF_FLOPPY for fd_table[], and so
forth.  So:

    -hda becomes type = IF_IDE, bus = 0, unit = 0
    -hdb becomes type = IF_IDE, bus = 0, unit = 1
    ...
    -sd becomes type = IF_SD, bus = 0, unit = 0
    1st -pflash becomes type = IF_PFLASH, bus = 0, unit = 0
    2nd -pflash becomes type = IF_PFLASH, bus = 0, unit = 1
    ...

Other mappings from old to new global variables would have been
possible.  I figure this one was chosen because it comes with a
reasonable user interface.  Identifying block devices by (interface
type, bus, unit) is certainly nicer than by index in bs_table[].  Since
bus and/or unit make sense only with some interface types, they are
optional.

Things calmed down for a couple of years, until -device appeared
(v0.12).  Now we needed a way to define just a backend, without
requesting a frontend from the board.  Instead of inventing a new
option, this became IF_NONE, with meaningless bus and unit.

Over the next years, the block layer outgrew -drive's limited
capabilities to define frontends.  -blockdev appeard (v1.7.0) and
matured over several releases.  I don't remember exactly when it became
stable, relegating -drive if=none to legacy status.

What's *not* legacy is -drive with other interface types, simply because
there is no replacement.  Yet.  We clearly want one.

Questions?

On 11/15/21 16:57, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> On 11/15/21 13:55, Markus Armbruster wrote:
>>> drive_get_next() is basically a bad idea.  It returns the "next" block
>>> backend of a certain interface type.  "Next" means bus=0,unit=N, where
>>> subsequent calls count N up from zero, per interface type.
>>>
>>> This lets you define unit numbers implicitly by execution order.  If the
>>> order changes, or new calls appear "in the middle", unit numbers change.
>>> ABI break.  Hard to spot in review.
>>>
>>> Explicit is better than implicit: use drive_get() directly.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>   include/sysemu/blockdev.h           |  1 -
>>>   blockdev.c                          | 10 ----------
>>>   hw/arm/aspeed.c                     | 21 +++++++++++++--------
>>>   hw/arm/cubieboard.c                 |  2 +-
>>>   hw/arm/imx25_pdk.c                  |  2 +-
>>>   hw/arm/integratorcp.c               |  2 +-
>>>   hw/arm/mcimx6ul-evk.c               |  2 +-
>>>   hw/arm/mcimx7d-sabre.c              |  2 +-
>>>   hw/arm/msf2-som.c                   |  2 +-
>>>   hw/arm/npcm7xx_boards.c             |  6 +++---
>>>   hw/arm/orangepi.c                   |  2 +-
>>>   hw/arm/raspi.c                      |  2 +-
>>>   hw/arm/realview.c                   |  2 +-
>>>   hw/arm/sabrelite.c                  |  2 +-
>>>   hw/arm/versatilepb.c                |  4 ++--
>>>   hw/arm/vexpress.c                   |  6 +++---
>>>   hw/arm/xilinx_zynq.c                | 16 +++++++++-------
>>>   hw/arm/xlnx-versal-virt.c           |  3 ++-
>>>   hw/arm/xlnx-zcu102.c                |  6 +++---
>>>   hw/microblaze/petalogix_ml605_mmu.c |  2 +-
>>>   hw/misc/sifive_u_otp.c              |  2 +-
>>>   hw/riscv/microchip_pfsoc.c          |  2 +-
>>>   hw/sparc64/niagara.c                |  2 +-
>>>   23 files changed, 49 insertions(+), 52 deletions(-)
>>
>>> @@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
>>>       }
>>>   
>>>       for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
>>> -        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
>>> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
>>> +                           drive_get(IF_SD, 0, i));
>>
>> If we put SD on bus #0, ...
>>
>>>       }
>>>   
>>>       if (bmc->soc.emmc.num_slots) {
>>> -        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
>>> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
>>> +                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
>>
>> ... we'd want to put eMMC on bus #1
> 
> Using separate buses for different kinds of devices would be neater, but
> it also would be an incompatible change.  This patch keeps existing
> bus/unit numbers working.  drive_get_next() can only use bus 0.

All Aspeed SoCs have 3 SPI busses, each with multiple CS, and also multiple
sdhci controllers with multiple slots.

How drives are defined for the aspeed machines can/should be improved.
The machine init iterates on the command line drives, attaches the
DriveInfo, in the order found, to a m25p80 device model first and then
follows on with the SD devices. This is fragile clearly and a bus+id
would be most welcome to identify the drive backend.

May be this is a prereq for this patchset ?

Thanks,

C.


> 
>>                                       but I see having eMMC cards on a
>> IF_SD bus as a bug, since these cards are soldered on the board.
> 
> IF_SD is not a bus, it's an "block interface type", which is really just
> a user interface thing.
> 
>>> --- a/hw/arm/vexpress.c
>>> +++ b/hw/arm/vexpress.c
>>> @@ -625,7 +625,7 @@ static void vexpress_common_init(MachineState *machine)
>>>                             qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_WPROT));
>>>       qdev_connect_gpio_out_named(dev, "card-inserted", 0,
>>>                             qdev_get_gpio_in(sysctl, ARM_SYSCTL_GPIO_MMC_CARDIN));
>>> -    dinfo = drive_get_next(IF_SD);
>>> +    dinfo = drive_get(IF_SD, 0, 0);
>>
>> Can we have one interface refactor per patch (IF_SD, IF_PFLASH, IF_MTD...)?
> 
> Peter asked for one patch per "board/SoC model".  I'll do whatever helps
> reviewers.
> 
>>> @@ -657,7 +657,7 @@ static void vexpress_common_init(MachineState *machine)
>>>   
>>>       sysbus_create_simple("pl111", map[VE_CLCD], pic[14]);
>>>   
>>> -    dinfo = drive_get_next(IF_PFLASH);
>>> +    dinfo = drive_get(IF_PFLASH, 0, 0);
>>
>>> -static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>>> -                                         bool is_qspi)
>>> +static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>>> +                                        bool is_qspi, int unit0)
>>>   {
>>> +    int unit = unit0;
>>>       DeviceState *dev;
>>>       SysBusDevice *busdev;
>>>       SSIBus *spi;
>>> @@ -156,7 +157,7 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>>>           spi = (SSIBus *)qdev_get_child_bus(dev, bus_name);
>>>   
>>>           for (j = 0; j < num_ss; ++j) {
>>> -            DriveInfo *dinfo = drive_get_next(IF_MTD);
>>> +            DriveInfo *dinfo = drive_get(IF_MTD, 0, unit++);
>>
>>> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
>>> index 3dc2b5e8ca..45eb19ab3b 100644
>>> --- a/hw/arm/xlnx-zcu102.c
>>> +++ b/hw/arm/xlnx-zcu102.c
>>> @@ -190,7 +190,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>>>           BusState *spi_bus;
>>>           DeviceState *flash_dev;
>>>           qemu_irq cs_line;
>>> -        DriveInfo *dinfo = drive_get_next(IF_MTD);
>>> +        DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
>>
>> If this is bus #0, ...
>>
>>>           gchar *bus_name = g_strdup_printf("spi%d", i);
>>>   
>>>           spi_bus = qdev_get_child_bus(DEVICE(&s->soc), bus_name);
>>> @@ -212,7 +212,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>>>           BusState *spi_bus;
>>>           DeviceState *flash_dev;
>>>           qemu_irq cs_line;
>>> -        DriveInfo *dinfo = drive_get_next(IF_MTD);
>>> +        DriveInfo *dinfo = drive_get(IF_MTD, 0, XLNX_ZYNQMP_NUM_SPIS + i);
>>
>> ... I'd expect we use bus #1 here (different connector on the board).
> 
> See above.
> 
>>>           int bus = i / XLNX_ZYNQMP_NUM_QSPI_BUS_CS;
>>>           gchar *bus_name = g_strdup_printf("qspi%d", bus);
> 

Cédric Le Goater <clg@kaod.org> writes:

> On 11/15/21 16:57, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> 
>>> On 11/15/21 13:55, Markus Armbruster wrote:
>>>> drive_get_next() is basically a bad idea.  It returns the "next" block
>>>> backend of a certain interface type.  "Next" means bus=0,unit=N, where
>>>> subsequent calls count N up from zero, per interface type.
>>>>
>>>> This lets you define unit numbers implicitly by execution order.  If the
>>>> order changes, or new calls appear "in the middle", unit numbers change.
>>>> ABI break.  Hard to spot in review.
>>>>
>>>> Explicit is better than implicit: use drive_get() directly.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>   include/sysemu/blockdev.h           |  1 -
>>>>   blockdev.c                          | 10 ----------
>>>>   hw/arm/aspeed.c                     | 21 +++++++++++++--------
>>>>   hw/arm/cubieboard.c                 |  2 +-
>>>>   hw/arm/imx25_pdk.c                  |  2 +-
>>>>   hw/arm/integratorcp.c               |  2 +-
>>>>   hw/arm/mcimx6ul-evk.c               |  2 +-
>>>>   hw/arm/mcimx7d-sabre.c              |  2 +-
>>>>   hw/arm/msf2-som.c                   |  2 +-
>>>>   hw/arm/npcm7xx_boards.c             |  6 +++---
>>>>   hw/arm/orangepi.c                   |  2 +-
>>>>   hw/arm/raspi.c                      |  2 +-
>>>>   hw/arm/realview.c                   |  2 +-
>>>>   hw/arm/sabrelite.c                  |  2 +-
>>>>   hw/arm/versatilepb.c                |  4 ++--
>>>>   hw/arm/vexpress.c                   |  6 +++---
>>>>   hw/arm/xilinx_zynq.c                | 16 +++++++++-------
>>>>   hw/arm/xlnx-versal-virt.c           |  3 ++-
>>>>   hw/arm/xlnx-zcu102.c                |  6 +++---
>>>>   hw/microblaze/petalogix_ml605_mmu.c |  2 +-
>>>>   hw/misc/sifive_u_otp.c              |  2 +-
>>>>   hw/riscv/microchip_pfsoc.c          |  2 +-
>>>>   hw/sparc64/niagara.c                |  2 +-
>>>>   23 files changed, 49 insertions(+), 52 deletions(-)
>>>
>>>> @@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
>>>>       }
>>>>         for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
>>>> -        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
>>>> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
>>>> +                           drive_get(IF_SD, 0, i));
>>>
>>> If we put SD on bus #0, ...
>>>
>>>>       }
>>>>         if (bmc->soc.emmc.num_slots) {
>>>> -        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
>>>> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
>>>> +                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
>>>
>>> ... we'd want to put eMMC on bus #1
>>
>> Using separate buses for different kinds of devices would be neater, but
>> it also would be an incompatible change.  This patch keeps existing
>> bus/unit numbers working.  drive_get_next() can only use bus 0.
>
> All Aspeed SoCs have 3 SPI busses, each with multiple CS, and also multiple
> sdhci controllers with multiple slots.
>
> How drives are defined for the aspeed machines can/should be improved.
> The machine init iterates on the command line drives, attaches the
> DriveInfo, in the order found, to a m25p80 device model first and then
> follows on with the SD devices. This is fragile clearly and a bus+id
> would be most welcome to identify the drive backend.
>
> May be this is a prereq for this patchset ?

Such a change will probably be easier to review after this patch,
because then it's just a matter of changing / dumbing down parameters to
drive_get().

I can't judge whether incompatible change is okay here.

On 11/16/21 10:29, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> On 11/15/21 16:57, Markus Armbruster wrote:
>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>>
>>>> On 11/15/21 13:55, Markus Armbruster wrote:
>>>>> drive_get_next() is basically a bad idea.  It returns the "next" block
>>>>> backend of a certain interface type.  "Next" means bus=0,unit=N, where
>>>>> subsequent calls count N up from zero, per interface type.
>>>>>
>>>>> This lets you define unit numbers implicitly by execution order.  If the
>>>>> order changes, or new calls appear "in the middle", unit numbers change.
>>>>> ABI break.  Hard to spot in review.
>>>>>
>>>>> Explicit is better than implicit: use drive_get() directly.
>>>>>
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>> ---
>>>>>    include/sysemu/blockdev.h           |  1 -
>>>>>    blockdev.c                          | 10 ----------
>>>>>    hw/arm/aspeed.c                     | 21 +++++++++++++--------
>>>>>    hw/arm/cubieboard.c                 |  2 +-
>>>>>    hw/arm/imx25_pdk.c                  |  2 +-
>>>>>    hw/arm/integratorcp.c               |  2 +-
>>>>>    hw/arm/mcimx6ul-evk.c               |  2 +-
>>>>>    hw/arm/mcimx7d-sabre.c              |  2 +-
>>>>>    hw/arm/msf2-som.c                   |  2 +-
>>>>>    hw/arm/npcm7xx_boards.c             |  6 +++---
>>>>>    hw/arm/orangepi.c                   |  2 +-
>>>>>    hw/arm/raspi.c                      |  2 +-
>>>>>    hw/arm/realview.c                   |  2 +-
>>>>>    hw/arm/sabrelite.c                  |  2 +-
>>>>>    hw/arm/versatilepb.c                |  4 ++--
>>>>>    hw/arm/vexpress.c                   |  6 +++---
>>>>>    hw/arm/xilinx_zynq.c                | 16 +++++++++-------
>>>>>    hw/arm/xlnx-versal-virt.c           |  3 ++-
>>>>>    hw/arm/xlnx-zcu102.c                |  6 +++---
>>>>>    hw/microblaze/petalogix_ml605_mmu.c |  2 +-
>>>>>    hw/misc/sifive_u_otp.c              |  2 +-
>>>>>    hw/riscv/microchip_pfsoc.c          |  2 +-
>>>>>    hw/sparc64/niagara.c                |  2 +-
>>>>>    23 files changed, 49 insertions(+), 52 deletions(-)
>>>>
>>>>> @@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
>>>>>        }
>>>>>          for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
>>>>> -        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
>>>>> +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
>>>>> +                           drive_get(IF_SD, 0, i));
>>>>
>>>> If we put SD on bus #0, ...
>>>>
>>>>>        }
>>>>>          if (bmc->soc.emmc.num_slots) {
>>>>> -        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
>>>>> +        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
>>>>> +                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
>>>>
>>>> ... we'd want to put eMMC on bus #1
>>>
>>> Using separate buses for different kinds of devices would be neater, but
>>> it also would be an incompatible change.  This patch keeps existing
>>> bus/unit numbers working.  drive_get_next() can only use bus 0.
>>
>> All Aspeed SoCs have 3 SPI busses, each with multiple CS, and also multiple
>> sdhci controllers with multiple slots.
>>
>> How drives are defined for the aspeed machines can/should be improved.
>> The machine init iterates on the command line drives, attaches the
>> DriveInfo, in the order found, to a m25p80 device model first and then
>> follows on with the SD devices. This is fragile clearly and a bus+id
>> would be most welcome to identify the drive backend.
>>
>> May be this is a prereq for this patchset ?
> 
> Such a change will probably be easier to review after this patch,
> because then it's just a matter of changing / dumbing down parameters to
> drive_get().

ok.

> I can't judge whether incompatible change is okay here.

It looks ok to me since you are using the number of possible devices
of the previous controller as an offset for drive_get().

Thanks,

C.