On 6/9/20 8:39 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <phi...@redhat.com> writes:
>
>> On 5/28/20 1:04 PM, Markus Armbruster wrote:
>>> Commit 260bc9d8aa "hw/sd/sd.c: QOMify" QOMified only the device
>>> itself, not its users. It kept sd_init() around for non-QOMified
>>> users.
>>>
>>> More than four years later, three such users remain: omap1 (machines
>>> cheetah, sx1, sx1-v1) and omap2 (machines n800, n810) are not
>>> QOMified, and pl181 (machines integratorcp, realview-eb,
>>> realview-eb-mpcore, realview-pb-a8 realview-pbx-a9, versatileab,
>>> versatilepb, vexpress-a15, vexpress-a9) is not QOMified properly.
>>>
>>> The issue I presently have with this: an "sd-card" device should plug
>>> into an "sd-bus" (its DeviceClass member bus_type says so), but
>>> sd_init() leaves it unplugged. This is normally a bug (I just fixed
>>> some instances), and I'd like to assert proper pluggedness to prevent
>>> regressions. However, the qdev-but-not-quite thing returned by
>>> sd_init() would fail the assertion. Meh.
>>>
>>> Make sd_init() hide it from QOM/qdev. Visible in "info qom-tree",
>>> here's the change for cheetah:
>>>
>>> /machine (cheetah-machine)
>>> [...]
>>> /unattached (container)
>>> [...]
>>> /device[5] (serial-mm)
>>> /serial (serial)
>>> /serial[0] (qemu:memory-region)
>>> - /device[6] (sd-card)
>>> - /device[7] (omap-gpio)
>>> + /device[6] (omap-gpio)
>>> [rest of device[*] renumbered...]
>>>
>>> Cc: "Philippe Mathieu-Daudé" <phi...@redhat.com>
>>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>>> ---
>>> hw/sd/sd.c | 40 ++++++++++++++++++++++++++++------------
>>> 1 file changed, 28 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 3c06a0ac6d..7070a116ea 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -83,6 +83,10 @@ enum SDCardStates {
>>> struct SDState {
>>> DeviceState parent_obj;
>>>
>>> + /* If true, created by sd_init() for a non-qdevified caller */
>>> + /* TODO purge them with fire */
>>> + bool me_no_qdev_me_kill_mammoth_with_rocks;
>>
>> Your next patch does not use me_no_qdev_me_kill_mammoth_with_rocks in
>> qdev_assert_realized_properly().
>
> It doesn't have to, because this qdev-but-not-quite thing isn't visible
> there.
>
>> Suggestion for less ugly hack:
>>
>> static int qdev_assert_realized_properly(Object *obj, void *opaque)
>> {
>> DeviceState *dev = DEVICE(object_dynamic_cast(obj, TYPE_DEVICE));
>> DeviceClass *dc;
>>
>> if (dev) {
>> if (object_dynamic_cast(OBJECT(obj), TYPE_SD_CARD)) {
>> /* bla bla bla */
>> return 17;
>> }
>> dc = DEVICE_GET_CLASS(dev);
>> assert(dev->realized);
>> assert(dev->parent_bus || !dc->bus_type);
>> }
>> return 0;
>> }
>
> Now qdev_assert_realized_properly() knows about the caveman. I don't
> like that.
>
> My hack keeps the knowledge strictly local, and protects all users of
> QOM from getting exposed to the caveman, not just the "realized
> properly" assertion. My hack is locally ugly, but I consider that a
> feature ;)
Understood.
>
> My patch could be made smaller: @me_no_qdev_me_kill_mammoth_with_rocks
> exists only to make the parts supporting the caveman more immediately
> obvious.
>
>>
>>> +
>>> /* SD Memory Card Registers */
>>> uint32_t ocr;
>>> uint8_t scr[8];
>>> @@ -129,6 +133,8 @@ struct SDState {
>>> bool cmd_line;
>>> };
>>>
>>> +static void sd_realize(DeviceState *dev, Error **errp);
>>> +
>>> static const char *sd_state_name(enum SDCardStates state)
>>> {
>>> static const char *state_name[] = {
>>> @@ -590,7 +596,7 @@ static void sd_cardchange(void *opaque, bool load,
>>> Error **errp)
>>> {
>>> SDState *sd = opaque;
>>> DeviceState *dev = DEVICE(sd);
>>> - SDBus *sdbus = SD_BUS(qdev_get_parent_bus(dev));
>>> + SDBus *sdbus;
>>> bool inserted = sd_get_inserted(sd);
>>> bool readonly = sd_get_readonly(sd);
>>>
>>> @@ -601,19 +607,17 @@ static void sd_cardchange(void *opaque, bool load,
>>> Error **errp)
>>> trace_sdcard_ejected();
>>> }
>>>
>>> - /* The IRQ notification is for legacy non-QOM SD controller devices;
>>> - * QOMified controllers use the SDBus APIs.
>>> - */
>>> - if (sdbus) {
>>> - sdbus_set_inserted(sdbus, inserted);
>>> - if (inserted) {
>>> - sdbus_set_readonly(sdbus, readonly);
>>> - }
>>> - } else {
>>> + if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
>>> qemu_set_irq(sd->inserted_cb, inserted);
>>> if (inserted) {
>>> qemu_set_irq(sd->readonly_cb, readonly);
>>> }
>>> + } else {
>>> + sdbus = SD_BUS(qdev_get_parent_bus(dev));
>>> + sdbus_set_inserted(sdbus, inserted);
>>> + if (inserted) {
>>> + sdbus_set_readonly(sdbus, readonly);
>>> + }
>>> }
>>> }
>>>
>>> @@ -697,6 +701,7 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
>>> {
>>> Object *obj;
>>> DeviceState *dev;
>>> + SDState *sd;
>>> Error *err = NULL;
>>>
>>> obj = object_new(TYPE_SD_CARD);
>>> @@ -707,13 +712,24 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
>>> return NULL;
>>> }
>>> qdev_prop_set_bit(dev, "spi", is_spi);
>>> - object_property_set_bool(obj, true, "realized", &err);
>>> +
>>> + /*
>>> + * Realizing the device properly would put it into the QOM
>>> + * composition tree even though it is not plugged into an
>>> + * appropriate bus. That's a no-no. Hide the device from
>>> + * QOM/qdev, and call its qdev realize callback directly.
>>> + */
>>> + object_ref(obj);
>>> + object_unparent(obj);
>>> + sd_realize(dev, &err);
>>> if (err) {
>>> error_reportf_err(err, "sd_init failed: ");
>>> return NULL;
>>> }
>>>
>>> - return SD_CARD(dev);
>>> + sd = SD_CARD(dev);
>>> + sd->me_no_qdev_me_kill_mammoth_with_rocks = true;
>>> + return sd;
>>> }
>>>
>>> void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
>>>
>
