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 <mjros...@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
