* [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property @ 2015-03-21 15:54 Paolo Bonzini 2015-03-23 9:10 ` Markus Armbruster 0 siblings, 1 reply; 12+ messages in thread From: Paolo Bonzini @ 2015-03-21 15:54 UTC (permalink / raw) To: qemu-devel; +Cc: Edgar E. Iglesias, Peter Crosthwaite, armbru, Peter Maydell Add a drive property that can be used with sdhci-pci (instead of the odd "-drive if=sd,... -device sdhci-pci" that has no equivalent in the rest of QEMU). Then adjust the zynq platform to set it based on the DriveInfo that it gets. Note that in this case the BlockBackend is attached to the SDHCI controller. This is not a problem; the ->dev field is really unused and the dev_opaque still points to the SDState struct. Required for 2.3, as it changes how users access the sdhci-pci device. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/arm/xilinx_zynq.c | 9 +++++++++ hw/sd/sd.c | 4 +++- hw/sd/sdhci.c | 6 ++---- hw/sd/sdhci.h | 1 + 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c index 5c37521..5d2b262 100644 --- a/hw/arm/xilinx_zynq.c +++ b/hw/arm/xilinx_zynq.c @@ -114,6 +114,7 @@ static void zynq_init(MachineState *machine) MemoryRegion *ext_ram = g_new(MemoryRegion, 1); MemoryRegion *ocm_ram = g_new(MemoryRegion, 1); DeviceState *dev; + DriveInfo *dinfo; SysBusDevice *busdev; qemu_irq pic[64]; Error *err = NULL; @@ -217,11 +218,19 @@ static void zynq_init(MachineState *machine) gem_init(&nd_table[1], 0xE000C000, pic[77-IRQ_OFFSET]); dev = qdev_create(NULL, "generic-sdhci"); + dinfo = drive_get_next(IF_SD); + if (dinfo) { + qdev_prop_set_drive_nofail(dev, "drive", blk_by_legacy_dinfo(dinfo)); + } qdev_init_nofail(dev); sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0100000); sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[56-IRQ_OFFSET]); dev = qdev_create(NULL, "generic-sdhci"); + dinfo = drive_get_next(IF_SD); + if (dinfo) { + qdev_prop_set_drive_nofail(dev, "drive", blk_by_legacy_dinfo(dinfo)); + } qdev_init_nofail(dev); sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000); sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]); diff --git a/hw/sd/sd.c b/hw/sd/sd.c index f955265..ff6bc6d 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -494,7 +494,9 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) sd->enable = true; sd_reset(sd, blk); if (sd->blk) { - blk_attach_dev_nofail(sd->blk, sd); + if (!blk_get_attached_dev(sd->blk)) { + blk_attach_dev_nofail(sd->blk, sd); + } blk_set_dev_ops(sd->blk, &sd_block_ops, sd); } vmstate_register(NULL, -1, &sd_vmstate, sd); diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index 27b914a..078b0bd 100644 --- a/hw/sd/sdhci.c +++ b/hw/sd/sdhci.c @@ -1144,10 +1144,7 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s) static void sdhci_initfn(SDHCIState *s) { - DriveInfo *di; - - di = drive_get_next(IF_SD); - s->card = sd_init(di ? blk_by_legacy_dinfo(di) : NULL, false); + s->card = sd_init(s->blk, false); if (s->card == NULL) { exit(1); } @@ -1217,6 +1214,7 @@ static Property sdhci_properties[] = { DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, SDHC_CAPAB_REG_DEFAULT), DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0), + DEFINE_PROP_DRIVE("drive", SDHCIState, blk), DEFINE_PROP_END_OF_LIST(), }; diff --git a/hw/sd/sdhci.h b/hw/sd/sdhci.h index 3352d23..69ccf58 100644 --- a/hw/sd/sdhci.h +++ b/hw/sd/sdhci.h @@ -237,6 +237,7 @@ typedef struct SDHCIState { PCIDevice pcidev; SysBusDevice busdev; }; + BlockBackend *blk; SDState *card; MemoryRegion iomem; -- 2.3.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-21 15:54 [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property Paolo Bonzini @ 2015-03-23 9:10 ` Markus Armbruster 2015-03-23 12:05 ` Paolo Bonzini 0 siblings, 1 reply; 12+ messages in thread From: Markus Armbruster @ 2015-03-23 9:10 UTC (permalink / raw) To: Paolo Bonzini Cc: Edgar E. Iglesias, Peter Crosthwaite, qemu-devel, Peter Maydell Paolo Bonzini <pbonzini@redhat.com> writes: > Add a drive property that can be used with sdhci-pci (instead of the odd > "-drive if=sd,... -device sdhci-pci" that has no equivalent in the rest > of QEMU). Ugh. sdhci-pci is incorrectly qdevified: it uses drive_get_next() in its realize() method. Is it the only device model with this issue? > Then adjust the zynq platform to set it based on the DriveInfo > that it gets. > > Note that in this case the BlockBackend is attached to the SDHCI controller. > This is not a problem; the ->dev field is really unused and the dev_opaque > still points to the SDState struct. Uh-oh, sounds fishy... > Required for 2.3, as it changes how users access the sdhci-pci device. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/arm/xilinx_zynq.c | 9 +++++++++ > hw/sd/sd.c | 4 +++- > hw/sd/sdhci.c | 6 ++---- > hw/sd/sdhci.h | 1 + > 4 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c > index 5c37521..5d2b262 100644 > --- a/hw/arm/xilinx_zynq.c > +++ b/hw/arm/xilinx_zynq.c > @@ -114,6 +114,7 @@ static void zynq_init(MachineState *machine) > MemoryRegion *ext_ram = g_new(MemoryRegion, 1); > MemoryRegion *ocm_ram = g_new(MemoryRegion, 1); > DeviceState *dev; > + DriveInfo *dinfo; > SysBusDevice *busdev; > qemu_irq pic[64]; > Error *err = NULL; > @@ -217,11 +218,19 @@ static void zynq_init(MachineState *machine) > gem_init(&nd_table[1], 0xE000C000, pic[77-IRQ_OFFSET]); > > dev = qdev_create(NULL, "generic-sdhci"); > + dinfo = drive_get_next(IF_SD); > + if (dinfo) { > + qdev_prop_set_drive_nofail(dev, "drive", blk_by_legacy_dinfo(dinfo)); > + } > qdev_init_nofail(dev); > sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0100000); > sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[56-IRQ_OFFSET]); > > dev = qdev_create(NULL, "generic-sdhci"); > + dinfo = drive_get_next(IF_SD); > + if (dinfo) { > + qdev_prop_set_drive_nofail(dev, "drive", blk_by_legacy_dinfo(dinfo)); > + } > qdev_init_nofail(dev); > sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xE0101000); > sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[79-IRQ_OFFSET]); I dislike drive_get_next(), because it makes the unit number implicit in the order of calls. Explicit would be easier to understand, and make breaking ABI harder. But as long as we do it elsewhere, you get to do it here. > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index f955265..ff6bc6d 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -494,7 +494,9 @@ SDState *sd_init(BlockBackend *blk, bool is_spi) > sd->enable = true; > sd_reset(sd, blk); > if (sd->blk) { > - blk_attach_dev_nofail(sd->blk, sd); > + if (!blk_get_attached_dev(sd->blk)) { > + blk_attach_dev_nofail(sd->blk, sd); > + } > blk_set_dev_ops(sd->blk, &sd_block_ops, sd); > } > vmstate_register(NULL, -1, &sd_vmstate, sd); Oww! This is *nasty*. Block frontends and backends are connected both ways: * The backend is a BlockBackend. * The frontend can be any kind of modern device model, i.e. subtype of TYPE_DEVICE. Or it can be a non-qdevified one. - If it's modern, it has a property pointing to the backend. It's named "drive" or "driveFOO" by convention. It's defined with DEFINE_PROP_DRIVE(). The property is implemented with a BlockBackend * member in the instance struct. Generally named blk. Fully-featured block devices generally have it within a BlockConf member (by convention named conf), and use DEFINE_BLOCK_PROPERTIES(). Example: "scsi-hd" & friends. - If it's non-qdevified, it has a BlockConf * member, which is set by a the device creation function. Example: sd_init(). * The backend has a pointer to the frontend This is BlockBackend member dev. - If the frontend is modern, it points to a DeviceState. - If it's non-qdevified, it points to whatever. Therefore, dev is void *. This connection is managed with blk_attach_dev(), blk_detach_dev() & friends. * The backend has an *optional* set of frontend callbacks This is BlockBackend members dev_ops, dev_opaque. The former is a bunch of function pointers, and the latter is a void * to go with it. Why do we need the latter? Consider a device model with multiple backends. Passing just dev to callback would only identify the device, but not which of its backends. Example: the floppy controller models the controller and all the drives connected to it (this is arguably a modeling mistake). dev points to the device model, but dev_opaque points *into* the device model, namely to its member drives[i]. The frontend callbacks are installed with blk_set_dev_ops(). Now let's see how your patch uses this stuff. We have two device models in play: the SD controller (either sdhci-pci or generic-sdhci, both modern), and the SD card (non-qdevified sd.c). Before the patch: The SD card is a block frontend, and as such has a member BlockBackend *blk in its struct SDState. Relevant part of sd_init(): if (sd->blk) { blk_attach_dev_nofail(sd->blk, sd); blk_set_dev_ops(sd->blk, &sd_block_ops, sd); } Backend's dev points to SDState, and dev_opaque, too. Exactly how single-backend non-qdevified frontends are supposed to work. The SD controller is *not* a block frontend. It knows nothing about BlockBackend. After the patch: The SD controller is now a block backend. Both the SD controller and the SD card point to the same backend. The backend now points to the SD controller instead of the SD card. Thus, the SD card no longer plays the block frontend role when its sitting in a generic-sdhci or sdhci-pci controller. It still plays it when its sitting in something else, and because of that we have this identity crisis code in sd_init(): if (!blk_get_attached_dev(sd->blk)) { blk_attach_dev_nofail(sd->blk, sd); } blk_set_dev_ops(sd->blk, &sd_block_ops, sd); Take the frontend role, unless something else already took it. But still install frontend callbacks, on the (correct) assumption that whatever took the frontend role didn't also install them. >From 30,000ft, this looks a bit like the floppy case: BB's dev points to the block device, and BB's dev_opaque points within the device. If you descend a bit, it looks a lot more like the usb-storage hack that has caused us nothing but grief: two separate device models attaching to the same BlockBackend. What is the usb-storage hack? Device model usb-storage pretends to be a block device, but really is a SCSI controller that can serve just one SCSI device, which it creates automatically, in its realize() method. Since the automatically created device isn't accessible at -device / device_add level, we need to stick the drive property for it into usb-storage. Before the realize() method creates the SCSI device, it carefully detaches the usb-storage device: /* * Hack alert: this pretends to be a block device, but it's really * a SCSI bus that can serve only a single device, which it * creates automatically. But first it needs to detach from its * blockdev, or else scsi_bus_legacy_add_drive() dies when it * attaches again. * * The hack is probably a bad idea. */ blk_detach_dev(blk, &s->dev.qdev); s->conf.blk = NULL; Bad idea, but ABI. Before we make another bad idea ABI, let's stop and think. I believe the proper solution for your problem is qdevifying the SD card. If we can't do that for 2.3, and we absolutely need *something* for 2.3 (do we?), we should still consider whether that something will get in the way of the proper solution. > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c > index 27b914a..078b0bd 100644 > --- a/hw/sd/sdhci.c > +++ b/hw/sd/sdhci.c > @@ -1144,10 +1144,7 @@ static inline unsigned int sdhci_get_fifolen(SDHCIState *s) > > static void sdhci_initfn(SDHCIState *s) > { > - DriveInfo *di; > - > - di = drive_get_next(IF_SD); > - s->card = sd_init(di ? blk_by_legacy_dinfo(di) : NULL, false); > + s->card = sd_init(s->blk, false); > if (s->card == NULL) { > exit(1); > } > @@ -1217,6 +1214,7 @@ static Property sdhci_properties[] = { > DEFINE_PROP_UINT32("capareg", SDHCIState, capareg, > SDHC_CAPAB_REG_DEFAULT), > DEFINE_PROP_UINT32("maxcurr", SDHCIState, maxcurr, 0), > + DEFINE_PROP_DRIVE("drive", SDHCIState, blk), > DEFINE_PROP_END_OF_LIST(), > }; > > diff --git a/hw/sd/sdhci.h b/hw/sd/sdhci.h > index 3352d23..69ccf58 100644 > --- a/hw/sd/sdhci.h > +++ b/hw/sd/sdhci.h > @@ -237,6 +237,7 @@ typedef struct SDHCIState { > PCIDevice pcidev; > SysBusDevice busdev; > }; > + BlockBackend *blk; > SDState *card; > MemoryRegion iomem; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 9:10 ` Markus Armbruster @ 2015-03-23 12:05 ` Paolo Bonzini 2015-03-23 12:09 ` Peter Maydell 2015-03-23 13:35 ` Markus Armbruster 0 siblings, 2 replies; 12+ messages in thread From: Paolo Bonzini @ 2015-03-23 12:05 UTC (permalink / raw) To: Markus Armbruster Cc: Edgar E. Iglesias, Peter Crosthwaite, qemu-devel, Peter Maydell On 23/03/2015 10:10, Markus Armbruster wrote: > Ugh. sdhci-pci is incorrectly qdevified: it uses drive_get_next() in > its realize() method. Is it the only device model with this issue? I think SD devices are the only ones, but all of them have the issue. Ironically, the qdevified ones call drive_get_next(), while the others get a BlockBackend from the outside. > I dislike drive_get_next(), because it makes the unit number implicit in > the order of calls. Explicit would be easier to understand, and make > breaking ABI harder. But as long as we do it elsewhere, you get to do > it here. On the other hand, drive_get_next() is exactly what the code used to do and *also* makes breaking ABI harder... as long as we're in hard freeze. > From 30,000ft, this looks a bit like the floppy case: BB's dev points to > the block device, and BB's dev_opaque points within the device. > > If you descend a bit, it looks a lot more like the usb-storage hack that > has caused us nothing but grief: two separate device models attaching to > the same BlockBackend. > > What is the usb-storage hack? Device model usb-storage pretends to be a > block device, but really is a SCSI controller that can serve just one > SCSI device, which it creates automatically, in its realize() method. > Since the automatically created device isn't accessible at -device / > device_add level, we need to stick the drive property for it into > usb-storage. Before the realize() method creates the SCSI device, it > carefully detaches the usb-storage device: > > /* > * Hack alert: this pretends to be a block device, but it's really > * a SCSI bus that can serve only a single device, which it > * creates automatically. But first it needs to detach from its > * blockdev, or else scsi_bus_legacy_add_drive() dies when it > * attaches again. > * > * The hack is probably a bad idea. > */ > blk_detach_dev(blk, &s->dev.qdev); > s->conf.blk = NULL; > > Bad idea, but ABI. > > Before we make another bad idea ABI, let's stop and think. > > I believe the proper solution for your problem is qdevifying the SD > card. The question is whether there is a use for qdevifying the SD card. Each SD/MMC controller will have exactly zero or one SD cards, but the hw/sd/sd.c interface already treats "BlockBackend ejected" as "zero SD cards": if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable) { return 0; } Unlike SCSI, the SD card code: 1) doesn't need multiplexing (a la scsi-hd/scsi-cd/scsi-generic) 2) doesn't have a bus to talk on (real-world SD cards are just connected with GPIO pins; hw/sd/sd.c abstracts the bitbanging protocol but still there is only one device to talk to) So in the end I think it's easier to treat hw/sd/sd.c as the common code for all hw/sd/* devices, like e.g. hw/display/vga.c. > If we can't do that for 2.3, and we absolutely need *something* for 2.3 > (do we?), we should still consider whether that something will get in > the way of the proper solution. If you want me to fix the sd.c identity crisis for 2.3, and remove blk_attach_dev I can do it. It will be a series of patches much like this one, so this one in particular doesn't get in the way. The only alternative for 2.3 is reverting the patch for sdhci-pci. I certainly don't want "-drive if=sd -device sdhci-pci" to become ABI! Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 12:05 ` Paolo Bonzini @ 2015-03-23 12:09 ` Peter Maydell 2015-03-23 13:19 ` Paolo Bonzini 2015-03-23 13:35 ` Markus Armbruster 1 sibling, 1 reply; 12+ messages in thread From: Peter Maydell @ 2015-03-23 12:09 UTC (permalink / raw) To: Paolo Bonzini Cc: Edgar E. Iglesias, Peter Crosthwaite, Markus Armbruster, QEMU Developers On 23 March 2015 at 12:05, Paolo Bonzini <pbonzini@redhat.com> wrote: > > The only alternative for 2.3 is reverting the patch for sdhci-pci. I > certainly don't want "-drive if=sd -device sdhci-pci" to become ABI! What's wrong with that, incidentally? It's what I would have expected for connecting an SD card to sdhci-pci, given how the other SD controller devices work. (The only difference with sdhci-pci is it happens to be a pluggable device rather than hardwired into a board model.) -- PMM ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 12:09 ` Peter Maydell @ 2015-03-23 13:19 ` Paolo Bonzini 2015-03-23 13:43 ` Markus Armbruster 0 siblings, 1 reply; 12+ messages in thread From: Paolo Bonzini @ 2015-03-23 13:19 UTC (permalink / raw) To: Peter Maydell Cc: Edgar E. Iglesias, Peter Crosthwaite, Markus Armbruster, QEMU Developers On 23/03/2015 13:09, Peter Maydell wrote: > > The only alternative for 2.3 is reverting the patch for sdhci-pci. I > > certainly don't want "-drive if=sd -device sdhci-pci" to become ABI! > > What's wrong with that, incidentally? It's what I would have > expected for connecting an SD card to sdhci-pci, given > how the other SD controller devices work. (The only > difference with sdhci-pci is it happens to be a > pluggable device rather than hardwired into a board model.) Usually, "-drive if=" options (apart from if=none) are only handled at board creation time, and is not affected by -device. PCI devices use "-device sdhci-pci,drive=xyz" to tie themselves to a -drive option. Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 13:19 ` Paolo Bonzini @ 2015-03-23 13:43 ` Markus Armbruster 0 siblings, 0 replies; 12+ messages in thread From: Markus Armbruster @ 2015-03-23 13:43 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Peter Crosthwaite, QEMU Developers, Edgar E. Iglesias Paolo Bonzini <pbonzini@redhat.com> writes: > On 23/03/2015 13:09, Peter Maydell wrote: >> > The only alternative for 2.3 is reverting the patch for sdhci-pci. I >> > certainly don't want "-drive if=sd -device sdhci-pci" to become ABI! >> >> What's wrong with that, incidentally? It's what I would have >> expected for connecting an SD card to sdhci-pci, given >> how the other SD controller devices work. (The only >> difference with sdhci-pci is it happens to be a >> pluggable device rather than hardwired into a board model.) > > Usually, "-drive if=" options (apart from if=none) are only handled at > board creation time, and is not affected by -device. PCI devices use > "-device sdhci-pci,drive=xyz" to tie themselves to a -drive option. Yes. In other words: realize() methods aren't supposed to go on fishing expeditions for backends. They should rely on their backend properties. The disgusting way we implement if=scsi should serve as deterrent, not as example to emulate. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 12:05 ` Paolo Bonzini 2015-03-23 12:09 ` Peter Maydell @ 2015-03-23 13:35 ` Markus Armbruster 2015-03-23 13:56 ` Paolo Bonzini 2015-03-23 15:01 ` Peter Crosthwaite 1 sibling, 2 replies; 12+ messages in thread From: Markus Armbruster @ 2015-03-23 13:35 UTC (permalink / raw) To: Paolo Bonzini Cc: Edgar E. Iglesias, Peter Crosthwaite, qemu-devel, Peter Maydell Paolo Bonzini <pbonzini@redhat.com> writes: > On 23/03/2015 10:10, Markus Armbruster wrote: >> Ugh. sdhci-pci is incorrectly qdevified: it uses drive_get_next() in >> its realize() method. Is it the only device model with this issue? > > I think SD devices are the only ones, but all of them have the issue. > > Ironically, the qdevified ones call drive_get_next(), while the others > get a BlockBackend from the outside. > >> I dislike drive_get_next(), because it makes the unit number implicit in >> the order of calls. Explicit would be easier to understand, and make >> breaking ABI harder. But as long as we do it elsewhere, you get to do >> it here. > > On the other hand, drive_get_next() is exactly what the code used to do > and *also* makes breaking ABI harder... as long as we're in hard freeze. Sticking to drive_get_next() for 2.3 makes sense. >> From 30,000ft, this looks a bit like the floppy case: BB's dev points to >> the block device, and BB's dev_opaque points within the device. >> >> If you descend a bit, it looks a lot more like the usb-storage hack that >> has caused us nothing but grief: two separate device models attaching to >> the same BlockBackend. >> >> What is the usb-storage hack? Device model usb-storage pretends to be a >> block device, but really is a SCSI controller that can serve just one >> SCSI device, which it creates automatically, in its realize() method. >> Since the automatically created device isn't accessible at -device / >> device_add level, we need to stick the drive property for it into >> usb-storage. Before the realize() method creates the SCSI device, it >> carefully detaches the usb-storage device: >> >> /* >> * Hack alert: this pretends to be a block device, but it's really >> * a SCSI bus that can serve only a single device, which it >> * creates automatically. But first it needs to detach from its >> * blockdev, or else scsi_bus_legacy_add_drive() dies when it >> * attaches again. >> * >> * The hack is probably a bad idea. >> */ >> blk_detach_dev(blk, &s->dev.qdev); >> s->conf.blk = NULL; >> >> Bad idea, but ABI. >> >> Before we make another bad idea ABI, let's stop and think. >> >> I believe the proper solution for your problem is qdevifying the SD >> card. > > The question is whether there is a use for qdevifying the SD card. Okay, that's a fair question. > Each SD/MMC controller will have exactly zero or one SD cards, but the > hw/sd/sd.c interface already treats "BlockBackend ejected" as "zero SD > cards": > > if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable) { > return 0; > } > > Unlike SCSI, the SD card code: > > 1) doesn't need multiplexing (a la scsi-hd/scsi-cd/scsi-generic) > > 2) doesn't have a bus to talk on (real-world SD cards are just connected > with GPIO pins; hw/sd/sd.c abstracts the bitbanging protocol but still > there is only one device to talk to) > > So in the end I think it's easier to treat hw/sd/sd.c as the common code > for all hw/sd/* devices, like e.g. hw/display/vga.c. To pick a block device precedent: like floppy. I don't like that the floppy controller and its drives are fused. However, the fusing has been *much* less grief than the usb-storage hack: basically just a weird user interface to configure the drives, namely --global instead of --device. If sd.c is common code rather than a device model in its own right, perhaps SDState should be unboxed in SDHCIState, just like the FDrive are unboxed in FDCtrl. The "drive" property can then be connected straight to SDState member blk. >> If we can't do that for 2.3, and we absolutely need *something* for 2.3 >> (do we?), we should still consider whether that something will get in >> the way of the proper solution. > > If you want me to fix the sd.c identity crisis for 2.3, and remove > blk_attach_dev I can do it. It will be a series of patches much like > this one, so this one in particular doesn't get in the way. Perhaps split sd_init() into two parts: an inner, "common code" part, and an outer, "independend non-qdevified device" part. // Inner FIXME pick a sane name // TODO should spi be a property? if yes, drop @is_spi! void sd_init1(SDState *sd, bool is_spi, Error **errp) { if (sd->blk && blk_is_read_only(sd->blk)) { error_setg(errp, "Cannot use read-only drive"); return NULL; } sd->buf = blk_blockalign(sd->blk, 512); sd->spi = is_spi; sd->enable = true; sd_reset(sd); if (sd->sd->blk) { blk_set_dev_ops(sd->blk, &sd_block_ops, sd); } vmstate_register(NULL, -1, &sd_vmstate, sd); } // Outer SDState *sd_init(BlockBackend *blk, bool is_spi) { Error *err = NULL; SDState *sd; sd = g_malloc0(sizeof(SDState)); sd->blk = blk; sd_init1(sd, is_spi, &err); if (err) { error_report_err(err); free(sd); return NULL; } if (sd->blk) { blk_attach_dev_nofail(sd->blk, sd); } return sd; } The qdevified controller devices use only the inner part. The outer part dies when the last controller gets qdevified. > The only alternative for 2.3 is reverting the patch for sdhci-pci. I > certainly don't want "-drive if=sd -device sdhci-pci" to become ABI! Good grief, no :) If we can't get things sufficiently right in time for 2.3, we should revert or disable the device just for the release, then whip it into shape in the next development cycle. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 13:35 ` Markus Armbruster @ 2015-03-23 13:56 ` Paolo Bonzini 2015-03-23 15:01 ` Peter Crosthwaite 1 sibling, 0 replies; 12+ messages in thread From: Paolo Bonzini @ 2015-03-23 13:56 UTC (permalink / raw) To: Markus Armbruster Cc: Edgar E. Iglesias, Peter Crosthwaite, qemu-devel, Peter Maydell On 23/03/2015 14:35, Markus Armbruster wrote: >>> From 30,000ft, this looks a bit like the floppy case: BB's dev points to >>> the block device, and BB's dev_opaque points within the device. >>> >>> If you descend a bit, it looks a lot more like the usb-storage hack that >>> has caused us nothing but grief: two separate device models attaching to >>> the same BlockBackend. >>> >>> What is the usb-storage hack? Device model usb-storage pretends to be a >>> block device, but really is a SCSI controller that can serve just one >>> SCSI device, which it creates automatically, in its realize() method. >>> Since the automatically created device isn't accessible at -device / >>> device_add level, we need to stick the drive property for it into >>> usb-storage. Before the realize() method creates the SCSI device, it >>> carefully detaches the usb-storage device: >>> >>> /* >>> * Hack alert: this pretends to be a block device, but it's really >>> * a SCSI bus that can serve only a single device, which it >>> * creates automatically. But first it needs to detach from its >>> * blockdev, or else scsi_bus_legacy_add_drive() dies when it >>> * attaches again. >>> * >>> * The hack is probably a bad idea. >>> */ >>> blk_detach_dev(blk, &s->dev.qdev); >>> s->conf.blk = NULL; >>> >>> Bad idea, but ABI. >>> >>> Before we make another bad idea ABI, let's stop and think. >>> >>> I believe the proper solution for your problem is qdevifying the SD >>> card. >> >> The question is whether there is a use for qdevifying the SD card. > > Okay, that's a fair question. > >> Each SD/MMC controller will have exactly zero or one SD cards, but the >> hw/sd/sd.c interface already treats "BlockBackend ejected" as "zero SD >> cards": >> >> if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable) { >> return 0; >> } >> >> Unlike SCSI, the SD card code: >> >> 1) doesn't need multiplexing (a la scsi-hd/scsi-cd/scsi-generic) >> >> 2) doesn't have a bus to talk on (real-world SD cards are just connected >> with GPIO pins; hw/sd/sd.c abstracts the bitbanging protocol but still >> there is only one device to talk to) >> >> So in the end I think it's easier to treat hw/sd/sd.c as the common code >> for all hw/sd/* devices, like e.g. hw/display/vga.c. > > To pick a block device precedent: like floppy. I picked VGA because you have devices doing fancy stuff on top of the common code, but floppy and AHCI are similar too. > I don't like that the floppy controller and its drives are fused. > However, the fusing has been *much* less grief than the usb-storage > hack: basically just a weird user interface to configure the drives, > namely --global instead of --device. Right, which we don't even have in the case of sdhci-pci. > If sd.c is common code rather than a device model in its own right, > perhaps SDState should be unboxed in SDHCIState, just like the FDrive > are unboxed in FDCtrl. The "drive" property can then be connected > straight to SDState member blk. Yes, of course not 2.3 work. Added to BiteSizedTasks. > Perhaps split sd_init() into two parts: an inner, "common code" part, > and an outer, "independend non-qdevified device" part. I think I'm just going to move blk_attach_dev_nofail to the callers, so that then this patch can just remove the call in sdhci.c. Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 13:35 ` Markus Armbruster 2015-03-23 13:56 ` Paolo Bonzini @ 2015-03-23 15:01 ` Peter Crosthwaite 2015-03-23 15:15 ` Peter Maydell ` (2 more replies) 1 sibling, 3 replies; 12+ messages in thread From: Peter Crosthwaite @ 2015-03-23 15:01 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Paolo Bonzini, qemu-devel@nongnu.org Developers, Edgar E. Iglesias On Mon, Mar 23, 2015 at 7:05 PM, Markus Armbruster <armbru@redhat.com> wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 23/03/2015 10:10, Markus Armbruster wrote: >>> Ugh. sdhci-pci is incorrectly qdevified: it uses drive_get_next() in >>> its realize() method. Is it the only device model with this issue? >> >> I think SD devices are the only ones, but all of them have the issue. >> >> Ironically, the qdevified ones call drive_get_next(), while the others >> get a BlockBackend from the outside. >> >>> I dislike drive_get_next(), because it makes the unit number implicit in >>> the order of calls. Explicit would be easier to understand, and make >>> breaking ABI harder. But as long as we do it elsewhere, you get to do >>> it here. >> >> On the other hand, drive_get_next() is exactly what the code used to do >> and *also* makes breaking ABI harder... as long as we're in hard freeze. > > Sticking to drive_get_next() for 2.3 makes sense. > >>> From 30,000ft, this looks a bit like the floppy case: BB's dev points to >>> the block device, and BB's dev_opaque points within the device. >>> >>> If you descend a bit, it looks a lot more like the usb-storage hack that >>> has caused us nothing but grief: two separate device models attaching to >>> the same BlockBackend. >>> >>> What is the usb-storage hack? Device model usb-storage pretends to be a >>> block device, but really is a SCSI controller that can serve just one >>> SCSI device, which it creates automatically, in its realize() method. >>> Since the automatically created device isn't accessible at -device / >>> device_add level, we need to stick the drive property for it into >>> usb-storage. Before the realize() method creates the SCSI device, it >>> carefully detaches the usb-storage device: >>> >>> /* >>> * Hack alert: this pretends to be a block device, but it's really >>> * a SCSI bus that can serve only a single device, which it >>> * creates automatically. But first it needs to detach from its >>> * blockdev, or else scsi_bus_legacy_add_drive() dies when it >>> * attaches again. >>> * >>> * The hack is probably a bad idea. >>> */ >>> blk_detach_dev(blk, &s->dev.qdev); >>> s->conf.blk = NULL; >>> >>> Bad idea, but ABI. >>> >>> Before we make another bad idea ABI, let's stop and think. >>> >>> I believe the proper solution for your problem is qdevifying the SD >>> card. >> >> The question is whether there is a use for qdevifying the SD card. > > Okay, that's a fair question. > >> Each SD/MMC controller will have exactly zero or one SD cards, but the >> hw/sd/sd.c interface already treats "BlockBackend ejected" as "zero SD >> cards": >> >> if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable) { >> return 0; >> } >> >> Unlike SCSI, the SD card code: >> >> 1) doesn't need multiplexing (a la scsi-hd/scsi-cd/scsi-generic) >> >> 2) doesn't have a bus to talk on (real-world SD cards are just connected >> with GPIO pins; hw/sd/sd.c abstracts the bitbanging protocol but still >> there is only one device to talk to) >> >> So in the end I think it's easier to treat hw/sd/sd.c as the common code >> for all hw/sd/* devices, like e.g. hw/display/vga.c. > > To pick a block device precedent: like floppy. > > I don't like that the floppy controller and its drives are fused. > However, the fusing has been *much* less grief than the usb-storage > hack: basically just a weird user interface to configure the drives, > namely --global instead of --device. > > If sd.c is common code rather than a device model in its own right, > perhaps SDState should be unboxed in SDHCIState, just like the FDrive > are unboxed in FDCtrl. The "drive" property can then be connected > straight to SDState member blk. > sd.c for me is definitely a device in its own right, and SDHCI is quite separate. We already have multiple SD controllers connecting to SD cards. Ideally, SD should be bussified (or the modern equivalient using Links). I actually have code in my my tree that connects an SDHCI to two cards with muxing logic handled by another peripheral. There are a wider class of SD devices that can use the SD bus beyond SD cards (although we don't model any today). Another thing to consider is MMC support which is a subtle variant on SD card. We need the QOMification of sd.c to setup user-settable props or an inheritance hierarchy for the different flavors of SD/MMC cards. I don't think rolling into the controller is a good idea as then the controller needs to take ownership of any card configuration. I think the correct way forward is the qom/qdevification. Note that SD has a SPI mode, every SD card is in theory a valid SSI device. We could unify SD under SSI to achieve both QOMification and busification. I would then expect the block setup of sd.c to be very similar to hw/block/m25p80.c (SPI flash). Regards, Peter >>> If we can't do that for 2.3, and we absolutely need *something* for 2.3 >>> (do we?), we should still consider whether that something will get in >>> the way of the proper solution. >> >> If you want me to fix the sd.c identity crisis for 2.3, and remove >> blk_attach_dev I can do it. It will be a series of patches much like >> this one, so this one in particular doesn't get in the way. > > Perhaps split sd_init() into two parts: an inner, "common code" part, > and an outer, "independend non-qdevified device" part. > > // Inner FIXME pick a sane name > // TODO should spi be a property? if yes, drop @is_spi! > void sd_init1(SDState *sd, bool is_spi, Error **errp) > { > if (sd->blk && blk_is_read_only(sd->blk)) { > error_setg(errp, "Cannot use read-only drive"); > return NULL; > } > > sd->buf = blk_blockalign(sd->blk, 512); > sd->spi = is_spi; > sd->enable = true; > sd_reset(sd); > if (sd->sd->blk) { > blk_set_dev_ops(sd->blk, &sd_block_ops, sd); > } > vmstate_register(NULL, -1, &sd_vmstate, sd); > } > > // Outer > SDState *sd_init(BlockBackend *blk, bool is_spi) > { > Error *err = NULL; > SDState *sd; > > sd = g_malloc0(sizeof(SDState)); > sd->blk = blk; > sd_init1(sd, is_spi, &err); > if (err) { > error_report_err(err); > free(sd); > return NULL; > } > if (sd->blk) { > blk_attach_dev_nofail(sd->blk, sd); > } > return sd; > } > > The qdevified controller devices use only the inner part. The outer > part dies when the last controller gets qdevified. > >> The only alternative for 2.3 is reverting the patch for sdhci-pci. I >> certainly don't want "-drive if=sd -device sdhci-pci" to become ABI! > > Good grief, no :) > > If we can't get things sufficiently right in time for 2.3, we should > revert or disable the device just for the release, then whip it into > shape in the next development cycle. > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 15:01 ` Peter Crosthwaite @ 2015-03-23 15:15 ` Peter Maydell 2015-03-23 15:58 ` Paolo Bonzini 2015-03-24 14:53 ` Markus Armbruster 2 siblings, 0 replies; 12+ messages in thread From: Peter Maydell @ 2015-03-23 15:15 UTC (permalink / raw) To: Peter Crosthwaite Cc: qemu-devel@nongnu.org Developers, Paolo Bonzini, Markus Armbruster, Edgar E. Iglesias On 23 March 2015 at 15:01, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Mon, Mar 23, 2015 at 7:05 PM, Markus Armbruster <armbru@redhat.com> wrote: >> If sd.c is common code rather than a device model in its own right, >> perhaps SDState should be unboxed in SDHCIState, just like the FDrive >> are unboxed in FDCtrl. The "drive" property can then be connected >> straight to SDState member blk. > sd.c for me is definitely a device in its own right, and SDHCI is > quite separate. We already have multiple SD controllers connecting to > SD cards. Ideally, SD should be bussified (or the modern equivalient > using Links). Yes, I think sd.c is just the way it is because it was written long before QOM was ever invented, and nobody's cared enough since to QOM and qdevify it... -- PMM ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 15:01 ` Peter Crosthwaite 2015-03-23 15:15 ` Peter Maydell @ 2015-03-23 15:58 ` Paolo Bonzini 2015-03-24 14:53 ` Markus Armbruster 2 siblings, 0 replies; 12+ messages in thread From: Paolo Bonzini @ 2015-03-23 15:58 UTC (permalink / raw) To: Peter Crosthwaite, Markus Armbruster Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Edgar E. Iglesias On 23/03/2015 16:01, Peter Crosthwaite wrote: > Note that SD has a SPI mode, every SD card is in theory a valid SSI > device. We could unify SD under SSI to achieve both QOMification and > busification. That wouldn't really be the way most SD cards work, anyway. The protocol doesn't do simultaneous bidirectional transfers like SSI does. With your proposal the right way to invoke sdhci-pci would be "-device sdhci-pci,id=sdhci -drive if=none,...,id=sd -device sd,drive=sd,bus=sdhci.0". Similarly, instantiating an SD card on an SSI bus would be "-device ssi-sd,id=ssisd -drive if=none,...,id=sd -device sd,drive=sd,bus=ssisd.0". > I would then expect the block setup of sd.c to be very similar to > hw/block/m25p80.c (SPI flash). Absolutely not, hw/block/m25p80.c has the same issue of doing drive_get_next in the realize function. Paolo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property 2015-03-23 15:01 ` Peter Crosthwaite 2015-03-23 15:15 ` Peter Maydell 2015-03-23 15:58 ` Paolo Bonzini @ 2015-03-24 14:53 ` Markus Armbruster 2 siblings, 0 replies; 12+ messages in thread From: Markus Armbruster @ 2015-03-24 14:53 UTC (permalink / raw) To: Peter Crosthwaite Cc: Peter Maydell, Edgar E. Iglesias, qemu-devel@nongnu.org Developers, Paolo Bonzini Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes: > On Mon, Mar 23, 2015 at 7:05 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 23/03/2015 10:10, Markus Armbruster wrote: [...] >>>> I believe the proper solution for your problem is qdevifying the SD >>>> card. >>> >>> The question is whether there is a use for qdevifying the SD card. >> >> Okay, that's a fair question. >> >>> Each SD/MMC controller will have exactly zero or one SD cards, but the >>> hw/sd/sd.c interface already treats "BlockBackend ejected" as "zero SD >>> cards": >>> >>> if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable) { >>> return 0; >>> } >>> >>> Unlike SCSI, the SD card code: >>> >>> 1) doesn't need multiplexing (a la scsi-hd/scsi-cd/scsi-generic) >>> >>> 2) doesn't have a bus to talk on (real-world SD cards are just connected >>> with GPIO pins; hw/sd/sd.c abstracts the bitbanging protocol but still >>> there is only one device to talk to) >>> >>> So in the end I think it's easier to treat hw/sd/sd.c as the common code >>> for all hw/sd/* devices, like e.g. hw/display/vga.c. >> >> To pick a block device precedent: like floppy. >> >> I don't like that the floppy controller and its drives are fused. >> However, the fusing has been *much* less grief than the usb-storage >> hack: basically just a weird user interface to configure the drives, >> namely --global instead of --device. >> >> If sd.c is common code rather than a device model in its own right, >> perhaps SDState should be unboxed in SDHCIState, just like the FDrive >> are unboxed in FDCtrl. The "drive" property can then be connected >> straight to SDState member blk. >> > > sd.c for me is definitely a device in its own right, and SDHCI is > quite separate. We already have multiple SD controllers connecting to > SD cards. Ideally, SD should be bussified (or the modern equivalient > using Links). I actually have code in my my tree that connects an > SDHCI to two cards with muxing logic handled by another peripheral. > > There are a wider class of SD devices that can use the SD bus beyond > SD cards (although we don't model any today). Another thing to > consider is MMC support which is a subtle variant on SD card. We need > the QOMification of sd.c to setup user-settable props or an > inheritance hierarchy for the different flavors of SD/MMC cards. I > don't think rolling into the controller is a good idea as then the > controller needs to take ownership of any card configuration. > > I think the correct way forward is the qom/qdevification. > > Note that SD has a SPI mode, every SD card is in theory a valid SSI > device. We could unify SD under SSI to achieve both QOMification and > busification. > > I would then expect the block setup of sd.c to be very similar to > hw/block/m25p80.c (SPI flash). Do you think you'll be able to work on this? It would be nice to have -device sdhci-pci working in 2.4. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-03-24 14:54 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-21 15:54 [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property Paolo Bonzini 2015-03-23 9:10 ` Markus Armbruster 2015-03-23 12:05 ` Paolo Bonzini 2015-03-23 12:09 ` Peter Maydell 2015-03-23 13:19 ` Paolo Bonzini 2015-03-23 13:43 ` Markus Armbruster 2015-03-23 13:35 ` Markus Armbruster 2015-03-23 13:56 ` Paolo Bonzini 2015-03-23 15:01 ` Peter Crosthwaite 2015-03-23 15:15 ` Peter Maydell 2015-03-23 15:58 ` Paolo Bonzini 2015-03-24 14:53 ` 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.