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

[Philippe Mathieu-Daudé]

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

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;
}

> +
>      /* 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)
>