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

[Eduardo Habkost]

On Tue, Mar 10, 2015 at 11:43:41PM +0100, Andreas Färber wrote:
> Am 10.03.2015 um 22:57 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 <address@hidden>
> > ---
> > Changes v1 -> v2:
> >  * Keep existing check for NULL icc_bridge and error reporting, instead
> >    of assing assert(icc_bridge)
> > ---
> >  hw/i386/pc.c      | 13 +++++++++++--
> >  target-i386/cpu.c | 14 ++------------
> >  target-i386/cpu.h |  3 +--
> >  3 files changed, 14 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index b5b2aad..a26e0ec 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -992,18 +992,27 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
> > level)
> >  static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> >                            DeviceState *icc_bridge, Error **errp)
> >  {
> > -    X86CPU *cpu;
> > +    X86CPU *cpu = NULL;
> >      Error *local_err = NULL;
> >  
> > -    cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
> > +    if (icc_bridge == NULL) {
> > +        error_setg(&local_err, "Invalid icc-bridge value");
> > +        goto out;
> > +    }
> > +
> > +    cpu = cpu_x86_create(cpu_model, &local_err);
> 
> We had previously discussed reference counting. Here I would expect:

I will try to extend the analysis with ownership of each reference:

> 
> OBJECT(cpu)->ref == 1

And the owner of the reference is pc_new_cpu() (cpu variable).

> 
> >      if (local_err != NULL) {
> >          error_propagate(errp, local_err);
> >          return NULL;
> >      }
> >  
> > +    qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, 
> > "icc"));
> 
> OBJECT(cpu)->ref == 2

And the owners are: pc_new_cpu()/cpu and icc_bridge.

Now, what if we error out and destroy the CPU after we already added the
CPU to icc_bridge? Is icc_bridge going to keep pointing to the dead
object, or is there some bus-detach magic somewhere that will make it
work?

> 
> > +    object_unref(OBJECT(cpu));
> 
> OBJECT(cpu)->ref == 1

Here pc_new_cpu() is dropping its reference too early! In practice it is
now borrowing the reference owned by icc_bridge, and I don't think we
should do that.

I just kept the object_unref() call here because I didn't want to change
any behavior when moving code, but I think it doesn't belong here.

> 
> > +
> >      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> >      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> 
> OBJECT(cpu)->ref == 1 or 2 depending on DeviceClass::realize :)

If it's 2, it won't be our problem as we don't own the extra reference.
It's the responsibility of whoever grabbed the extra reference.

But I assume the property setters above MUST not add any extra reference
in case they return an error. Correct?

> 
> >  
> > +out:
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          object_unref(OBJECT(cpu));
> 

And here we have something that was already broken: X86CPU instance_init
calls cpu_exec_init(), the CPU is added to the global CPU list without
increasing reference counting, and the global list will point to a
destroyed object if we enter the error path.

In other words, if anything fails after cpu_exec_init() is called, we're
screwed. In the future it will be on realize, but we probably need to
move it closer to the end of realize.


> object_unref(NULL) looks unusual but is valid.

Yes. Makes the code simpler. :)

> 
> Should we change the return NULL to jump here, too, then?

Yes, the cpu_x86_create() error check could just do a "goto out".

> 
> OBJECT(cpu)->ref == 0 or 1
> 
> I wonder whether we need another object_unref(OBJECT(cpu)) for the
> non-error case, either here or in the callers? Out of scope for this
> patch, of course.

So, how I see it: if we are returning a reference to the object, now it
belongs to the caller, and it should be the caller responsibility to
call object_unref(). So the non-error object_unref() after
qdev_set_parent_bus() is not supposed to be in pc_new_cpus(), but in the
callers. Either way we choose, we should document it so callers know who
owns the reference they get.

Alternatively, we could simply change pc_new_cpu() to _not_ return a
pointer to the CPU, and make pc_cpus_init() deal with the APIC MMIO
mapping using some another approach.

-- 
Eduardo