Re: [Qemu-devel] [PATCH v4 18/21] s390x: implement query-hotpluggable-cpus

[David Hildenbrand <david@redhat.com>]

On 12.09.2017 15:43, Igor Mammedov wrote:
> On Mon, 11 Sep 2017 17:21:47 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> CPU hotplug is only possible on a per core basis on s390x.
>>
>> As we now have ms->possible_cpus, we can get rid of the global variable
>> cpu_states.
>>
>> While rewriting s390_cpu_addr2state() completely to be based on
>> possible_cpus, move it to cpu.c, as it is independent of the virtio-ccw
>> machine.
> I'd split patch on
>  1) introduce possible cpus 
>  2) rewrite s390_cpu_addr2state() using #2

Than I have to keep the global variable + setting it for one patch.
Might not be worth the trouble. Will have a look.

[...]
>>  
>> +static CPUArchId *s390_find_cpu_slot(MachineState *ms, uint32_t core_id,
>> +                                     int *idx)
>> +{
>> +    if (core_id >= ms->possible_cpus->len) {
>> +        return NULL;
>> +    }
>> +    /* core_id corresponds to the index */
>> +    if (idx) {
>> +        *idx = core_id;
>> +    }
>> +    return &ms->possible_cpus->cpus[core_id];
>> +}
> it looks like cpu_index == core_id == idx in possible_cpus,
> is this helper really necessary?
> (we have it in x86 because of possible not 1:1 mapping)
> 
> I'd drop it and just access array directly

Just kept this because the other architectures also have this. I can of
course drop it. The nice thing about this helper is the comment :)

[...]
>>  }
>> +
>> +S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> +{
> target/cpu.c and cpu itself preferably shouldn't pull in
> or depend on machine, so I'd keep s390_cpu_addr2state() where it's now
> or somewhere in board related files

Thomas requested this. I actually don't care., but it looks like a
generic "get_cpu_by_arch_id" function. But I don't really want to go
that additional path now. And also I don't want to move this back and forth.

Thomas, what's your opinion?

Thanks!


-- 

Thanks,

David