Re: [Qemu-devel] [PATCH 1/3] hw/sd/pxa2xx_mmci: convert to SysBusDevice object

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 4 December 2015 at 07:30, Markus Armbruster <address@hidden> wrote:
>> Peter Maydell <address@hidden> writes:
>>
>>> On 7 September 2015 at 17:57, Markus Armbruster <address@hidden> wrote:
>>>> Peter Maydell <address@hidden> writes:
>>>>
>>>>> On 7 September 2015 at 17:40, Markus Armbruster <address@hidden> wrote:
>>>>>> Peter Maydell <address@hidden> writes:
>>>>>>
>>>>>>> Convert the pxa2xx_mmci device to be a sysbus device.
>>>>>
>>>>>>> +static Property pxa2xx_mmci_properties[] = {
>>>>>>> +    /* Note: pointer property 'drive' may remain NULL, thus no need
>>>>>>> +     * for dc->cannot_instantiate_with_device_add_yet = true;
>>>>>>> +     * Unfortunately this can't be a DEFINE_PROP_DRIVE, because
>>>>>>> +     * setting a 'drive' property results in a call to blk_attach_dev()
>>>>>>> +     * attaching the BlockBackend to this device; that then means that
>>>>>>> +     * the call in sd_init() to blk_attach_dev_nofail() which tries to
>>>>>>> +     * attach the BlockBackend to the SD card object aborts.
>>>>>>> +     */
>>>>>>> +    DEFINE_PROP_PTR("drive", PXA2xxMMCIState, blk),
>>>>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>>>>> +};
>>>>>>
>>>>>> As far as I can tell, this problem is an artifact of our interface to
>>>>>> the common sd-card code, namely sd_init().  sd_init() was made for the
>>>>>> pre-qdev world: it creates and completely initializes the common
>>>>>> SDState.
>>>>>>
>>>>>> In qdev, we do this in three separate steps: create, set properties,
>>>>>> realize.  Split up sd_init(), and the problem should go away.
>>>>>
>>>>> Yes, but I don't really want to gate QOMification of mmc
>>>>> controller devices on the more complicated problem of
>>>>> QOMifying sd.c itself, especially since we already have several
>>>>> QOMified mmc controllers...
>>>>
>>>> Is serial.c QOMified?  I don't think so, it's merely structured in a
>>>> QOM-friendly way: typedef SerialState, realize helper
>>>> serial_realize_core(), unrealize helper serial_exit_core().  If
>>>> SerialState had more properties, we'd also need a macro to define them.
>>>
>>> It looks like since we had this conversation the problem has been
>>> dealt with in commit 5ec911c30ff433 by simply turning the sd_init() call
>>> to blk_attach_dev_nofail() into a call to blk_attach_dev() which ignores
>>> its error return. So I should be able to do this with a DEFINE_PROP_DRIVE
>>> now I think...
>>
>> Ignoring the error is intentional according to the comment, but why is
>> it appropriate?
>
> That seems like a question to ask the author and reviewer of that
> commit :-) [cc'd].
>
> The intention seems to have been to allow sdhci to do the same thing
> I want -- take a drive property (which attaches the BlockBackend to
> the controller device) and then hand the BlockBackend to sd_init()
> without having it blow up.
>
> Incidentally, in an ideal world wouldn't the block/drive properties
> be on the SD card object rather than the controller object ? At least,
> we seem to have that split for IDE and SCSI disks.

This is really an instance of the common split device pattern: we have a
front end (a.k.a. device model) connected to a backend (or even multiple
backends).  A character device model is connected to a CharDriverState.
A NIC device model is connected to NICPeers.  And a block device is
connected to a BlockBackend.

For qdevified devices, the connection is made with a qdev property.  A
character device model has one defined with DEFINE_PROP_CHR().  A NIC
device model has one defined with DEFINE_PROP_NETDEV(), usually via
DEFINE_NIC_PROPERTIES().  A block device model has one defined with
DEFINE_PROP_DRIVE(), often via DEFINE_BLOCK_PROPERTIES().

Backends commonly can only be connected to one frontend.  The properties
check the applicable constraints, and error out when the user tries to
violate them.  Example: if you try to connect a block backend to
multiple frontends by specifying the same drive= value with each of
them, you get an error.  Good, because without that, you'd likely get
data corruption.

To finally answer your question: the proper owner of the property
connecting the backend is the frontend half of the split device.  So, if
we have separate device models for controller and the block devices
connected to it, the block backend property belongs to the latter, not
the former.  If we have separate device models for SD controller and
card, the block backend property belongs to the card, not the
controller.

Non-qdevified devices need to setup the connection manually, making sure
applicable constraints get checked.

As I explained above, the problem with SD cards is "an artifact of our
interface to the common sd-card code": it serves both non-qdevified and
qdevified code, badly.  We should either qdevify everything, or
restructure the common code to make it serve both qdevified and
non-qdevified code well.  I acknowledge the former is a tall order.  The
latter, however, should be doable, and I pointed to similarly common
code that already does it.

Commit 5ec911c papers over the problem instead, in sd_init().  Works
basically like this:

1. Try to connect the backend, ignoring errors.

2. Configure the backend to taste.

Fine if the backend is already connected to this frontend, and we're
trying to connect it again only because our code is too disorganized to
know.

However, if the backend is actually connected to something else, it's
bound to end in tears.