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

[Philippe Mathieu-Daudé]

On 6/9/20 8:39 AM, Markus Armbruster wrote:
> 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 ;)

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