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

[Markus Armbruster]

Eduardo Habkost <address@hidden> 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.