Re: [Qemu-devel] [PATCH for-2.3] sdhci: add "drive" property

[Paolo Bonzini]

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