| Message ID | 1358314380-9400-4-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Wed, 16 Jan 2013 06:32:48 +0100 Andreas Färber <afaerber@suse.de> wrote: > Adapt the signature of x86_cpu_realize(), hook up to > DeviceClass::realize and set realized = true in cpu_x86_init(). > > Signed-off-by: Andreas Färber <afaerber@suse.de> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Igor Mammedov <imammedo@redhat.com> Reviewed-By: Igor Mammedov <imammedo@redhat.com> > --- > target-i386/cpu-qom.h | 3 --- > target-i386/cpu.c | 7 +++++-- > target-i386/helper.c | 2 +- > 3 Dateien geändert, 6 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-) > > diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h > index 332916a..3478dc9 100644 > --- a/target-i386/cpu-qom.h > +++ b/target-i386/cpu-qom.h > @@ -72,8 +72,5 @@ static inline X86CPU *x86_env_get_cpu(CPUX86State *env) > > #define ENV_GET_CPU(e) CPU(x86_env_get_cpu(e)) > > -/* TODO Drop once ObjectClass::realize is available */ > -void x86_cpu_realize(Object *obj, Error **errp); > - > > #endif > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 333745b..640dcdb 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2140,9 +2140,9 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error > **errp) } > #endif > > -void x86_cpu_realize(Object *obj, Error **errp) > +static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > { > - X86CPU *cpu = X86_CPU(obj); > + X86CPU *cpu = X86_CPU(dev); > CPUX86State *env = &cpu->env; > > if (env->cpuid_7_0_ebx_features && env->cpuid_level < 7) { > @@ -2247,6 +2247,9 @@ static void x86_cpu_common_class_init(ObjectClass > *oc, void *data) { > X86CPUClass *xcc = X86_CPU_CLASS(oc); > CPUClass *cc = CPU_CLASS(oc); > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + dc->realize = x86_cpu_realizefn; > > xcc->parent_reset = cc->reset; > cc->reset = x86_cpu_reset; > diff --git a/target-i386/helper.c b/target-i386/helper.c > index 547c25e..bf43d6a 100644 > --- a/target-i386/helper.c > +++ b/target-i386/helper.c > @@ -1280,7 +1280,7 @@ X86CPU *cpu_x86_init(const char *cpu_model) > return NULL; > } > > - x86_cpu_realize(OBJECT(cpu), &error); > + object_property_set_bool(OBJECT(cpu), true, "realized", &error); > if (error) { > error_free(error); > object_delete(OBJECT(cpu));
On Wed, Jan 16, 2013 at 06:32:48AM +0100, Andreas Färber wrote: [...] > @@ -2247,6 +2247,9 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > { > X86CPUClass *xcc = X86_CPU_CLASS(oc); > CPUClass *cc = CPU_CLASS(oc); > + DeviceClass *dc = DEVICE_CLASS(oc); > + > + dc->realize = x86_cpu_realizefn; The DeviceClass documenation says: "Any type may override the @realize and/or @unrealize callbacks but needs to call (and thus save) the parent type's implementation if so desired." Why are you not following it?
Am 16.01.2013 17:04, schrieb Eduardo Habkost: > On Wed, Jan 16, 2013 at 06:32:48AM +0100, Andreas Färber wrote: > [...] >> @@ -2247,6 +2247,9 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) >> { >> X86CPUClass *xcc = X86_CPU_CLASS(oc); >> CPUClass *cc = CPU_CLASS(oc); >> + DeviceClass *dc = DEVICE_CLASS(oc); >> + >> + dc->realize = x86_cpu_realizefn; > > The DeviceClass documenation says: > > "Any type may override the @realize and/or @unrealize callbacks but > needs to call (and thus save) the parent type's implementation if so > desired." > > Why are you not following it? "if so desired" - I didn't desire or need to call code that calls an initfn that no longer exists after this patch. Same as the ISADevice conversion series did not unnecessarily call the DeviceClass-level backwards-compatibility realizefn: to save time-consuming ...Class::parent_realizefn field additions and to not in the end call code that doesn't NULL-check ...DeviceClass::init. That's qdev's old "leaf type" concept mentioned in the same documentation. I mentioned in the cover letter that this needs to be changed once a CPUClass-level realizefn is introduced. I could introduce a no-op realizefn there and do the regular store+call. Note that wherever we set realized = true on CPUs, we need to test and re-review for further unchecked uses of parent_bus. It has been pushed to qom-cpu-realize branch for anyone that wants to play with the latest version. Andreas
On Wed, Jan 16, 2013 at 11:52:47PM +0100, Andreas Färber wrote: > Am 16.01.2013 17:04, schrieb Eduardo Habkost: > > On Wed, Jan 16, 2013 at 06:32:48AM +0100, Andreas Färber wrote: > > [...] > >> @@ -2247,6 +2247,9 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > >> { > >> X86CPUClass *xcc = X86_CPU_CLASS(oc); > >> CPUClass *cc = CPU_CLASS(oc); > >> + DeviceClass *dc = DEVICE_CLASS(oc); > >> + > >> + dc->realize = x86_cpu_realizefn; > > > > The DeviceClass documenation says: > > > > "Any type may override the @realize and/or @unrealize callbacks but > > needs to call (and thus save) the parent type's implementation if so > > desired." > > > > Why are you not following it? > > "if so desired" - I didn't desire or need to call code that calls an > initfn that no longer exists after this patch. Same as the ISADevice > conversion series did not unnecessarily call the DeviceClass-level > backwards-compatibility realizefn: to save time-consuming > ...Class::parent_realizefn field additions and to not in the end call > code that doesn't NULL-check ...DeviceClass::init. That's qdev's old > "leaf type" concept mentioned in the same documentation. I had read "if so desired" as "if it's desired to override the realize callback", not as "if it's desired to call the parent realize function". I believed every class could assume that subclasses would never override realize() without calling the parent class' realize function (so we could add stuff to DeviceClass.realize and CPUClass.realize in the future and be sure that the code would be always called). But from the documentation mentioning "new leaf types should consult their respective parent type", it looks like this decision would be taken/documented in each base class. If that's the case, then OK. > > I mentioned in the cover letter that this needs to be changed once a > CPUClass-level realizefn is introduced. I could introduce a no-op > realizefn there and do the regular store+call. That was the semantics I was expecting: base classes would safely introduce realize functions without worrying if subclasses would override it incorrectly and break it. Anyway, saving the parent function in every subclass is so cumbersome that simply documenting it as "CPUClass subclasses must call qemu_init_vcpu()" sounds easier than "CPUClass subclasses must save the parent's realize() and call it". So: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > Note that wherever we set realized = true on CPUs, we need to test and > re-review for further unchecked uses of parent_bus. It has been pushed > to qom-cpu-realize branch for anyone that wants to play with the latest > version. > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Am 17.01.2013 00:43, schrieb Eduardo Habkost: > On Wed, Jan 16, 2013 at 11:52:47PM +0100, Andreas Färber wrote: >> Am 16.01.2013 17:04, schrieb Eduardo Habkost: >>> On Wed, Jan 16, 2013 at 06:32:48AM +0100, Andreas Färber wrote: >>> [...] >>>> @@ -2247,6 +2247,9 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) >>>> { >>>> X86CPUClass *xcc = X86_CPU_CLASS(oc); >>>> CPUClass *cc = CPU_CLASS(oc); >>>> + DeviceClass *dc = DEVICE_CLASS(oc); >>>> + >>>> + dc->realize = x86_cpu_realizefn; >>> >>> The DeviceClass documenation says: >>> >>> "Any type may override the @realize and/or @unrealize callbacks but >>> needs to call (and thus save) the parent type's implementation if so >>> desired." >>> >>> Why are you not following it? >> >> "if so desired" - I didn't desire or need to call code that calls an >> initfn that no longer exists after this patch. Same as the ISADevice >> conversion series did not unnecessarily call the DeviceClass-level >> backwards-compatibility realizefn: to save time-consuming >> ...Class::parent_realizefn field additions and to not in the end call >> code that doesn't NULL-check ...DeviceClass::init. That's qdev's old >> "leaf type" concept mentioned in the same documentation. > > I had read "if so desired" as "if it's desired to override the realize > callback", not as "if it's desired to call the parent realize function". Sorry, and I thought my documentation was too verbose already. ;) > I believed every class could assume that subclasses would never override > realize() without calling the parent class' realize function (so we > could add stuff to DeviceClass.realize and CPUClass.realize in the > future and be sure that the code would be always called). > > But from the documentation mentioning "new leaf types should consult > their respective parent type", it looks like this decision would be > taken/documented in each base class. If that's the case, then OK. I've sent out a patch improving QOM and DeviceClass documentation. :) >> I mentioned in the cover letter that this needs to be changed once a >> CPUClass-level realizefn is introduced. I could introduce a no-op >> realizefn there and do the regular store+call. > > That was the semantics I was expecting: base classes would safely > introduce realize functions without worrying if subclasses would > override it incorrectly and break it. We could do that if we fix up the respective DeviceClass::init, SysBusDeviceClass::init etc. code. Question is (just as with some x86 CPU code) whether it's worth cleaning up when we know that it is to be refactored later. > Anyway, saving the parent function in every subclass is so cumbersome > that simply documenting it as "CPUClass subclasses must call > qemu_init_vcpu()" sounds easier than "CPUClass subclasses must save the > parent's realize() and call it". [snip] Actually that particular piece of code is unrelated to this discussion since qemu_init_vcpu() still operates on CPUArchState and thus cannot be moved into CPUClass yet. The reason is that cpus.c:qemu_kvm_cpu_thread_fn sets cpu_single_env, and I do not see a solution for that - suggestions or patches welcome. However, I see that kvm-all.c:kvm_on_sigbus_vcpu() can be switched to CPUState now, so that cpus.c:qemu_kvm_eat_signals() can be changed to CPUState, used from cpus.c:qemu_kvm_wait_io_event(). But cpus.c:cpu_thread_is_idle() still uses env->halted, which is blocked by the search for an acceptable solution to flush the TLB at CPUState level (exec.c:cpu_common_post_load()). Andreas
On Thu, Jan 17, 2013 at 09:03:59AM +0100, Andreas Färber wrote: [...] > >> I mentioned in the cover letter that this needs to be changed once a > >> CPUClass-level realizefn is introduced. I could introduce a no-op > >> realizefn there and do the regular store+call. > > > > That was the semantics I was expecting: base classes would safely > > introduce realize functions without worrying if subclasses would > > override it incorrectly and break it. > > We could do that if we fix up the respective DeviceClass::init, > SysBusDeviceClass::init etc. code. Question is (just as with some x86 > CPU code) whether it's worth cleaning up when we know that it is to be > refactored later. Actually I am not sure it would be nice to require every single class to save/call the parent realize function. I am starting to like the more relaxed requirement. :-) > > > Anyway, saving the parent function in every subclass is so cumbersome > > that simply documenting it as "CPUClass subclasses must call > > qemu_init_vcpu()" sounds easier than "CPUClass subclasses must save the > > parent's realize() and call it". > [snip] > > Actually that particular piece of code is unrelated to this discussion > since qemu_init_vcpu() still operates on CPUArchState and thus cannot be > moved into CPUClass yet. The reason is that > cpus.c:qemu_kvm_cpu_thread_fn sets cpu_single_env, and I do not see a > solution for that - suggestions or patches welcome. I used qemu_init_vcpu() as an example because it's something called by the realize function for all targets, and one day could be called by a common CPUClass realize function. I didn't check if it was possible to convert it today, already. My point is: if you need to save the pointer and call the parent realize function only if documented and required by the parent class, the parent could as well simply document it as "subclasses of TYPE_FOO should manually call foo_realize() if they override the realize function" instead of "subclasses of TYPE_FOO should save and call the parent realize function if they override de realize function". Won't it be easier and simpler? > > However, I see that kvm-all.c:kvm_on_sigbus_vcpu() can be switched to > CPUState now, so that cpus.c:qemu_kvm_eat_signals() can be changed to > CPUState, used from cpus.c:qemu_kvm_wait_io_event(). > But cpus.c:cpu_thread_is_idle() still uses env->halted, which is blocked > by the search for an acceptable solution to flush the TLB at CPUState > level (exec.c:cpu_common_post_load()). > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h index 332916a..3478dc9 100644 --- a/target-i386/cpu-qom.h +++ b/target-i386/cpu-qom.h @@ -72,8 +72,5 @@ static inline X86CPU *x86_env_get_cpu(CPUX86State *env) #define ENV_GET_CPU(e) CPU(x86_env_get_cpu(e)) -/* TODO Drop once ObjectClass::realize is available */ -void x86_cpu_realize(Object *obj, Error **errp); - #endif diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 333745b..640dcdb 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2140,9 +2140,9 @@ static void x86_cpu_apic_init(X86CPU *cpu, Error **errp) } #endif -void x86_cpu_realize(Object *obj, Error **errp) +static void x86_cpu_realizefn(DeviceState *dev, Error **errp) { - X86CPU *cpu = X86_CPU(obj); + X86CPU *cpu = X86_CPU(dev); CPUX86State *env = &cpu->env; if (env->cpuid_7_0_ebx_features && env->cpuid_level < 7) { @@ -2247,6 +2247,9 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) { X86CPUClass *xcc = X86_CPU_CLASS(oc); CPUClass *cc = CPU_CLASS(oc); + DeviceClass *dc = DEVICE_CLASS(oc); + + dc->realize = x86_cpu_realizefn; xcc->parent_reset = cc->reset; cc->reset = x86_cpu_reset; diff --git a/target-i386/helper.c b/target-i386/helper.c index 547c25e..bf43d6a 100644 --- a/target-i386/helper.c +++ b/target-i386/helper.c @@ -1280,7 +1280,7 @@ X86CPU *cpu_x86_init(const char *cpu_model) return NULL; } - x86_cpu_realize(OBJECT(cpu), &error); + object_property_set_bool(OBJECT(cpu), true, "realized", &error); if (error) { error_free(error); object_delete(OBJECT(cpu));
Adapt the signature of x86_cpu_realize(), hook up to DeviceClass::realize and set realized = true in cpu_x86_init(). Signed-off-by: Andreas Färber <afaerber@suse.de> Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu-qom.h | 3 --- target-i386/cpu.c | 7 +++++-- target-i386/helper.c | 2 +- 3 Dateien geändert, 6 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)