Re: [Qemu-devel] [PATCH] qdev: Make -device FOO, help help again when FOO is not pluggable

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Mar 16, 2015 at 06:33:52PM +0100, Markus Armbruster wrote:
> Doesn't work since commit 31bed55 changed qdev_device_help() to reject
> abstract devices and devices that have
> cannot_instantiate_with_device_add_yet set.
> 
> The former makes sense: abstract devices are purely internal, and the
> implementation of the help feature can't cope with them.
> 
> The latter makes less sense: the implementation works fine, and even
> though you can't -device such a device, the help may still be useful
> elsewhere, for instance with -global.
> 
> Revert the latter by moving the cannot_instantiate_with_device_add_yet
> check back to the other caller of qdev_get_device_class(),
> qdev_device_add().

This reintroduces the following crash:

  $ ./x86_64-softmmu/qemu-system-x86_64    -device host-x86_64-cpu,help
  qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:1391: host_x86_cpu_initfn: Assertion `(kvm_allowed)' failed.
  Aborted (core dumped)

And this (which is not x86-specific because other arches also call
cpu_exec_init() inside instance_init):

  $ ./x86_64-softmmu/qemu-system-x86_64    -monitor stdio
  QEMU 2.2.50 monitor - type 'help' for more information
  (qemu) device_add Nehalem-x86_64-cpu,help
  Nehalem-x86_64-cpu.filtered-features=X86CPUFeatureWordInfo
  Nehalem-x86_64-cpu.feature-words=X86CPUFeatureWordInfo
  Nehalem-x86_64-cpu.apic-id=int
  Nehalem-x86_64-cpu.tsc-frequency=int
  Nehalem-x86_64-cpu.model-id=string
  Nehalem-x86_64-cpu.vendor=string
  Nehalem-x86_64-cpu.xlevel=int
  Nehalem-x86_64-cpu.level=int
  Nehalem-x86_64-cpu.stepping=int
  Nehalem-x86_64-cpu.model=int
  Nehalem-x86_64-cpu.family=int
  Nehalem-x86_64-cpu.kvm=bool
  Nehalem-x86_64-cpu.enforce=bool
  Nehalem-x86_64-cpu.check=bool
  Nehalem-x86_64-cpu.hv-time=bool
  Nehalem-x86_64-cpu.hv-vapic=bool
  Nehalem-x86_64-cpu.hv-relaxed=bool
  Nehalem-x86_64-cpu.hv-spinlocks=int
  Nehalem-x86_64-cpu.pmu=bool
  (qemu) Segmentation fault (core dumped)

Should we:

1) Live with the crashes until we move all code with side-effects outside
   instance_init (including bot not limited to cpu_exec_init() calls on most
   CPU classes);
2) add a "instance_init_is_unsafe" flag to those classes classes; or
3) Keep the current code until we fix the classes that have unsafe
   instance_init functions?

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qdev-monitor.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 5d30ac5..b0b8cf1 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -180,10 +180,14 @@ static const char *find_typename_by_alias(const char *alias)
>      return NULL;
>  }
>  
> +
> +/**
> + * Return DeviceClass for concrete device type @driver.
> + * On error, store an error through @errp if non-null, and return %NULL.
> + */
>  static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>  {
>      ObjectClass *oc;
> -    DeviceClass *dc;
>  
>      oc = object_class_by_name(*driver);
>      if (!oc) {
> @@ -206,15 +210,7 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>          return NULL;
>      }
>  
> -    dc = DEVICE_CLASS(oc);
> -    if (dc->cannot_instantiate_with_device_add_yet ||
> -        (qdev_hotplug && !dc->hotpluggable)) {
> -        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "driver",
> -                  "pluggable device type");
> -        return NULL;
> -    }
> -
> -    return dc;
> +    return DEVICE_CLASS(oc);
>  }
>  
>  
> @@ -512,6 +508,12 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>          error_free(err);
>          return NULL;
>      }
> +    if (dc->cannot_instantiate_with_device_add_yet ||
> +        (qdev_hotplug && !dc->hotpluggable)) {
> +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "driver",
> +                      "pluggable device type");
> +        return NULL;
> +    }
>  
>      /* find bus */
>      path = qemu_opt_get(opts, "bus");
> -- 
> 1.9.3
> 

-- 
Eduardo