Re: [Qemu-devel] [PATCH v6 7/7] s390x/cpu: Allow hotplug of CPUs

[Matthew Rosato <mjrosato@linux.vnet.ibm.com>]

On 02/22/2016 12:22 PM, Andreas Färber wrote:
> Am 22.02.2016 um 18:06 schrieb Matthew Rosato:
>> Implement cpu hotplug routine and add the machine hook.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 33 +++++++++++++++++++++++++++++++++
>>  target-s390x/cpu.c         |  7 +++++++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 3090e76..ea007e8 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -22,6 +22,7 @@
>>  #include "s390-pci-bus.h"
>>  #include "hw/s390x/storage-keys.h"
>>  #include "hw/compat.h"
>> +#include "qom/cpu.h"
>>  
>>  #define TYPE_S390_CCW_MACHINE               "s390-ccw-machine"
>>  
>> @@ -185,6 +186,37 @@ static HotplugHandler *s390_get_hotplug_handler(MachineState *machine,
>>      return NULL;
>>  }
>>  
>> +static void s390_hot_add_cpu(const int64_t id, Error **errp)
>> +{
>> +    MachineState *machine = MACHINE(qdev_get_machine());
>> +
>> +    if (id < 0) {
>> +        error_setg(errp, "Invalid CPU id: %" PRIi64, id);
>> +        return;
>> +    }
>> +
>> +    if (cpu_exists(id)) {
>> +        error_setg(errp, "Unable to add CPU: %" PRIi64
>> +                   ", it already exists", id);
>> +        return;
>> +    }
>> +
>> +    if (id >= max_cpus) {
>> +        error_setg(errp, "Unable to add CPU: %" PRIi64
>> +                   ", max allowed: %d", id, max_cpus - 1);
>> +        return;
>> +    }
>> +
>> +    if (id != (last_cpu->cpu_index + 1)) {
> 
> This assumption about the order of the CPU list looks dangerous to me.
> Aren't there global int fields used by x86 already for the number of
> CPUs that you could use instead? That will allow you to drop the ugly
> preceding renaming patch. Please avoid messing with the CPU list
> directly from target code.
> 

Not that I can find.  We were keeping track of it with our own int for
s390x, I just switched to this approach per review suggestion -- Sounds
like you'd rather I bring back the int and inspect that, that's fine by
me.

>> +        error_setg(errp, "Unable to add CPU: %" PRIi64
>> +                   ", The next available id is %d", id,
>> +                   (last_cpu->cpu_index + 1));
>> +        return;
>> +    }
>> +
>> +    cpu_s390x_init(machine->cpu_model);
> 
> No proper error handling - that's why blindly reusing these old helpers
> is a bad idea.
> 

Good point -- Let me re-work this into something more like f(model, id,
local_err).

Matt