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

[Peter Maydell]

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.

thanks
-- PMM