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

[Peter Crosthwaite]

On Mon, Mar 23, 2015 at 7:05 PM, Markus Armbruster <address@hidden> wrote:
> Paolo Bonzini <address@hidden> 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.
>