| Message ID | 1425576411-32639-2-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: > Instead of passing icc_bridge from the PC initialization code to > cpu_x86_create(), make the PC initialization code attach the CPU to > icc_bridge. > > The only difference here is that icc_bridge attachment will now be done > after x86_cpu_parse_featurestr() is called. But this shouldn't make any > difference, as property setters shouldn't depend on icc_bridge. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Looks okay to me, Reviewed-by: Andreas Färber <afaerber@suse.de> But using this smaller patch will still make inlining pc_new_cpu(), where you are moving it to, bigger diffstat-wise (WIP). Regards, Andreas
On Tue, Mar 10, 2015 at 12:50:07PM +0100, Andreas Färber wrote: > Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: > > Instead of passing icc_bridge from the PC initialization code to > > cpu_x86_create(), make the PC initialization code attach the CPU to > > icc_bridge. > > > > The only difference here is that icc_bridge attachment will now be done > > after x86_cpu_parse_featurestr() is called. But this shouldn't make any > > difference, as property setters shouldn't depend on icc_bridge. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > Looks okay to me, > > Reviewed-by: Andreas Färber <afaerber@suse.de> > > But using this smaller patch will still make inlining pc_new_cpu(), > where you are moving it to, bigger diffstat-wise (WIP). I just see this it as a reason to not inline pc_new_cpu(). :)
Am 10.03.2015 um 13:42 schrieb Eduardo Habkost: > On Tue, Mar 10, 2015 at 12:50:07PM +0100, Andreas Färber wrote: >> Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: >>> Instead of passing icc_bridge from the PC initialization code to >>> cpu_x86_create(), make the PC initialization code attach the CPU to >>> icc_bridge. >>> >>> The only difference here is that icc_bridge attachment will now be done >>> after x86_cpu_parse_featurestr() is called. But this shouldn't make any >>> difference, as property setters shouldn't depend on icc_bridge. >>> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >> >> Looks okay to me, >> >> Reviewed-by: Andreas Färber <afaerber@suse.de> >> >> But using this smaller patch will still make inlining pc_new_cpu(), >> where you are moving it to, bigger diffstat-wise (WIP). > > I just see this it as a reason to not inline pc_new_cpu(). :) Did you actually read my code? cpu_x86_create() does not allow in-place initialization, in addition to doing the realized=true. Andreas
Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: > Instead of passing icc_bridge from the PC initialization code to > cpu_x86_create(), make the PC initialization code attach the CPU to > icc_bridge. > > The only difference here is that icc_bridge attachment will now be done > after x86_cpu_parse_featurestr() is called. But this shouldn't make any > difference, as property setters shouldn't depend on icc_bridge. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/i386/pc.c | 6 +++++- > target-i386/cpu.c | 14 ++------------ > target-i386/cpu.h | 3 +-- > 3 files changed, 8 insertions(+), 15 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index ed54d93..66b9fa6 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -995,12 +995,16 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > X86CPU *cpu; > Error *local_err = NULL; > > - cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err); > + cpu = cpu_x86_create(cpu_model, &local_err); > if (local_err != NULL) { > error_propagate(errp, local_err); > return NULL; > } > > + assert(icc_bridge); On second thoughts, why are you asserting here rather than setting errp? Just add an out: below and goto out, like I did. On startup it doesn't matter much, but for hot-add asserting would not be so nice. Regards, Andreas > + qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc")); > + object_unref(OBJECT(cpu)); > + > object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); > object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 50907d0..097924c 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -2076,8 +2076,7 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) > > } > > -X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge, > - Error **errp) > +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) > { > X86CPU *cpu = NULL; > X86CPUClass *xcc; > @@ -2108,15 +2107,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge, > > cpu = X86_CPU(object_new(object_class_get_name(oc))); > > -#ifndef CONFIG_USER_ONLY > - if (icc_bridge == NULL) { > - error_setg(&error, "Invalid icc-bridge value"); > - goto out; > - } > - qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc")); > - object_unref(OBJECT(cpu)); > -#endif > - > x86_cpu_parse_featurestr(CPU(cpu), features, &error); > if (error) { > goto out; [snip]
On Tue, Mar 10, 2015 at 01:51:26PM +0100, Andreas Färber wrote: > Am 10.03.2015 um 13:42 schrieb Eduardo Habkost: > > On Tue, Mar 10, 2015 at 12:50:07PM +0100, Andreas Färber wrote: > >> Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: > >>> Instead of passing icc_bridge from the PC initialization code to > >>> cpu_x86_create(), make the PC initialization code attach the CPU to > >>> icc_bridge. > >>> > >>> The only difference here is that icc_bridge attachment will now be done > >>> after x86_cpu_parse_featurestr() is called. But this shouldn't make any > >>> difference, as property setters shouldn't depend on icc_bridge. > >>> > >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >> > >> Looks okay to me, > >> > >> Reviewed-by: Andreas Färber <afaerber@suse.de> > >> > >> But using this smaller patch will still make inlining pc_new_cpu(), > >> where you are moving it to, bigger diffstat-wise (WIP). > > > > I just see this it as a reason to not inline pc_new_cpu(). :) > > Did you actually read my code? cpu_x86_create() does not allow in-place > initialization, in addition to doing the realized=true. I hadn't, sorry, I was simply thinking in general terms, where I wouldn't want to inline a function if that would mean duplicating code. I did read the qom-cpu-x86 code now, and I still don't see why you would want to duplicate existing pc_new_cpu() code that is needed on both call sites. I assume there is a way to avoid code duplication and still allow in-place initialization. Anyway, this discussion about diff sizes and duplication will be obsolete as soon as we apply a new version of the icc-bus removal patch. So I would prefer to discuss the details once we have a new patch
Am 10.03.2015 um 14:24 schrieb Eduardo Habkost: > On Tue, Mar 10, 2015 at 01:51:26PM +0100, Andreas Färber wrote: >> Am 10.03.2015 um 13:42 schrieb Eduardo Habkost: >>> On Tue, Mar 10, 2015 at 12:50:07PM +0100, Andreas Färber wrote: >>>> Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: >>>>> Instead of passing icc_bridge from the PC initialization code to >>>>> cpu_x86_create(), make the PC initialization code attach the CPU to >>>>> icc_bridge. >>>>> >>>>> The only difference here is that icc_bridge attachment will now be done >>>>> after x86_cpu_parse_featurestr() is called. But this shouldn't make any >>>>> difference, as property setters shouldn't depend on icc_bridge. >>>>> >>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>>> >>>> Looks okay to me, >>>> >>>> Reviewed-by: Andreas Färber <afaerber@suse.de> >>>> >>>> But using this smaller patch will still make inlining pc_new_cpu(), >>>> where you are moving it to, bigger diffstat-wise (WIP). >>> >>> I just see this it as a reason to not inline pc_new_cpu(). :) >> >> Did you actually read my code? cpu_x86_create() does not allow in-place >> initialization, in addition to doing the realized=true. > > I hadn't, sorry, I was simply thinking in general terms, where I > wouldn't want to inline a function if that would mean duplicating code. > > I did read the qom-cpu-x86 code now, and I still don't see why you would > want to duplicate existing pc_new_cpu() code that is needed on both call > sites. I assume there is a way to avoid code duplication and still allow > in-place initialization. > > Anyway, this discussion about diff sizes and duplication will be > obsolete as soon as we apply a new version of the icc-bus removal patch. > So I would prefer to discuss the details once we have a new patch My series (and Bharata's) cannot wait for some ominous new ICC removal patch, which you yourself said would take some time to review and test... In short, if the only thing in common is setting apic-id and the icc bridge parent, is that really worth having a function for? (Care to join #qemu?) Andreas
On Tue, Mar 10, 2015 at 02:22:01PM +0100, Andreas Färber wrote: > Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: > > Instead of passing icc_bridge from the PC initialization code to > > cpu_x86_create(), make the PC initialization code attach the CPU to > > icc_bridge. > > > > The only difference here is that icc_bridge attachment will now be done > > after x86_cpu_parse_featurestr() is called. But this shouldn't make any > > difference, as property setters shouldn't depend on icc_bridge. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > hw/i386/pc.c | 6 +++++- > > target-i386/cpu.c | 14 ++------------ > > target-i386/cpu.h | 3 +-- > > 3 files changed, 8 insertions(+), 15 deletions(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index ed54d93..66b9fa6 100644 > > --- a/hw/i386/pc.c > > +++ b/hw/i386/pc.c > > @@ -995,12 +995,16 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > > X86CPU *cpu; > > Error *local_err = NULL; > > > > - cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err); > > + cpu = cpu_x86_create(cpu_model, &local_err); > > if (local_err != NULL) { > > error_propagate(errp, local_err); > > return NULL; > > } > > > > + assert(icc_bridge); > > On second thoughts, why are you asserting here rather than setting errp? > Just add an out: below and goto out, like I did. > > On startup it doesn't matter much, but for hot-add asserting would not > be so nice. Because not having icc_bus passed as argument would be a coding error. Also, I have no idea what kind of things would break if we destroy a CPU after cpu_exec_init() was already called in instance_init.
Am 10.03.2015 um 14:30 schrieb Eduardo Habkost: > On Tue, Mar 10, 2015 at 02:22:01PM +0100, Andreas Färber wrote: >> Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: >>> Instead of passing icc_bridge from the PC initialization code to >>> cpu_x86_create(), make the PC initialization code attach the CPU to >>> icc_bridge. >>> >>> The only difference here is that icc_bridge attachment will now be done >>> after x86_cpu_parse_featurestr() is called. But this shouldn't make any >>> difference, as property setters shouldn't depend on icc_bridge. >>> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>> --- >>> hw/i386/pc.c | 6 +++++- >>> target-i386/cpu.c | 14 ++------------ >>> target-i386/cpu.h | 3 +-- >>> 3 files changed, 8 insertions(+), 15 deletions(-) >>> >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>> index ed54d93..66b9fa6 100644 >>> --- a/hw/i386/pc.c >>> +++ b/hw/i386/pc.c >>> @@ -995,12 +995,16 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, >>> X86CPU *cpu; >>> Error *local_err = NULL; >>> >>> - cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err); >>> + cpu = cpu_x86_create(cpu_model, &local_err); >>> if (local_err != NULL) { >>> error_propagate(errp, local_err); >>> return NULL; >>> } >>> >>> + assert(icc_bridge); >> >> On second thoughts, why are you asserting here rather than setting errp? >> Just add an out: below and goto out, like I did. >> >> On startup it doesn't matter much, but for hot-add asserting would not >> be so nice. > > Because not having icc_bus passed as argument would be a coding error. > > Also, I have no idea what kind of things would break if we destroy a CPU > after cpu_exec_init() was already called in instance_init. Then do it before cpu_x86_create()! :) Also, every memory allocation failure can result in an assertion (which is why I'm trying to cut down on their number). Andreas
On Tue, Mar 10, 2015 at 02:30:34PM +0100, Andreas Färber wrote: > Am 10.03.2015 um 14:24 schrieb Eduardo Habkost: > > On Tue, Mar 10, 2015 at 01:51:26PM +0100, Andreas Färber wrote: > >> Am 10.03.2015 um 13:42 schrieb Eduardo Habkost: > >>> On Tue, Mar 10, 2015 at 12:50:07PM +0100, Andreas Färber wrote: > >>>> Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: > >>>>> Instead of passing icc_bridge from the PC initialization code to > >>>>> cpu_x86_create(), make the PC initialization code attach the CPU to > >>>>> icc_bridge. > >>>>> > >>>>> The only difference here is that icc_bridge attachment will now be done > >>>>> after x86_cpu_parse_featurestr() is called. But this shouldn't make any > >>>>> difference, as property setters shouldn't depend on icc_bridge. > >>>>> > >>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >>>> > >>>> Looks okay to me, > >>>> > >>>> Reviewed-by: Andreas Färber <afaerber@suse.de> > >>>> > >>>> But using this smaller patch will still make inlining pc_new_cpu(), > >>>> where you are moving it to, bigger diffstat-wise (WIP). > >>> > >>> I just see this it as a reason to not inline pc_new_cpu(). :) > >> > >> Did you actually read my code? cpu_x86_create() does not allow in-place > >> initialization, in addition to doing the realized=true. > > > > I hadn't, sorry, I was simply thinking in general terms, where I > > wouldn't want to inline a function if that would mean duplicating code. > > > > I did read the qom-cpu-x86 code now, and I still don't see why you would > > want to duplicate existing pc_new_cpu() code that is needed on both call > > sites. I assume there is a way to avoid code duplication and still allow > > in-place initialization. > > > > Anyway, this discussion about diff sizes and duplication will be > > obsolete as soon as we apply a new version of the icc-bus removal patch. > > So I would prefer to discuss the details once we have a new patch > > My series (and Bharata's) cannot wait for some ominous new ICC removal > patch, which you yourself said would take some time to review and test... I have no idea about timing and ordering, really. It was just a guess, because the ICC removal was already in the list for review. > > In short, if the only thing in common is setting apic-id and the icc > bridge parent, is that really worth having a function for? (Care to join > #qemu?) I am not sure until I see the actual patch. Before I see it, I believe icc-bridge and apic-id is probably worth a common function. If it's just for apic-id, probably not. But that's all speaking in general terms, and once I see the actual patch I may be easily convinced that inlining will be better in this case. :) (I will be in #qemu in 1 hour.)
On Tue, Mar 10, 2015 at 02:33:17PM +0100, Andreas Färber wrote: > Am 10.03.2015 um 14:30 schrieb Eduardo Habkost: > > On Tue, Mar 10, 2015 at 02:22:01PM +0100, Andreas Färber wrote: > >> Am 05.03.2015 um 18:26 schrieb Eduardo Habkost: > >>> Instead of passing icc_bridge from the PC initialization code to > >>> cpu_x86_create(), make the PC initialization code attach the CPU to > >>> icc_bridge. > >>> > >>> The only difference here is that icc_bridge attachment will now be done > >>> after x86_cpu_parse_featurestr() is called. But this shouldn't make any > >>> difference, as property setters shouldn't depend on icc_bridge. > >>> > >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > >>> --- > >>> hw/i386/pc.c | 6 +++++- > >>> target-i386/cpu.c | 14 ++------------ > >>> target-i386/cpu.h | 3 +-- > >>> 3 files changed, 8 insertions(+), 15 deletions(-) > >>> > >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c > >>> index ed54d93..66b9fa6 100644 > >>> --- a/hw/i386/pc.c > >>> +++ b/hw/i386/pc.c > >>> @@ -995,12 +995,16 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, > >>> X86CPU *cpu; > >>> Error *local_err = NULL; > >>> > >>> - cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err); > >>> + cpu = cpu_x86_create(cpu_model, &local_err); > >>> if (local_err != NULL) { > >>> error_propagate(errp, local_err); > >>> return NULL; > >>> } > >>> > >>> + assert(icc_bridge); > >> > >> On second thoughts, why are you asserting here rather than setting errp? > >> Just add an out: below and goto out, like I did. > >> > >> On startup it doesn't matter much, but for hot-add asserting would not > >> be so nice. > > > > Because not having icc_bus passed as argument would be a coding error. > > > > Also, I have no idea what kind of things would break if we destroy a CPU > > after cpu_exec_init() was already called in instance_init. > > Then do it before cpu_x86_create()! :) > > Also, every memory allocation failure can result in an assertion (which > is why I'm trying to cut down on their number). I still think assert() is good enough (and simpler) if it's a coding error that should never happen in the first place, but I will send a new version that moves the existing error_setg() call from cpu.c to pc.c.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index ed54d93..66b9fa6 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -995,12 +995,16 @@ static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id, X86CPU *cpu; Error *local_err = NULL; - cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err); + cpu = cpu_x86_create(cpu_model, &local_err); if (local_err != NULL) { error_propagate(errp, local_err); return NULL; } + assert(icc_bridge); + qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc")); + object_unref(OBJECT(cpu)); + object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err); object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 50907d0..097924c 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2076,8 +2076,7 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp) } -X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge, - Error **errp) +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp) { X86CPU *cpu = NULL; X86CPUClass *xcc; @@ -2108,15 +2107,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge, cpu = X86_CPU(object_new(object_class_get_name(oc))); -#ifndef CONFIG_USER_ONLY - if (icc_bridge == NULL) { - error_setg(&error, "Invalid icc-bridge value"); - goto out; - } - qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc")); - object_unref(OBJECT(cpu)); -#endif - x86_cpu_parse_featurestr(CPU(cpu), features, &error); if (error) { goto out; @@ -2139,7 +2129,7 @@ X86CPU *cpu_x86_init(const char *cpu_model) Error *error = NULL; X86CPU *cpu; - cpu = cpu_x86_create(cpu_model, NULL, &error); + cpu = cpu_x86_create(cpu_model, &error); if (error) { goto out; } diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 0638d24..8d748bd 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -982,8 +982,7 @@ typedef struct CPUX86State { #include "cpu-qom.h" X86CPU *cpu_x86_init(const char *cpu_model); -X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge, - Error **errp); +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp); int cpu_x86_exec(CPUX86State *s); void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf); void x86_cpudef_setup(void);
Instead of passing icc_bridge from the PC initialization code to cpu_x86_create(), make the PC initialization code attach the CPU to icc_bridge. The only difference here is that icc_bridge attachment will now be done after x86_cpu_parse_featurestr() is called. But this shouldn't make any difference, as property setters shouldn't depend on icc_bridge. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/i386/pc.c | 6 +++++- target-i386/cpu.c | 14 ++------------ target-i386/cpu.h | 3 +-- 3 files changed, 8 insertions(+), 15 deletions(-)