Re: [Qemu-devel] [PATCH] target-i386: initialize APIC at CPU level

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 04 Oct 2012 18:15:48 +0200
Andreas Färber <afaerber@suse.de> wrote:
> > +    env->apic_state = qdev_create(NULL, apic_type);
> > +
> > +    if (env->apic_state == NULL) {
> > +        error_set(errp, QERR_DEVICE_INIT_FAILED, apic_type);
> > +        return;
> > +    }
> > +
> > +    object_property_add_child(OBJECT(cpu), "apic",
> > +                              OBJECT(env->apic_state), NULL);
> > +    qdev_prop_set_uint8(env->apic_state, "id", env->cpuid_apic_id);
> > +    /* TODO: convert to link<> */
> > +    qdev_prop_set_ptr(env->apic_state, "cpu_env", env);
> 
> Do we still need env->apic_state and a property on the APIC at all? From
> the X86CPU side we can access the "apic" property to retrieve the
> pointer,
It would introduce too much overhead. For test I've just substituted all uses
of env->apic_state with a func that could serve as get_child(name)

+DeviceState *get_apic(CPUX86State *env) {
+    X86CPU *cpu = x86_env_get_cpu(env);
+    Object *o = object_resolve_path_component(OBJECT(cpu), (gchar *)"apic");
+    DeviceState *dev = OBJECT_CHECK(DeviceState, o, "device");
+    return dev; 
+}

and run qemu under valgrind --tool=callgrind, booting openbsd for 5 min.

         hot path      get_apic() call count   get_apic() incl cost. 
TCG: pic_irq_request()         7M               44  (8th from top 10)
KVM: kvm_arch_post_run()       2M               38  (6th from top 10)
NO PATCH: pic_irq_request()   18M               0.8

pic_irq_request() calls get_apic() 3x, kvm_arch_post_run() - 2x times

I suppose calling get_apic() only once on hot path could reduce amount of
calls but property searching and dynamic cast won't disappear any way.
Looks like it's better to leave apic_state as it is.

> and from the APIC we should be able to navigate to its parent,
> right?
while adding object_get_parent(), I've noticed in object.h
"
 * There is no way for a child to determine what its parent is.  It is not
 * a bidirectional relationship.  This is by design.
"
Would be it acceptable to allow child access to parent?

...
> 
> Andreas
> 

Thanks,
   Igor