On Fri, 29 Jun 2018 17:34:24 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Fri, Jun 29, 2018 at 05:16:52PM -0300, Eduardo Habkost wrote:
> > On Fri, Jun 29, 2018 at 05:18:21PM +0200, Igor Mammedov wrote:
> > > On Fri, 29 Jun 2018 12:14:05 +0100
> > > Daniel P. Berrangé <berra...@redhat.com> wrote:
> > >
> > > > On Fri, Jun 29, 2018 at 01:08:38PM +0200, Paolo Bonzini wrote:
> > > > > On 29/06/2018 13:07, Greg Kurz wrote:
> > > > > >>>> Also asserting current_machine != NULL is not necessary, since
> > > > > >>>> you're
> > > > > >>>> immediately dereferencing it.
> > > > > >>> Is there a practical way to simply initialize the accelerators
> > > > > >>> earlier
> > > > > >>> in startup sequence, so we just remove or at least reduce, the
> > > > > >>> liklihood
> > > > > >>> of accessing it too early ?
> > > > > >> We can try, though not for 3.0 of course.
> > > > > >>
> > > > > > FWIW, the motivation for this patch was kvm_enabled() being called
> > > > > > under
> > > > > > the class_init function of the machine TypeInfo. This happens way
> > > > > > earlier
> > > > > > than accelerator init. Not sure this is doable, but I can have a
> > > > > > look.
> > > > > >
> > > > >
> > > > > Probably not, that's way too early indeed.
> > > >
> > > > Yeah, doing anything non-trivial in class_init is just asking for
> > > > trouble,
> > > > as conceivably nothing is initialized at that point.
> > > isn't class_init called lazily? (so it might actually work as far as type
> > > isn't touched before kvm is initialized)
> >
> > You have a good point: this means class_init bugs won't always
> > trigger the assert because of lazy class_init. It would be a
> > good idea to add a functional test that calls qom-list-types
> > using --preconfig to try to trigger them.
>
> Heh, I just noticed that the first thing we do immediately after
> parsing command-line options is calling:
>
> select_machine()
> find_default_machine()
> object_class_get_list()
> object_class_foreach()
> g_hash_table_foreach()
> object_class_foreach_tramp()
> type_initialize()
>
> ...which will call class_init for every single QOM type in QEMU.
>
Yes indeed. IIUC, this would trigger the assert if any QOM type
calls a ${acc}_enabled() from its class_init then.
BTW, I've just realized it triggers on x86:
#0 0x00007fffef7f0660 in raise () at /lib64/libc.so.6
#1 0x00007fffef7f1c41 in abort () at /lib64/libc.so.6
#2 0x00007fffef7e8f7a in __assert_fail_base () at /lib64/libc.so.6
#3 0x00007fffef7e8ff2 in () at /lib64/libc.so.6
#4 0x0000555555860cdd in assert_accelerator_initialized (allowed=false) at
/home/greg/Work/qemu/qemu-master/accel/accel.c:56
#5 0x000055555593c4db in host_x86_cpu_class_init (oc=0x5555566864c0, data=0x0)
at /home/greg/Work/qemu/qemu-master/target/i386/cpu.c:2841
#6 0x0000555555c93ff3 in type_initialize (ti=0x5555565ef3c0) at
/home/greg/Work/qemu/qemu-master/qom/object.c:342
#7 0x0000555555c95143 in object_class_foreach_tramp (key=0x5555565ca630,
value=0x5555565ef3c0, opaque=0x7fffffffd9b0) at
/home/greg/Work/qemu/qemu-master/qom/object.c:813
#8 0x00007ffff76afec0 in g_hash_table_foreach () at /lib64/libglib-2.0.so.0
#9 0x0000555555c95222 in object_class_foreach (fn=0x555555c95373
<object_class_get_list_tramp>, implements_type=0x555555e31996 "machine",
include_abstract=false, opaque=0x7fffffffda00) at
/home/greg/Work/qemu/qemu-master/qom/object.c:835
#10 0x0000555555c953f1 in object_class_get_list (implements_type=0x555555e31996
"machine", include_abstract=false) at
/home/greg/Work/qemu/qemu-master/qom/object.c:889
#11 0x00005555559c1608 in find_default_machine () at
/home/greg/Work/qemu/qemu-master/vl.c:1416
#12 0x00005555559c5517 in select_machine () at
/home/greg/Work/qemu/qemu-master/vl.c:2668
#13 0x00005555559c846d in main (argc=1, argv=0x7fffffffdd88,
envp=0x7fffffffdd98) at /home/greg/Work/qemu/qemu-master/vl.c:3987
static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
{
X86CPUClass *xcc = X86_CPU_CLASS(oc);
xcc->host_cpuid_required = true;
xcc->ordering = 8;
if (kvm_enabled()) {
xcc->model_description =
"KVM processor with all supported host features ";
} else if (hvf_enabled()) {
xcc->model_description =
"HVF processor with all supported host features ";
}
}
And, indeed, since commit d6dcc5583e7 (QEMU 2.11), -cpu ? shows the base
class description:
x86 host Enables all features supported by the accelerator in the
current host
instead of the expected:
x86 host KVM processor with all supported host features
This is a good illustration on how a bug can go unnoticed, but would be
caught with this patch. So I'll polish v3 and post it ASAP.
Cheers,
--
Greg
