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 <ehabk...@redhat.com>
> > ---
> > 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
