* [PATCH RFC 0/2] Eliminate drive_get_next()
@ 2021-11-15 12:55 ` Markus Armbruster
0 siblings, 0 replies; 32+ messages in thread
From: Markus Armbruster @ 2021-11-15 12:55 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, bin.meng, mark.cave-ayland, edgar.iglesias,
sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel,
atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg,
kwolf, qemu-riscv, andrew, f4bug, Andrew.Baumann, jcd, kfting,
hreitz, palmer
This is RFC because I'm unsure about the removal of
/* Reason: init() method uses drive_get_next() */
dc->user_creatable = false;
in PATCH 1. Both users appear to wire up some GPIO. If that's
necessary for the thing to work, we should just replace the comment.
Aside: there may be devices that need manual wiring to work, yet don't
have user_creatable unset. Bugs if you ask me. I don't have smart
ideas on how to track them down.
Markus Armbruster (2):
hw/sd/ssi-sd: Do not create SD card within controller's realize
hw: Replace drive_get_next() by drive_get()
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/stellaris.c | 15 ++++++++++++++-
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/riscv/sifive_u.c | 15 +++++++++++++--
hw/sd/ssi-sd.c | 29 -----------------------------
hw/sparc64/niagara.c | 2 +-
26 files changed, 76 insertions(+), 84 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH RFC 0/2] Eliminate drive_get_next() @ 2021-11-15 12:55 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 12:55 UTC (permalink / raw) To: qemu-devel Cc: kwolf, hreitz, clg, peter.maydell, andrew, joel, b.galvani, jcd, andrew.smirnov, sundeep.lkml, hskinnemoen, kfting, nieklinnenbank, Andrew.Baumann, f4bug, edgar.iglesias, alistair, bin.meng, palmer, atar4qemu, mark.cave-ayland, qemu-block, qemu-arm, qemu-riscv This is RFC because I'm unsure about the removal of /* Reason: init() method uses drive_get_next() */ dc->user_creatable = false; in PATCH 1. Both users appear to wire up some GPIO. If that's necessary for the thing to work, we should just replace the comment. Aside: there may be devices that need manual wiring to work, yet don't have user_creatable unset. Bugs if you ask me. I don't have smart ideas on how to track them down. Markus Armbruster (2): hw/sd/ssi-sd: Do not create SD card within controller's realize hw: Replace drive_get_next() by drive_get() 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/stellaris.c | 15 ++++++++++++++- 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/riscv/sifive_u.c | 15 +++++++++++++-- hw/sd/ssi-sd.c | 29 ----------------------------- hw/sparc64/niagara.c | 2 +- 26 files changed, 76 insertions(+), 84 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC 1/2] hw/sd/ssi-sd: Do not create SD card within controller's realize 2021-11-15 12:55 ` Markus Armbruster @ 2021-11-15 12:55 ` Markus Armbruster -1 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 12:55 UTC (permalink / raw) To: qemu-devel Cc: peter.maydell, bin.meng, mark.cave-ayland, edgar.iglesias, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, f4bug, Andrew.Baumann, jcd, kfting, hreitz, palmer ssi_sd_realize() creates an "sd-card" device. This is inappropriate, and marked FIXME. Move it to the boards that create these devices. Prior art: commit eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for device "pl181". Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/arm/stellaris.c | 15 ++++++++++++++- hw/riscv/sifive_u.c | 15 +++++++++++++-- hw/sd/ssi-sd.c | 29 ----------------------------- 3 files changed, 27 insertions(+), 32 deletions(-) diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c index 78827ace6b..b6c8a5d609 100644 --- a/hw/arm/stellaris.c +++ b/hw/arm/stellaris.c @@ -10,6 +10,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "hw/sysbus.h" +#include "hw/sd/sd.h" #include "hw/ssi/ssi.h" #include "hw/arm/boot.h" #include "qemu/timer.h" @@ -1157,6 +1158,9 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) void *bus; DeviceState *sddev; DeviceState *ssddev; + DriveInfo *dinfo; + DeviceState *carddev; + BlockBackend *blk; /* * Some boards have both an OLED controller and SD card connected to @@ -1221,8 +1225,17 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) * - Make the ssd0323 OLED controller chipselect active-low */ bus = qdev_get_child_bus(dev, "ssi"); - sddev = ssi_create_peripheral(bus, "ssi-sd"); + + dinfo = drive_get(IF_SD, 0, 0); + blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; + carddev = qdev_new(TYPE_SD_CARD); + qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal); + qdev_prop_set_bit(carddev, "spi", true); + qdev_realize_and_unref(carddev, + qdev_get_child_bus(sddev, "sd-bus"), + &error_fatal); + ssddev = ssi_create_peripheral(bus, "ssd0323"); gpio_out[GPIO_D][0] = qemu_irq_split( qdev_get_gpio_in_named(sddev, SSI_GPIO_CS, 0), diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 589ae72a59..aa74e67889 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -46,6 +46,7 @@ #include "hw/char/serial.h" #include "hw/cpu/cluster.h" #include "hw/misc/unimp.h" +#include "hw/sd/sd.h" #include "hw/ssi/ssi.h" #include "target/riscv/cpu.h" #include "hw/riscv/riscv_hart.h" @@ -536,7 +537,8 @@ static void sifive_u_machine_init(MachineState *machine) uint32_t fdt_load_addr; uint64_t kernel_entry; DriveInfo *dinfo; - DeviceState *flash_dev, *sd_dev; + BlockBackend *blk; + DeviceState *flash_dev, *sd_dev, *card_dev; qemu_irq flash_cs, sd_cs; /* Initialize SoC */ @@ -670,7 +672,7 @@ static void sifive_u_machine_init(MachineState *machine) /* Connect an SPI flash to SPI0 */ flash_dev = qdev_new("is25wp256"); - dinfo = drive_get_next(IF_MTD); + dinfo = drive_get(IF_MTD, 0, 0); if (dinfo) { qdev_prop_set_drive_err(flash_dev, "drive", blk_by_legacy_dinfo(dinfo), @@ -686,6 +688,15 @@ static void sifive_u_machine_init(MachineState *machine) sd_cs = qdev_get_gpio_in_named(sd_dev, SSI_GPIO_CS, 0); sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi2), 1, sd_cs); + + dinfo = drive_get(IF_SD, 0, 0); + blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; + card_dev = qdev_new(TYPE_SD_CARD); + qdev_prop_set_drive_err(card_dev, "drive", blk, &error_fatal); + qdev_prop_set_bit(card_dev, "spi", true); + qdev_realize_and_unref(card_dev, + qdev_get_child_bus(sd_dev, "sd-bus"), + &error_fatal); } static bool sifive_u_machine_get_start_in_flash(Object *obj, Error **errp) diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index e60854eeef..558506f6a0 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -368,36 +368,9 @@ static const VMStateDescription vmstate_ssi_sd = { static void ssi_sd_realize(SSIPeripheral *d, Error **errp) { - ERRP_GUARD(); ssi_sd_state *s = SSI_SD(d); - DeviceState *carddev; - DriveInfo *dinfo; qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS, DEVICE(d), "sd-bus"); - - /* Create and plug in the sd card */ - /* FIXME use a qdev drive property instead of drive_get_next() */ - dinfo = drive_get_next(IF_SD); - carddev = qdev_new(TYPE_SD_CARD); - if (dinfo) { - if (!qdev_prop_set_drive_err(carddev, "drive", - blk_by_legacy_dinfo(dinfo), errp)) { - goto fail; - } - } - - if (!object_property_set_bool(OBJECT(carddev), "spi", true, errp)) { - goto fail; - } - - if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), errp)) { - goto fail; - } - - return; - -fail: - error_prepend(errp, "failed to init SD card: "); } static void ssi_sd_reset(DeviceState *dev) @@ -426,8 +399,6 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data) k->cs_polarity = SSI_CS_LOW; dc->vmsd = &vmstate_ssi_sd; dc->reset = ssi_sd_reset; - /* Reason: init() method uses drive_get_next() */ - dc->user_creatable = false; } static const TypeInfo ssi_sd_info = { -- 2.31.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH RFC 1/2] hw/sd/ssi-sd: Do not create SD card within controller's realize @ 2021-11-15 12:55 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 12:55 UTC (permalink / raw) To: qemu-devel Cc: kwolf, hreitz, clg, peter.maydell, andrew, joel, b.galvani, jcd, andrew.smirnov, sundeep.lkml, hskinnemoen, kfting, nieklinnenbank, Andrew.Baumann, f4bug, edgar.iglesias, alistair, bin.meng, palmer, atar4qemu, mark.cave-ayland, qemu-block, qemu-arm, qemu-riscv ssi_sd_realize() creates an "sd-card" device. This is inappropriate, and marked FIXME. Move it to the boards that create these devices. Prior art: commit eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for device "pl181". Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/arm/stellaris.c | 15 ++++++++++++++- hw/riscv/sifive_u.c | 15 +++++++++++++-- hw/sd/ssi-sd.c | 29 ----------------------------- 3 files changed, 27 insertions(+), 32 deletions(-) diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c index 78827ace6b..b6c8a5d609 100644 --- a/hw/arm/stellaris.c +++ b/hw/arm/stellaris.c @@ -10,6 +10,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "hw/sysbus.h" +#include "hw/sd/sd.h" #include "hw/ssi/ssi.h" #include "hw/arm/boot.h" #include "qemu/timer.h" @@ -1157,6 +1158,9 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) void *bus; DeviceState *sddev; DeviceState *ssddev; + DriveInfo *dinfo; + DeviceState *carddev; + BlockBackend *blk; /* * Some boards have both an OLED controller and SD card connected to @@ -1221,8 +1225,17 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) * - Make the ssd0323 OLED controller chipselect active-low */ bus = qdev_get_child_bus(dev, "ssi"); - sddev = ssi_create_peripheral(bus, "ssi-sd"); + + dinfo = drive_get(IF_SD, 0, 0); + blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; + carddev = qdev_new(TYPE_SD_CARD); + qdev_prop_set_drive_err(carddev, "drive", blk, &error_fatal); + qdev_prop_set_bit(carddev, "spi", true); + qdev_realize_and_unref(carddev, + qdev_get_child_bus(sddev, "sd-bus"), + &error_fatal); + ssddev = ssi_create_peripheral(bus, "ssd0323"); gpio_out[GPIO_D][0] = qemu_irq_split( qdev_get_gpio_in_named(sddev, SSI_GPIO_CS, 0), diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index 589ae72a59..aa74e67889 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -46,6 +46,7 @@ #include "hw/char/serial.h" #include "hw/cpu/cluster.h" #include "hw/misc/unimp.h" +#include "hw/sd/sd.h" #include "hw/ssi/ssi.h" #include "target/riscv/cpu.h" #include "hw/riscv/riscv_hart.h" @@ -536,7 +537,8 @@ static void sifive_u_machine_init(MachineState *machine) uint32_t fdt_load_addr; uint64_t kernel_entry; DriveInfo *dinfo; - DeviceState *flash_dev, *sd_dev; + BlockBackend *blk; + DeviceState *flash_dev, *sd_dev, *card_dev; qemu_irq flash_cs, sd_cs; /* Initialize SoC */ @@ -670,7 +672,7 @@ static void sifive_u_machine_init(MachineState *machine) /* Connect an SPI flash to SPI0 */ flash_dev = qdev_new("is25wp256"); - dinfo = drive_get_next(IF_MTD); + dinfo = drive_get(IF_MTD, 0, 0); if (dinfo) { qdev_prop_set_drive_err(flash_dev, "drive", blk_by_legacy_dinfo(dinfo), @@ -686,6 +688,15 @@ static void sifive_u_machine_init(MachineState *machine) sd_cs = qdev_get_gpio_in_named(sd_dev, SSI_GPIO_CS, 0); sysbus_connect_irq(SYS_BUS_DEVICE(&s->soc.spi2), 1, sd_cs); + + dinfo = drive_get(IF_SD, 0, 0); + blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; + card_dev = qdev_new(TYPE_SD_CARD); + qdev_prop_set_drive_err(card_dev, "drive", blk, &error_fatal); + qdev_prop_set_bit(card_dev, "spi", true); + qdev_realize_and_unref(card_dev, + qdev_get_child_bus(sd_dev, "sd-bus"), + &error_fatal); } static bool sifive_u_machine_get_start_in_flash(Object *obj, Error **errp) diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index e60854eeef..558506f6a0 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -368,36 +368,9 @@ static const VMStateDescription vmstate_ssi_sd = { static void ssi_sd_realize(SSIPeripheral *d, Error **errp) { - ERRP_GUARD(); ssi_sd_state *s = SSI_SD(d); - DeviceState *carddev; - DriveInfo *dinfo; qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SD_BUS, DEVICE(d), "sd-bus"); - - /* Create and plug in the sd card */ - /* FIXME use a qdev drive property instead of drive_get_next() */ - dinfo = drive_get_next(IF_SD); - carddev = qdev_new(TYPE_SD_CARD); - if (dinfo) { - if (!qdev_prop_set_drive_err(carddev, "drive", - blk_by_legacy_dinfo(dinfo), errp)) { - goto fail; - } - } - - if (!object_property_set_bool(OBJECT(carddev), "spi", true, errp)) { - goto fail; - } - - if (!qdev_realize_and_unref(carddev, BUS(&s->sdbus), errp)) { - goto fail; - } - - return; - -fail: - error_prepend(errp, "failed to init SD card: "); } static void ssi_sd_reset(DeviceState *dev) @@ -426,8 +399,6 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data) k->cs_polarity = SSI_CS_LOW; dc->vmsd = &vmstate_ssi_sd; dc->reset = ssi_sd_reset; - /* Reason: init() method uses drive_get_next() */ - dc->user_creatable = false; } static const TypeInfo ssi_sd_info = { -- 2.31.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 1/2] hw/sd/ssi-sd: Do not create SD card within controller's realize 2021-11-15 12:55 ` Markus Armbruster @ 2021-11-15 13:40 ` Peter Maydell -1 siblings, 0 replies; 32+ messages in thread From: Peter Maydell @ 2021-11-15 13:40 UTC (permalink / raw) To: Markus Armbruster Cc: bin.meng, mark.cave-ayland, qemu-devel, edgar.iglesias, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, f4bug, Andrew.Baumann, jcd, kfting, hreitz, palmer On Mon, 15 Nov 2021 at 12:56, Markus Armbruster <armbru@redhat.com> wrote: > > ssi_sd_realize() creates an "sd-card" device. This is inappropriate, > and marked FIXME. > > Move it to the boards that create these devices. Prior art: commit > eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for > device "pl181". > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > @@ -670,7 +672,7 @@ static void sifive_u_machine_init(MachineState *machine) > > /* Connect an SPI flash to SPI0 */ > flash_dev = qdev_new("is25wp256"); > - dinfo = drive_get_next(IF_MTD); > + dinfo = drive_get(IF_MTD, 0, 0); > if (dinfo) { > qdev_prop_set_drive_err(flash_dev, "drive", > blk_by_legacy_dinfo(dinfo), This part looks like it should have been in the other patch. -- PMM ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 1/2] hw/sd/ssi-sd: Do not create SD card within controller's realize @ 2021-11-15 13:40 ` Peter Maydell 0 siblings, 0 replies; 32+ messages in thread From: Peter Maydell @ 2021-11-15 13:40 UTC (permalink / raw) To: Markus Armbruster Cc: qemu-devel, kwolf, hreitz, clg, andrew, joel, b.galvani, jcd, andrew.smirnov, sundeep.lkml, hskinnemoen, kfting, nieklinnenbank, Andrew.Baumann, f4bug, edgar.iglesias, alistair, bin.meng, palmer, atar4qemu, mark.cave-ayland, qemu-block, qemu-arm, qemu-riscv On Mon, 15 Nov 2021 at 12:56, Markus Armbruster <armbru@redhat.com> wrote: > > ssi_sd_realize() creates an "sd-card" device. This is inappropriate, > and marked FIXME. > > Move it to the boards that create these devices. Prior art: commit > eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for > device "pl181". > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > @@ -670,7 +672,7 @@ static void sifive_u_machine_init(MachineState *machine) > > /* Connect an SPI flash to SPI0 */ > flash_dev = qdev_new("is25wp256"); > - dinfo = drive_get_next(IF_MTD); > + dinfo = drive_get(IF_MTD, 0, 0); > if (dinfo) { > qdev_prop_set_drive_err(flash_dev, "drive", > blk_by_legacy_dinfo(dinfo), This part looks like it should have been in the other patch. -- PMM ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 1/2] hw/sd/ssi-sd: Do not create SD card within controller's realize 2021-11-15 13:40 ` Peter Maydell @ 2021-11-15 13:48 ` Markus Armbruster -1 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 13:48 UTC (permalink / raw) To: Peter Maydell Cc: bin.meng, mark.cave-ayland, qemu-devel, edgar.iglesias, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, f4bug, Andrew.Baumann, jcd, kfting, hreitz, palmer Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 15 Nov 2021 at 12:56, Markus Armbruster <armbru@redhat.com> wrote: >> >> ssi_sd_realize() creates an "sd-card" device. This is inappropriate, >> and marked FIXME. >> >> Move it to the boards that create these devices. Prior art: commit >> eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for >> device "pl181". >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- > >> @@ -670,7 +672,7 @@ static void sifive_u_machine_init(MachineState *machine) >> >> /* Connect an SPI flash to SPI0 */ >> flash_dev = qdev_new("is25wp256"); >> - dinfo = drive_get_next(IF_MTD); >> + dinfo = drive_get(IF_MTD, 0, 0); >> if (dinfo) { >> qdev_prop_set_drive_err(flash_dev, "drive", >> blk_by_legacy_dinfo(dinfo), > > > This part looks like it should have been in the other patch. You're right. Thanks! ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 1/2] hw/sd/ssi-sd: Do not create SD card within controller's realize @ 2021-11-15 13:48 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 13:48 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, kwolf, hreitz, clg, andrew, joel, b.galvani, jcd, andrew.smirnov, sundeep.lkml, hskinnemoen, kfting, nieklinnenbank, Andrew.Baumann, f4bug, edgar.iglesias, alistair, bin.meng, palmer, atar4qemu, mark.cave-ayland, qemu-block, qemu-arm, qemu-riscv Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 15 Nov 2021 at 12:56, Markus Armbruster <armbru@redhat.com> wrote: >> >> ssi_sd_realize() creates an "sd-card" device. This is inappropriate, >> and marked FIXME. >> >> Move it to the boards that create these devices. Prior art: commit >> eb4f566bbb for device "generic-sdhci", and commit 26c607b86b for >> device "pl181". >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- > >> @@ -670,7 +672,7 @@ static void sifive_u_machine_init(MachineState *machine) >> >> /* Connect an SPI flash to SPI0 */ >> flash_dev = qdev_new("is25wp256"); >> - dinfo = drive_get_next(IF_MTD); >> + dinfo = drive_get(IF_MTD, 0, 0); >> if (dinfo) { >> qdev_prop_set_drive_err(flash_dev, "drive", >> blk_by_legacy_dinfo(dinfo), > > > This part looks like it should have been in the other patch. You're right. Thanks! ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-15 12:55 ` Markus Armbruster @ 2021-11-15 12:55 ` Markus Armbruster -1 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 12:55 UTC (permalink / raw) To: qemu-devel Cc: peter.maydell, bin.meng, mark.cave-ayland, edgar.iglesias, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, f4bug, Andrew.Baumann, jcd, kfting, hreitz, palmer 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-15 12:55 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 12:55 UTC (permalink / raw) To: qemu-devel Cc: kwolf, hreitz, clg, peter.maydell, andrew, joel, b.galvani, jcd, andrew.smirnov, sundeep.lkml, hskinnemoen, kfting, nieklinnenbank, Andrew.Baumann, f4bug, edgar.iglesias, alistair, bin.meng, palmer, atar4qemu, mark.cave-ayland, qemu-block, qemu-arm, qemu-riscv 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-15 12:55 ` Markus Armbruster @ 2021-11-15 13:38 ` Peter Maydell -1 siblings, 0 replies; 32+ messages in thread From: Peter Maydell @ 2021-11-15 13:38 UTC (permalink / raw) To: Markus Armbruster Cc: bin.meng, mark.cave-ayland, qemu-devel, edgar.iglesias, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, f4bug, Andrew.Baumann, jcd, kfting, hreitz, palmer 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-15 13:38 ` Peter Maydell 0 siblings, 0 replies; 32+ messages in thread From: Peter Maydell @ 2021-11-15 13:38 UTC (permalink / raw) To: Markus Armbruster Cc: qemu-devel, kwolf, hreitz, clg, andrew, joel, b.galvani, jcd, andrew.smirnov, sundeep.lkml, hskinnemoen, kfting, nieklinnenbank, Andrew.Baumann, f4bug, edgar.iglesias, alistair, bin.meng, palmer, atar4qemu, mark.cave-ayland, qemu-block, qemu-arm, qemu-riscv 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-15 13:38 ` Peter Maydell @ 2021-11-15 13:48 ` Markus Armbruster -1 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 13:48 UTC (permalink / raw) To: Peter Maydell Cc: bin.meng, mark.cave-ayland, qemu-devel, edgar.iglesias, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, f4bug, Andrew.Baumann, jcd, kfting, hreitz, palmer 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-15 13:48 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 13:48 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, kwolf, hreitz, clg, andrew, joel, b.galvani, jcd, andrew.smirnov, sundeep.lkml, hskinnemoen, kfting, nieklinnenbank, Andrew.Baumann, f4bug, edgar.iglesias, alistair, bin.meng, palmer, atar4qemu, mark.cave-ayland, qemu-block, qemu-arm, qemu-riscv 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-15 12:55 ` Markus Armbruster @ 2021-11-15 13:59 ` Philippe Mathieu-Daudé -1 siblings, 0 replies; 32+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-15 13:59 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: peter.maydell, bin.meng, mark.cave-ayland, jcd, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, Andrew.Baumann, sundeep.lkml, kfting, hreitz, palmer 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); ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-15 13:59 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 32+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-15 13:59 UTC (permalink / raw) To: Markus Armbruster, qemu-devel Cc: peter.maydell, bin.meng, mark.cave-ayland, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, Andrew.Baumann, jcd, kfting, hreitz, palmer 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); ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-15 13:59 ` Philippe Mathieu-Daudé @ 2021-11-15 15:57 ` Markus Armbruster -1 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 15:57 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: peter.maydell, bin.meng, mark.cave-ayland, qemu-devel, jcd, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, Andrew.Baumann, sundeep.lkml, kfting, hreitz, palmer 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); ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-15 15:57 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 15:57 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: qemu-devel, peter.maydell, bin.meng, mark.cave-ayland, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, Andrew.Baumann, jcd, kfting, hreitz, palmer 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); ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-15 15:57 ` Markus Armbruster @ 2021-11-15 21:15 ` Philippe Mathieu-Daudé -1 siblings, 0 replies; 32+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-15 21:15 UTC (permalink / raw) To: Markus Armbruster Cc: peter.maydell, bin.meng, mark.cave-ayland, qemu-devel, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, Andrew.Baumann, jcd, kfting, hreitz, palmer 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-15 21:15 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 32+ messages in thread From: Philippe Mathieu-Daudé @ 2021-11-15 21:15 UTC (permalink / raw) To: Markus Armbruster Cc: peter.maydell, bin.meng, mark.cave-ayland, qemu-devel, jcd, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, Andrew.Baumann, sundeep.lkml, kfting, hreitz, palmer 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-15 21:15 ` Philippe Mathieu-Daudé @ 2021-11-16 7:47 ` Markus Armbruster -1 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-16 7:47 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: peter.maydell, bin.meng, mark.cave-ayland, qemu-devel, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, Andrew.Baumann, jcd, kfting, hreitz, palmer 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? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-16 7:47 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-16 7:47 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: peter.maydell, bin.meng, mark.cave-ayland, qemu-devel, jcd, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, Andrew.Baumann, sundeep.lkml, kfting, hreitz, palmer 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? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-15 15:57 ` Markus Armbruster @ 2021-11-16 8:52 ` Cédric Le Goater -1 siblings, 0 replies; 32+ messages in thread From: Cédric Le Goater @ 2021-11-16 8:52 UTC (permalink / raw) To: Markus Armbruster, Philippe Mathieu-Daudé Cc: peter.maydell, bin.meng, mark.cave-ayland, qemu-devel, jcd, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, kwolf, qemu-riscv, andrew, Andrew.Baumann, sundeep.lkml, kfting, hreitz, palmer 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); > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-16 8:52 ` Cédric Le Goater 0 siblings, 0 replies; 32+ messages in thread From: Cédric Le Goater @ 2021-11-16 8:52 UTC (permalink / raw) To: Markus Armbruster, Philippe Mathieu-Daudé Cc: qemu-devel, peter.maydell, bin.meng, mark.cave-ayland, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, kwolf, qemu-riscv, andrew, Andrew.Baumann, jcd, kfting, hreitz, palmer 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); > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-16 8:52 ` Cédric Le Goater @ 2021-11-16 9:29 ` Markus Armbruster -1 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-16 9:29 UTC (permalink / raw) To: Cédric Le Goater Cc: peter.maydell, bin.meng, mark.cave-ayland, qemu-devel, jcd, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, kwolf, qemu-riscv, andrew, Philippe Mathieu-Daudé, Andrew.Baumann, sundeep.lkml, kfting, hreitz, palmer 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-16 9:29 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-16 9:29 UTC (permalink / raw) To: Cédric Le Goater Cc: Philippe Mathieu-Daudé, qemu-devel, peter.maydell, bin.meng, mark.cave-ayland, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, kwolf, qemu-riscv, andrew, Andrew.Baumann, jcd, kfting, hreitz, palmer 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() 2021-11-16 9:29 ` Markus Armbruster @ 2021-11-16 12:14 ` Cédric Le Goater -1 siblings, 0 replies; 32+ messages in thread From: Cédric Le Goater @ 2021-11-16 12:14 UTC (permalink / raw) To: Markus Armbruster Cc: peter.maydell, bin.meng, mark.cave-ayland, qemu-devel, jcd, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, kwolf, qemu-riscv, andrew, Philippe Mathieu-Daudé, Andrew.Baumann, sundeep.lkml, kfting, hreitz, palmer 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() @ 2021-11-16 12:14 ` Cédric Le Goater 0 siblings, 0 replies; 32+ messages in thread From: Cédric Le Goater @ 2021-11-16 12:14 UTC (permalink / raw) To: Markus Armbruster Cc: Philippe Mathieu-Daudé, qemu-devel, peter.maydell, bin.meng, mark.cave-ayland, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, kwolf, qemu-riscv, andrew, Andrew.Baumann, jcd, kfting, hreitz, palmer 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 0/2] Eliminate drive_get_next() 2021-11-15 12:55 ` Markus Armbruster @ 2021-11-15 14:05 ` Peter Maydell -1 siblings, 0 replies; 32+ messages in thread From: Peter Maydell @ 2021-11-15 14:05 UTC (permalink / raw) To: Markus Armbruster Cc: bin.meng, mark.cave-ayland, qemu-devel, edgar.iglesias, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, f4bug, Andrew.Baumann, jcd, kfting, hreitz, palmer On Mon, 15 Nov 2021 at 12:55, Markus Armbruster <armbru@redhat.com> wrote: > > This is RFC because I'm unsure about the removal of > > /* Reason: init() method uses drive_get_next() */ > dc->user_creatable = false; > > in PATCH 1. Both users appear to wire up some GPIO. If that's > necessary for the thing to work, we should just replace the comment. Looking at the code, it sort of is and sort of isn't. The GPIO line is the chip-select line. If you don't connect it then (because the ssi-sd device configures cs_polarity to SSI_CS_LOW, requesting an active-low chip-select) the device will always be selected. If the machine created an SSI bus with no SSI device attached to it then in theory the user could create an ssi-sd device and connect it there and have it work. But in practice it's really unlikely: machines create SSI buses with specific wired-in devices on them, and the guest OS knows what it has to do to enable the chip select for the device it wants to talk to (often some known GPIO pin on a GPIO controller). So I would keep the user_creatable = false, with a reason of "user should wire up GPIO chip-select line". But see below for a pile of contrary precedent. (The chip-select GPIO is created in the parent class, incidentally.) > Aside: there may be devices that need manual wiring to work, yet don't > have user_creatable unset. Bugs if you ask me. I don't have smart > ideas on how to track them down. Me neither. I notice that the TYPE_M25P80 is also an SSI peripheral with an active-low chipselect but that one doesn't set user_creatable to false. TYPE_SSD0323 also is user-creatable and that one has an active-high chipselect, which means the user can create a device but it will then never be usable because it will ignore all transactions. (More generally, looks like most subclasses of TYPE_SSI_PERIPHERAL don't set user_creatable = false.) -- PMM ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 0/2] Eliminate drive_get_next() @ 2021-11-15 14:05 ` Peter Maydell 0 siblings, 0 replies; 32+ messages in thread From: Peter Maydell @ 2021-11-15 14:05 UTC (permalink / raw) To: Markus Armbruster Cc: qemu-devel, kwolf, hreitz, clg, andrew, joel, b.galvani, jcd, andrew.smirnov, sundeep.lkml, hskinnemoen, kfting, nieklinnenbank, Andrew.Baumann, f4bug, edgar.iglesias, alistair, bin.meng, palmer, atar4qemu, mark.cave-ayland, qemu-block, qemu-arm, qemu-riscv On Mon, 15 Nov 2021 at 12:55, Markus Armbruster <armbru@redhat.com> wrote: > > This is RFC because I'm unsure about the removal of > > /* Reason: init() method uses drive_get_next() */ > dc->user_creatable = false; > > in PATCH 1. Both users appear to wire up some GPIO. If that's > necessary for the thing to work, we should just replace the comment. Looking at the code, it sort of is and sort of isn't. The GPIO line is the chip-select line. If you don't connect it then (because the ssi-sd device configures cs_polarity to SSI_CS_LOW, requesting an active-low chip-select) the device will always be selected. If the machine created an SSI bus with no SSI device attached to it then in theory the user could create an ssi-sd device and connect it there and have it work. But in practice it's really unlikely: machines create SSI buses with specific wired-in devices on them, and the guest OS knows what it has to do to enable the chip select for the device it wants to talk to (often some known GPIO pin on a GPIO controller). So I would keep the user_creatable = false, with a reason of "user should wire up GPIO chip-select line". But see below for a pile of contrary precedent. (The chip-select GPIO is created in the parent class, incidentally.) > Aside: there may be devices that need manual wiring to work, yet don't > have user_creatable unset. Bugs if you ask me. I don't have smart > ideas on how to track them down. Me neither. I notice that the TYPE_M25P80 is also an SSI peripheral with an active-low chipselect but that one doesn't set user_creatable to false. TYPE_SSD0323 also is user-creatable and that one has an active-high chipselect, which means the user can create a device but it will then never be usable because it will ignore all transactions. (More generally, looks like most subclasses of TYPE_SSI_PERIPHERAL don't set user_creatable = false.) -- PMM ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 0/2] Eliminate drive_get_next() 2021-11-15 14:05 ` Peter Maydell @ 2021-11-15 16:01 ` Markus Armbruster -1 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 16:01 UTC (permalink / raw) To: Peter Maydell Cc: bin.meng, mark.cave-ayland, qemu-devel, edgar.iglesias, sundeep.lkml, qemu-block, andrew.smirnov, hskinnemoen, joel, atar4qemu, alistair, b.galvani, nieklinnenbank, qemu-arm, clg, kwolf, qemu-riscv, andrew, f4bug, Andrew.Baumann, jcd, kfting, hreitz, palmer Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 15 Nov 2021 at 12:55, Markus Armbruster <armbru@redhat.com> wrote: >> >> This is RFC because I'm unsure about the removal of >> >> /* Reason: init() method uses drive_get_next() */ >> dc->user_creatable = false; >> >> in PATCH 1. Both users appear to wire up some GPIO. If that's >> necessary for the thing to work, we should just replace the comment. > > Looking at the code, it sort of is and sort of isn't. The GPIO line > is the chip-select line. If you don't connect it then (because the > ssi-sd device configures cs_polarity to SSI_CS_LOW, requesting an > active-low chip-select) the device will always be selected. If > the machine created an SSI bus with no SSI device attached to it > then in theory the user could create an ssi-sd device and connect > it there and have it work. But in practice it's really unlikely: > machines create SSI buses with specific wired-in devices on them, > and the guest OS knows what it has to do to enable the chip select > for the device it wants to talk to (often some known GPIO pin on > a GPIO controller). > > So I would keep the user_creatable = false, with a reason of > "user should wire up GPIO chip-select line". But see below for I'll do it this way. > a pile of contrary precedent. > > (The chip-select GPIO is created in the parent class, incidentally.) > >> Aside: there may be devices that need manual wiring to work, yet don't >> have user_creatable unset. Bugs if you ask me. I don't have smart >> ideas on how to track them down. > > Me neither. I notice that the TYPE_M25P80 is also an SSI peripheral > with an active-low chipselect but that one doesn't set user_creatable > to false. TYPE_SSD0323 also is user-creatable and that one has an > active-high chipselect, which means the user can create a device but > it will then never be usable because it will ignore all transactions. > (More generally, looks like most subclasses of TYPE_SSI_PERIPHERAL > don't set user_creatable = false.) For sysbus devices, we clear user_creatable by default, because sysbus devices pretty much always[*] need wiring. Is this the case for SSI bus devices, too? [*] The most prominent exception is "dynamic sysbus", which I believe was a mistake. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH RFC 0/2] Eliminate drive_get_next() @ 2021-11-15 16:01 ` Markus Armbruster 0 siblings, 0 replies; 32+ messages in thread From: Markus Armbruster @ 2021-11-15 16:01 UTC (permalink / raw) To: Peter Maydell Cc: qemu-devel, kwolf, hreitz, clg, andrew, joel, b.galvani, jcd, andrew.smirnov, sundeep.lkml, hskinnemoen, kfting, nieklinnenbank, Andrew.Baumann, f4bug, edgar.iglesias, alistair, bin.meng, palmer, atar4qemu, mark.cave-ayland, qemu-block, qemu-arm, qemu-riscv Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 15 Nov 2021 at 12:55, Markus Armbruster <armbru@redhat.com> wrote: >> >> This is RFC because I'm unsure about the removal of >> >> /* Reason: init() method uses drive_get_next() */ >> dc->user_creatable = false; >> >> in PATCH 1. Both users appear to wire up some GPIO. If that's >> necessary for the thing to work, we should just replace the comment. > > Looking at the code, it sort of is and sort of isn't. The GPIO line > is the chip-select line. If you don't connect it then (because the > ssi-sd device configures cs_polarity to SSI_CS_LOW, requesting an > active-low chip-select) the device will always be selected. If > the machine created an SSI bus with no SSI device attached to it > then in theory the user could create an ssi-sd device and connect > it there and have it work. But in practice it's really unlikely: > machines create SSI buses with specific wired-in devices on them, > and the guest OS knows what it has to do to enable the chip select > for the device it wants to talk to (often some known GPIO pin on > a GPIO controller). > > So I would keep the user_creatable = false, with a reason of > "user should wire up GPIO chip-select line". But see below for I'll do it this way. > a pile of contrary precedent. > > (The chip-select GPIO is created in the parent class, incidentally.) > >> Aside: there may be devices that need manual wiring to work, yet don't >> have user_creatable unset. Bugs if you ask me. I don't have smart >> ideas on how to track them down. > > Me neither. I notice that the TYPE_M25P80 is also an SSI peripheral > with an active-low chipselect but that one doesn't set user_creatable > to false. TYPE_SSD0323 also is user-creatable and that one has an > active-high chipselect, which means the user can create a device but > it will then never be usable because it will ignore all transactions. > (More generally, looks like most subclasses of TYPE_SSI_PERIPHERAL > don't set user_creatable = false.) For sysbus devices, we clear user_creatable by default, because sysbus devices pretty much always[*] need wiring. Is this the case for SSI bus devices, too? [*] The most prominent exception is "dynamic sysbus", which I believe was a mistake. ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2021-11-16 12:15 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-15 12:55 [PATCH RFC 0/2] Eliminate drive_get_next() Markus Armbruster 2021-11-15 12:55 ` Markus Armbruster 2021-11-15 12:55 ` [PATCH RFC 1/2] hw/sd/ssi-sd: Do not create SD card within controller's realize Markus Armbruster 2021-11-15 12:55 ` Markus Armbruster 2021-11-15 13:40 ` Peter Maydell 2021-11-15 13:40 ` Peter Maydell 2021-11-15 13:48 ` Markus Armbruster 2021-11-15 13:48 ` Markus Armbruster 2021-11-15 12:55 ` [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get() Markus Armbruster 2021-11-15 12:55 ` Markus Armbruster 2021-11-15 13:38 ` Peter Maydell 2021-11-15 13:38 ` Peter Maydell 2021-11-15 13:48 ` Markus Armbruster 2021-11-15 13:48 ` Markus Armbruster 2021-11-15 13:59 ` Philippe Mathieu-Daudé 2021-11-15 13:59 ` Philippe Mathieu-Daudé 2021-11-15 15:57 ` Markus Armbruster 2021-11-15 15:57 ` Markus Armbruster 2021-11-15 21:15 ` Philippe Mathieu-Daudé 2021-11-15 21:15 ` Philippe Mathieu-Daudé 2021-11-16 7:47 ` Markus Armbruster 2021-11-16 7:47 ` Markus Armbruster 2021-11-16 8:52 ` Cédric Le Goater 2021-11-16 8:52 ` Cédric Le Goater 2021-11-16 9:29 ` Markus Armbruster 2021-11-16 9:29 ` Markus Armbruster 2021-11-16 12:14 ` Cédric Le Goater 2021-11-16 12:14 ` Cédric Le Goater 2021-11-15 14:05 ` [PATCH RFC 0/2] Eliminate drive_get_next() Peter Maydell 2021-11-15 14:05 ` Peter Maydell 2021-11-15 16:01 ` Markus Armbruster 2021-11-15 16:01 ` Markus Armbruster
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.