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

[Paolo Bonzini]

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