Re: [Qemu-devel] [PATCH v2] accel: forbid early use of kvm_enabled() and friends

[Greg Kurz]

On Fri, 29 Jun 2018 17:34:24 -0300
Eduardo Habkost <address@hidden> 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é <address@hidden> 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