| Message ID | 1427292969-30929-2-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 25.03.2015 um 15:16 schrieb Markus Armbruster: > Drives defined with if!=none are for board initialization to wire up. > Board code calls drive_get() or similar to find them, and creates > devices with their qdev drive properties set accordingly. > > Except a few devices go on a fishing expedition for a suitable backend > instead of exposing a drive property for board code to set: they call > driver_get() or drive_get_next() in their realize() or init() method > to implicitly connect to the "next" backend with a certain interface > type. > > Picking up backends that way works when the devices are created by > board code. But it's inappropriate for -device or device_add. Not > only is this inconsistent with how the other block device models work > (they connect to a backend explicitly identified by a "drive" > property), it breaks when the "next" backend has been picked up by the > board already. > > Example: > > $ qemu-system-arm -S -M connex -pflash flash.img -device ssi-sd > Aborted (core dumped) > > Mark them with suitable FIXME comments. > > Cc: Andrzej Zaborowski <balrogg@gmail.com> > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Cc: "Andreas Färber" <andreas.faerber@web.de> > Cc: Michael Walle <michael@walle.cc> > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > hw/arm/spitz.c | 1 + > hw/block/m25p80.c | 1 + > hw/isa/pc87312.c | 2 ++ > hw/sd/milkymist-memcard.c | 1 + > hw/sd/pl181.c | 1 + > hw/sd/sdhci.c | 1 + > hw/sd/ssi-sd.c | 1 + > 7 files changed, 8 insertions(+) [...] > diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c > index 40a1106..2849e8d 100644 > --- a/hw/isa/pc87312.c > +++ b/hw/isa/pc87312.c > @@ -319,11 +319,13 @@ static void pc87312_realize(DeviceState *dev, Error **errp) > d = DEVICE(isa); > qdev_prop_set_uint32(d, "iobase", get_fdc_iobase(s)); > qdev_prop_set_uint32(d, "irq", 6); > + /* FIXME use a qdev drive property instead of drive_get() */ > drive = drive_get(IF_FLOPPY, 0, 0); > if (drive != NULL) { > qdev_prop_set_drive_nofail(d, "driveA", > blk_by_legacy_dinfo(drive)); > } > + /* FIXME use a qdev drive property instead of drive_get() */ > drive = drive_get(IF_FLOPPY, 0, 1); > if (drive != NULL) { > qdev_prop_set_drive_nofail(d, "driveB", As can be seen, there are drive properties driveA and driveB already on the destination device. How do you imagine this to be fixed? Add alias properties on the container device? (CC'ing Stefan) Regards, Andreas
On 25/03/2015 16:17, Andreas Färber wrote: > > @@ -319,11 +319,13 @@ static void pc87312_realize(DeviceState *dev, Error **errp) > > d = DEVICE(isa); > > qdev_prop_set_uint32(d, "iobase", get_fdc_iobase(s)); > > qdev_prop_set_uint32(d, "irq", 6); > > + /* FIXME use a qdev drive property instead of drive_get() */ > > drive = drive_get(IF_FLOPPY, 0, 0); > > if (drive != NULL) { > > qdev_prop_set_drive_nofail(d, "driveA", > > blk_by_legacy_dinfo(drive)); > > } > > + /* FIXME use a qdev drive property instead of drive_get() */ > > drive = drive_get(IF_FLOPPY, 0, 1); > > if (drive != NULL) { > > qdev_prop_set_drive_nofail(d, "driveB", > > As can be seen, there are drive properties driveA and driveB already on > the destination device. How do you imagine this to be fixed? Add alias > properties on the container device? (CC'ing Stefan) Yes, creating the children in pc87312_isa and setting the properties on pc87312 in the PReP board creation code. Paolo
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c index a16831c..da02932 100644 --- a/hw/arm/spitz.c +++ b/hw/arm/spitz.c @@ -168,6 +168,7 @@ static int sl_nand_init(SysBusDevice *dev) DriveInfo *nand; s->ctl = 0; + /* FIXME use a qdev drive property instead of drive_get() */ nand = drive_get(IF_MTD, 0, 0); s->nand = nand_init(nand ? blk_by_legacy_dinfo(nand) : NULL, s->manf_id, s->chip_id); diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index ff1106b..afe243b 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -623,6 +623,7 @@ static int m25p80_init(SSISlave *ss) s->dirty_page = -1; s->storage = blk_blockalign(s->blk, s->size); + /* FIXME use a qdev drive property instead of drive_get_next() */ dinfo = drive_get_next(IF_MTD); if (dinfo) { diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c index 40a1106..2849e8d 100644 --- a/hw/isa/pc87312.c +++ b/hw/isa/pc87312.c @@ -319,11 +319,13 @@ static void pc87312_realize(DeviceState *dev, Error **errp) d = DEVICE(isa); qdev_prop_set_uint32(d, "iobase", get_fdc_iobase(s)); qdev_prop_set_uint32(d, "irq", 6); + /* FIXME use a qdev drive property instead of drive_get() */ drive = drive_get(IF_FLOPPY, 0, 0); if (drive != NULL) { qdev_prop_set_drive_nofail(d, "driveA", blk_by_legacy_dinfo(drive)); } + /* FIXME use a qdev drive property instead of drive_get() */ drive = drive_get(IF_FLOPPY, 0, 1); if (drive != NULL) { qdev_prop_set_drive_nofail(d, "driveB", diff --git a/hw/sd/milkymist-memcard.c b/hw/sd/milkymist-memcard.c index 9661eaf..0cc53d9 100644 --- a/hw/sd/milkymist-memcard.c +++ b/hw/sd/milkymist-memcard.c @@ -255,6 +255,7 @@ static int milkymist_memcard_init(SysBusDevice *dev) DriveInfo *dinfo; BlockBackend *blk; + /* FIXME use a qdev drive property instead of drive_get_next() */ dinfo = drive_get_next(IF_SD); blk = dinfo ? blk_by_legacy_dinfo(dinfo) : NULL; s->card = sd_init(blk, false); diff --git a/hw/sd/pl181.c b/hw/sd/pl181.c index e704b6e..bf37da6 100644 --- a/hw/sd/pl181.c +++ b/hw/sd/pl181.c @@ -490,6 +490,7 @@ static int pl181_init(SysBusDevice *sbd) sysbus_init_irq(sbd, &s->irq[0]); sysbus_init_irq(sbd, &s->irq[1]); qdev_init_gpio_out(dev, s->cardstatus, 2); + /* FIXME use a qdev drive property instead of drive_get_next() */ dinfo = drive_get_next(IF_SD); s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false); if (s->card == NULL) { diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 27b914a..f056c52 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1146,6 +1146,7 @@ static void sdhci_initfn(SDHCIState *s) { DriveInfo *di; + /* FIXME use a qdev drive property instead of drive_get_next() */ di = drive_get_next(IF_SD); s->card = sd_init(di ? blk_by_legacy_dinfo(di) : NULL, false); if (s->card == NULL) { diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c index a71fbca..e4b2d4f 100644 --- a/hw/sd/ssi-sd.c +++ b/hw/sd/ssi-sd.c @@ -255,6 +255,7 @@ static int ssi_sd_init(SSISlave *d) DriveInfo *dinfo; s->mode = SSI_SD_CMD; + /* FIXME use a qdev drive property instead of drive_get_next() */ dinfo = drive_get_next(IF_SD); s->sd = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, true); if (s->sd == NULL) {
Drives defined with if!=none are for board initialization to wire up. Board code calls drive_get() or similar to find them, and creates devices with their qdev drive properties set accordingly. Except a few devices go on a fishing expedition for a suitable backend instead of exposing a drive property for board code to set: they call driver_get() or drive_get_next() in their realize() or init() method to implicitly connect to the "next" backend with a certain interface type. Picking up backends that way works when the devices are created by board code. But it's inappropriate for -device or device_add. Not only is this inconsistent with how the other block device models work (they connect to a backend explicitly identified by a "drive" property), it breaks when the "next" backend has been picked up by the board already. Example: $ qemu-system-arm -S -M connex -pflash flash.img -device ssi-sd Aborted (core dumped) Mark them with suitable FIXME comments. Cc: Andrzej Zaborowski <balrogg@gmail.com> Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Cc: "Andreas Färber" <andreas.faerber@web.de> Cc: Michael Walle <michael@walle.cc> Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/arm/spitz.c | 1 + hw/block/m25p80.c | 1 + hw/isa/pc87312.c | 2 ++ hw/sd/milkymist-memcard.c | 1 + hw/sd/pl181.c | 1 + hw/sd/sdhci.c | 1 + hw/sd/ssi-sd.c | 1 + 7 files changed, 8 insertions(+)