Re: [PATCH v2 23/24] sd: Hide the qdev-but-not-quite thing created by sd_init()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@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é" <philmd@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@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 ;)

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)
>>