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

[Paolo Bonzini]

On 04/12/2015 12:00, Peter Maydell wrote:
> 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.

[copying from the other thread]

FWIW, I don't think the SD card will be qdevified because it doesn't
need a bus.  It's similar indeed to SerialState, which was supposed to
be the poster child of QOM embedding and never got QOMified.

A host controller controls exactly one SD card, the SSI bridge is also
for exactly one SD card, etc.  So there's not much to gain from
qdevification of the card itself.  There would be a gain from
qdevification of the OMAP and StrongARM controllers, which is exactly
what this series does.

Paolo