[Qemu-devel] [PATCH for-2.11] hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false

[Qemu-devel] [PATCH for-2.11] hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false

[Thomas Huth]

QEMU currently aborts with an assertion message when the user is trying
to remove a dscm1xxxx again:

$ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
QEMU 2.9.93 monitor - type 'help' for more information
(qemu) device_add dscm1xxxx,id=xyz
(qemu) device_del xyz
**
ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
Aborted (core dumped)

Looks like this device has to be wired up in code and is not meant
to be hot-pluggable, so let's mark it with user_creatable = false.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ide/microdrive.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index e3fd30e..17917c0 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -575,12 +575,15 @@ PCMCIACardState *dscm1xxxx_init(DriveInfo *dinfo)
 static void dscm1xxxx_class_init(ObjectClass *oc, void *data)
 {
     PCMCIACardClass *pcc = PCMCIA_CARD_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
 
     pcc->cis = dscm1xxxx_cis;
     pcc->cis_len = sizeof(dscm1xxxx_cis);
 
     pcc->attach = dscm1xxxx_attach;
     pcc->detach = dscm1xxxx_detach;
+    /* Reason: Needs to be wired-up in code, see dscm1xxxx_init() */
+    dc->user_creatable = false;
 }
 
 static const TypeInfo dscm1xxxx_type_info = {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for-2.11] hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false

[Paolo Bonzini]

On 24/08/2017 05:03, Thomas Huth wrote:
> QEMU currently aborts with an assertion message when the user is trying
> to remove a dscm1xxxx again:
> 
> $ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
> QEMU 2.9.93 monitor - type 'help' for more information
> (qemu) device_add dscm1xxxx,id=xyz
> (qemu) device_del xyz
> **
> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
> 
> Looks like this device has to be wired up in code and is not meant
> to be hot-pluggable, so let's mark it with user_creatable = false.

The hotpluggable flag should be just a hint from the device, independent
of any knowledge of the board's behavior.  So a better question is why
qbus_is_hotpluggable was true at device_add time (I suppose
qdev_get_hotplug_handler was NULL at device_add time).

BTW, maybe we can get rid of qbus_is_hotpluggable by doing something
like this:

1) This code in device_set_realized:

        hotplug_ctrl = qdev_get_hotplug_handler(dev);
        if (hotplug_ctrl) {
            hotplug_handler_pre_plug(hotplug_ctrl, dev, &local_err);
            if (local_err != NULL) {
                goto fail;
            }
        }

can fail with an error (QERR_DEVICE_NO_HOTPLUG is already there) if
dev->hotplugged && !hotplug_ctrl.

2) In device_add, this code is now superfluous:

    if (qdev_hotplug && bus && !qbus_is_hotpluggable(bus)) {
        error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
        return NULL;
    }

3) in device_get_hotpluggable, the right side of the && can be replaced
with "qdev_get_hotplug_handler(dev) != NULL", getting rid of
qbus_is_hotpluggable.

Paolo

Re: [Qemu-devel] [PATCH for-2.11] hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false

[John Snow]

On 08/23/2017 11:03 PM, Thomas Huth wrote:
> QEMU currently aborts with an assertion message when the user is trying
> to remove a dscm1xxxx again:
> 
> $ aarch64-softmmu/qemu-system-aarch64 -S -M integratorcp -nographic
> QEMU 2.9.93 monitor - type 'help' for more information
> (qemu) device_add dscm1xxxx,id=xyz
> (qemu) device_del xyz
> **
> ERROR:qemu/qdev-monitor.c:872:qdev_unplug: assertion failed: (hotplug_ctrl)
> Aborted (core dumped)
> 
> Looks like this device has to be wired up in code and is not meant
> to be hot-pluggable, so let's mark it with user_creatable = false.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ide/microdrive.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
> index e3fd30e..17917c0 100644
> --- a/hw/ide/microdrive.c
> +++ b/hw/ide/microdrive.c
> @@ -575,12 +575,15 @@ PCMCIACardState *dscm1xxxx_init(DriveInfo *dinfo)
>  static void dscm1xxxx_class_init(ObjectClass *oc, void *data)
>  {
>      PCMCIACardClass *pcc = PCMCIA_CARD_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      pcc->cis = dscm1xxxx_cis;
>      pcc->cis_len = sizeof(dscm1xxxx_cis);
>  
>      pcc->attach = dscm1xxxx_attach;
>      pcc->detach = dscm1xxxx_detach;
> +    /* Reason: Needs to be wired-up in code, see dscm1xxxx_init() */
> +    dc->user_creatable = false;
>  }
>  
>  static const TypeInfo dscm1xxxx_type_info = {
> 

OK; I'll look into Paolo's suggestion as a further fix, but whether or
not we treat this property as a hint, it should be set anyway as you say.

Reviewed-by: John Snow <jsnow@redhat.com>

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js