From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46487) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ya177-0003ZX-7p for qemu-devel@nongnu.org; Mon, 23 Mar 2015 08:05:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ya173-0001Mn-HB for qemu-devel@nongnu.org; Mon, 23 Mar 2015 08:05:49 -0400 Received: from mail-wg0-x22a.google.com ([2a00:1450:400c:c00::22a]:36130) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ya173-0001Me-4P for qemu-devel@nongnu.org; Mon, 23 Mar 2015 08:05:45 -0400 Received: by wgra20 with SMTP id a20so144068957wgr.3 for ; Mon, 23 Mar 2015 05:05:44 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <55100193.5000701@redhat.com> Date: Mon, 23 Mar 2015 13:05:39 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1426953265-19940-1-git-send-email-pbonzini@redhat.com> <87twxct6yq.fsf@blackfin.pond.sub.org> In-Reply-To: <87twxct6yq.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit 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: Markus Armbruster Cc: "Edgar E. Iglesias" , Peter Crosthwaite , qemu-devel@nongnu.org, 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