qdev: Make -device FOO, help help again when FOO is not pluggable

PATCH In review Markus Armbruster 3 participants 5 messages Mar 16, 2015 – Mar 17, 2015

Share & tools

Markus Armbruster

author

2015-03-16 17:34 UTC #0 lore raw

PATCH 1/1 qdev: Make -device FOO, help help again when FOO is not pluggable

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 <[email protected]>

---
 qdev-monitor.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

qdev-monitor.c +12 ~~-10~~

index 5d30ac5..b0b8cf1 100644
@@ -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");

Signature · 1 line

1.9.3

Paolo Bonzini

2015-03-16 17:37 UTC #1 lore raw

Quoted 11 lines · 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.

Quoted 5 lines

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 <[email protected]>

Cc [email protected]

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

Paolo

Quoted 55 lines

---
 qdev-monitor.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

qdev-monitor.c +12 ~~-10~~

index 5d30ac5..b0b8cf1 100644
@@ -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) 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?

Quoted 59 lines

Signed-off-by Markus Armbruster <[email protected]>

---
 qdev-monitor.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

qdev-monitor.c +12 ~~-10~~

index 5d30ac5..b0b8cf1 100644
@@ -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");

Signature · 1 line

1.9.3

Signature · 1 line

Eduardo

Markus Armbruster

author

2015-03-17 07:16 UTC #3 lore raw

Quoted 40 lines · Eduardo Habkost

Quoted 7 lines · 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.
Quoted 7 lines
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

Quoted 8 lines

  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?

Quoted 7 lines

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.

Quoted 2 lines

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.

Eduardo Habkost

2015-03-17 14:58 UTC #4 lore raw

Quoted 51 lines · Markus Armbruster

Quoted 40 lines · Eduardo Habkost
Quoted 7 lines · 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.
Quoted 7 lines
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
Quoted 8 lines
  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)

Quoted 7 lines

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?

Quoted 7 lines

Quoted 7 lines
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);

Quoted 8 lines

Quoted 1 line
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.

Quoted 9 lines

Quoted 3 lines
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.

Quoted 5 lines

"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.

Signature · 1 line

Eduardo