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

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

[Markus Armbruster]

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

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

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

[Paolo Bonzini]

On 16/03/2015 18:33, 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().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Cc: qemu-stable@nongnu.org

I'm picking this patch up.  It will miss -rc0, but I'll send my next
pull request before -rc1.

Paolo

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

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

[Eduardo Habkost]

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

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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

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

I foolishly assumed that I can object_new() any device, so testing one
with cannot_instantiate_with_device_add_yet was enough.  Thanks for
paying attention.

We clearly need a test that object_new()s every known type.  Could this
be done with -object?  Probably not if we want to catch known bad side
effects.  Ideas?

> 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

To honor Anthony's long and distinguished service, we should name it
cannot_even_create_with_object_new_yet ;-P

If my working idea "you can object_new() any type, and in fact you have
to for introspection" is correct, then this as a stop gap, not a
solution.  If it's not correct, please educate me.

> 3) Keep the current code until we fix the classes that have unsafe
>    instance_init functions?

The help regression is already in 2.2, which reduces the urgency of
fixing it a bit.

Once we know which types's instance_init misbehave, (2) is easy.  (1)
might be, can't say.

"Until we fix" is potentially unbounded time, which makes me wary of
both (1) and (3).  If we pick either of them now, and a fix doesn't
appear during the next development cycle, I'd like us to switch to (2)
as a stop gap for 2.4.

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

[Eduardo Habkost]

On Tue, Mar 17, 2015 at 08:15:53AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > 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)
> 
> I foolishly assumed that I can object_new() any device, so testing one
> with cannot_instantiate_with_device_add_yet was enough.  Thanks for
> paying attention.
> 
> We clearly need a test that object_new()s every known type.  Could this
> be done with -object?  Probably not if we want to catch known bad side
> effects.  Ideas?

The ones above are easy ones: one crashes QEMU immediately after
device_add, another one crashes QEMU immediately. But I would like to
find a way to catch the side effects we aren't even aware of. Does
anybody know some trick that would allow us to easily detect any memory
writes outside the object struct while running the instance_init
functions?

> 
> > 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
> 
> To honor Anthony's long and distinguished service, we should name it
> cannot_even_create_with_object_new_yet ;-P
> 
> If my working idea "you can object_new() any type, and in fact you have
> to for introspection" is correct, then this as a stop gap, not a
> solution.  If it's not correct, please educate me.

Yes, this would be a stop gap. The problem is that the issue seems to
affect most CPU classes on most architectures, so we may take a while to
fix all of them.

I have just ran a script which tried "device_add $DEV,help" on all
no-user classes on qemu-system-x86_64, and the only crashes I've seen
were on the CPU classes. I think it's likely that we will need to set
the flag only on TYPE_CPU.

> 
> > 3) Keep the current code until we fix the classes that have unsafe
> >    instance_init functions?
> 
> The help regression is already in 2.2, which reduces the urgency of
> fixing it a bit.
> 
> Once we know which types's instance_init misbehave, (2) is easy.  (1)
> might be, can't say.

Well, (1) is the easiest solution, because it means doing nothing and
live with the crashes until we fix them. But the "fixing the crashes"
part may take a while.

> 
> "Until we fix" is potentially unbounded time, which makes me wary of
> both (1) and (3).  If we pick either of them now, and a fix doesn't
> appear during the next development cycle, I'd like us to switch to (2)
> as a stop gap for 2.4.

Makes sense to me.

-- 
Eduardo