From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34640) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YZyNV-0006hz-9P for qemu-devel@nongnu.org; Mon, 23 Mar 2015 05:10:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YZyNQ-0003xn-Qr for qemu-devel@nongnu.org; Mon, 23 Mar 2015 05:10:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52907) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YZyNQ-0003xb-KD for qemu-devel@nongnu.org; Mon, 23 Mar 2015 05:10:28 -0400 From: Markus Armbruster References: <1426953265-19940-1-git-send-email-pbonzini@redhat.com> Date: Mon, 23 Mar 2015 10:10:21 +0100 In-Reply-To: <1426953265-19940-1-git-send-email-pbonzini@redhat.com> (Paolo Bonzini's message of "Sat, 21 Mar 2015 16:54:25 +0100") Message-ID: <87twxct6yq.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: "Edgar E. Iglesias" , Peter Crosthwaite , qemu-devel@nongnu.org, Peter Maydell Paolo Bonzini 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 > --- > 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;