On 02/18/2016 04:36 AM, Igor Mammedov wrote:
> On Wed, 17 Feb 2016 15:12:34 -0500
> Matthew Rosato <mjros...@linux.vnet.ibm.com> wrote:
>
>> Introduce s390_register_cpustate, which will set the
>> machine/cpu[n] link with the current CPU state. Additionally,
>> maintain an array of state pointers indexed by CPU id for fast lookup
>> during interrupt handling.
>>
> cosmetic nit,
> you call qdev_get_machine() several time withing single function
> or when function already has machine argument. You probably shouldn't
> do it or do it once and use return value throughout function.
>
Oops, will fix that.
>> Signed-off-by: Matthew Rosato <mjros...@linux.vnet.ibm.com>
>> ---
>> hw/s390x/s390-virtio.c | 45 ++++++++++++++++++++++++++++++++++++---------
>> target-s390x/cpu.c | 14 +++++++++++++-
>> target-s390x/cpu.h | 1 +
>> 3 files changed, 50 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
>> index b3707f4..6a1e3f6 100644
>> --- a/hw/s390x/s390-virtio.c
>> +++ b/hw/s390x/s390-virtio.c
>> @@ -60,15 +60,35 @@
>> #define S390_TOD_CLOCK_VALUE_MISSING 0x00
>> #define S390_TOD_CLOCK_VALUE_PRESENT 0x01
>>
>> -static S390CPU **ipi_states;
>> +static S390CPU **cpu_states;
>>
>> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> {
>> - if (cpu_addr >= smp_cpus) {
>> + if (cpu_addr >= max_cpus) {
>> return NULL;
>> }
>>
>> - return ipi_states[cpu_addr];
>> + /* Fast lookup via CPU ID */
>> + return cpu_states[cpu_addr];
>> +}
>> +
>> +int s390_register_cpustate(uint16_t cpu_addr, S390CPU *state)
> isn't it the board responsibility to link CPU somewhere?
> I'd suggest to use for it generic device hotplug hooks
> see how get_hotplug_handler() and pc_machine_device_plug_cb() are used,
> it's executed after CPU's realize is successfully completed.
>
OK, I'll have a look at this.
>> +{
>> + gchar *name;
>> + int r = 0;
>> +
>> + name = g_strdup_printf("cpu[%i]", cpu_addr);
>> + if (object_property_get_link(qdev_get_machine(), name, NULL)) {
>> + r = -EEXIST;
>> + goto out;
>> + }
>> +
>> + object_property_set_link(qdev_get_machine(), OBJECT(state), name,\
>> + &error_abort);
>> +
>> +out:
>> + g_free(name);
>> + return r;
>> }
>>
>> void s390_init_ipl_dev(const char *kernel_filename,
>> @@ -98,19 +118,26 @@ void s390_init_ipl_dev(const char *kernel_filename,
>> void s390_init_cpus(MachineState *machine)
>> {
>> int i;
>> + gchar *name;
>>
>> if (machine->cpu_model == NULL) {
>> machine->cpu_model = "host";
>> }
>>
>> - ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
>> -
>> - for (i = 0; i < smp_cpus; i++) {
>> - S390CPU *cpu;
>> + cpu_states = g_malloc0(sizeof(S390CPU *) * max_cpus);
>>
>> - cpu = cpu_s390x_init(machine->cpu_model);
>> + for (i = 0; i < max_cpus; i++) {
>> + name = g_strdup_printf("cpu[%i]", i);
>> + object_property_add_link(qdev_get_machine(), name, TYPE_S390_CPU,
>> + (Object **) &cpu_states[i],
>> + object_property_allow_set_link,
>> + OBJ_PROP_LINK_UNREF_ON_RELEASE,
>> + &error_abort);
>> + g_free(name);
>> + }
>>
>> - ipi_states[i] = cpu;
>> + for (i = 0; i < smp_cpus; i++) {
>> + cpu_s390x_init(machine->cpu_model);
>> }
>> }
>>
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 5f6411f..620b2e3 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -32,6 +32,7 @@
>> #include "trace.h"
>> #ifndef CONFIG_USER_ONLY
>> #include "sysemu/arch_init.h"
>> +#include "sysemu/sysemu.h"
>> #endif
>>
>> #define CR0_RESET 0xE0UL
>> @@ -202,9 +203,20 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
>> **errp)
>> CPUS390XState *env = &cpu->env;
>>
>> #if !defined(CONFIG_USER_ONLY)
>> + if (s390_register_cpustate(next_cpu_id, cpu) < 0) {
>> + error_setg(errp, "Cannot have more than %d CPUs", max_cpus);
>> + return;
>> + }
>> qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
>> -#endif
>> + env->cpu_num = next_cpu_id;
>> + while (next_cpu_id < max_cpus - 1) {
>> + if (!cpu_exists(++next_cpu_id)) {
>> + break;
>> + }
>> + }
> maybe it's possible to reuse cpu_get_free_index() here?
>
Hmm, but based on your other comment, there's no need to track
next_cpu_id in this fashion afterall. To bring these 2 points together,
how about the following:
1) In Patch 4 (this patch), drop next_cpu_id. Since we don't allow
unplug and only do sequential cpu adds, rely on env->cpu_num =
cs->cpu_index here in realizefn.
2) For hotplug in patch 5, check against QTAILQ_LAST(&cpus, CPUTailQ) as
you suggest.
I tried this out quick, seems to work fine.
Matt
