>> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp)
>> +{
>> + S390CPU *cpu = NULL;
>> + Error *local_err = NULL;
>
> Think the naming schema is "err" now.
>
>> +
>> + if (id >= max_cpus) {
>> + error_setg(errp, "Unable to add CPU: %" PRIi64
>> + ", max allowed: %d", id, max_cpus - 1);
>> + goto out;
>
> Could we also move this check to the realize function?
>
>> + }
>> +
>> + cpu = cpu_s390x_create(machine->cpu_model, &local_err);
>> + if (local_err != NULL) {
>> + goto out;
>> + }
>> +
>> + object_property_set_int(OBJECT(cpu), id, "id", &local_err);
>
> We should add a check in between
>
> if (err) {
> goto out;
> }
>
>> + object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>> +
>> +out:
>> + if (cpu != NULL) {
>> + object_unref(OBJECT(cpu));
>
> Is the object_unref() here correct?
> I know that we have one reference from VCPU creation. Where does the second
> one
> come from (is it from the hotplug handler? then I'd prefer a comment here :D )
>
After some digging, I believe this unref is not necessary for s390
(bus-less) and I'm now questioning the i386 code that I used as a base...
@Igor/Andreas:
In i386, looks like the unrefs were due to the ref created when adding
the cpu to the icc bus. Andreas moved the checks outside of pc_new_cpu
and explains their purpose here:
0e3bd562 - pc: Ensure non-zero CPU ref count after attaching to ICC bus
But then a subsequent patch removed the bus and left the unrefs:
46232aaa - cpu/apic: drop icc bus/bridge
Should that patch not have also dropped the unrefs in pc_hot_add_cpu()
and pc_cpus_init()?
Matt
