On 12.09.2017 15:43, Igor Mammedov wrote: > On Mon, 11 Sep 2017 17:21:47 +0200 > David Hildenbrand <da...@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