[Qemu-devel] [PATCH 0/1] target-i386: Move icc_bridge code to PC

[Qemu-devel] [PATCH 0/1] target-i386: Move icc_bridge code to PC

[Eduardo Habkost]

This removes yet another chunk of PC-specific code from target-i386/cpu.c and
moves it to PC code.

WIth this we get closer to being able to change target-i386 to use
cpu_generic_init().

This series is based on my x86 tree, located at:
  https://github.com/ehabkost/qemu.git x86

Eduardo Habkost (1):
  target-i386: Remove icc_bridge parameter from cpu_x86_create()

 hw/i386/pc.c      |  6 +++++-
 target-i386/cpu.c | 14 ++------------
 target-i386/cpu.h |  3 +--
 3 files changed, 8 insertions(+), 15 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[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);
+    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);
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Andreas Färber]

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[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(). :)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Andreas Färber]

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Andreas Färber]

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]

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[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

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Andreas Färber]

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[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.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Andreas Färber]

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

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Eduardo Habkost]

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.)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/1] target-i386: Remove icc_bridge parameter from cpu_x86_create()

[Eduardo Habkost]

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.

-- 
Eduardo