[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 7/7] s390x/cpu: Allow hotplug of CPUs
From: |
Matthew Rosato |
Subject: |
Re: [Qemu-devel] [PATCH v6 7/7] s390x/cpu: Allow hotplug of CPUs |
Date: |
Mon, 22 Feb 2016 15:54:53 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 |
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 <address@hidden>
>> ---
>> 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
- [Qemu-devel] [PATCH v6 0/7] Allow hotplug of s390 CPUs, Matthew Rosato, 2016/02/22
- [Qemu-devel] [PATCH v6 3/7] s390x/cpu: Move some CPU initialization into realize, Matthew Rosato, 2016/02/22
- [Qemu-devel] [PATCH v6 2/7] s390x/cpu: Set initial CPU state in common routine, Matthew Rosato, 2016/02/22
- [Qemu-devel] [PATCH v6 5/7] s390/virtio-ccw: Add hotplug handler, Matthew Rosato, 2016/02/22
- [Qemu-devel] [PATCH v6 6/7] cpu: Add a last_cpu macro, Matthew Rosato, 2016/02/22
- [Qemu-devel] [PATCH v6 7/7] s390x/cpu: Allow hotplug of CPUs, Matthew Rosato, 2016/02/22
- [Qemu-devel] [PATCH v6 4/7] s390x/cpu: Add CPU property links, Matthew Rosato, 2016/02/22
- [Qemu-devel] [PATCH v6 1/7] s390x/cpu: Cleanup init in preparation for hotplug, Matthew Rosato, 2016/02/22