| Message ID | 20230213095035.158240-21-zhao1.liu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce hybrid CPU topology | expand |
On 13/02/2023 10.50, Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > When MachineState.topo is introduced, the topology related structures > become complicated. So we wrapped the access to topology fields of > MachineState.topo into some helpers, and we are using these helpers > to replace the use of MachineState.smp. > > In hw/s390x/s390-virtio-ccw.c, s390_init_cpus() needs "threads per core". > Before s390x supports hybrid, here we use smp-specific interface to get > "threads per core". > > For other cases, it's straightforward to replace topology access with > wrapped generic interfaces. ... > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c > index 3ac7ec9acf4e..d297daed1117 100644 > --- a/target/s390x/kvm/kvm.c > +++ b/target/s390x/kvm/kvm.c > @@ -406,9 +406,11 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu) > > int kvm_arch_init_vcpu(CPUState *cs) > { > - unsigned int max_cpus = MACHINE(qdev_get_machine())->smp.max_cpus; > + unsigned int max_cpus; > S390CPU *cpu = S390_CPU(cs); > + > kvm_s390_set_cpu_state(cpu, cpu->env.cpu_state); > + max_cpus = machine_topo_get_max_cpus(MACHINE(qdev_get_machine())); > cpu->irqstate = g_malloc0(VCPU_IRQ_BUF_SIZE(max_cpus)); > return 0; > } > @@ -2097,14 +2099,15 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) > > void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > { > - unsigned int max_cpus = MACHINE(qdev_get_machine())->smp.max_cpus; > - struct kvm_s390_irq_state irq_state = { > - .buf = (uint64_t) cpu->irqstate, > - .len = VCPU_IRQ_BUF_SIZE(max_cpus), > - }; > + unsigned int max_cpus; > + struct kvm_s390_irq_state irq_state; > CPUState *cs = CPU(cpu); > int32_t bytes; > > + max_cpus = machine_topo_get_max_cpus(MACHINE(qdev_get_machine())); > + irq_state.buf = (uint64_t) cpu->irqstate; > + irq_state.len = VCPU_IRQ_BUF_SIZE(max_cpus); Hi! Please don't replace struct initializers like this. There's a reason why these structs like irq_state are directly initialized with "= { ... }" at the beginning of the function: This automatically clears all fields that are not mentioned, e.g. also the "flags" field of struct kvm_s390_irq_state, which can be very important for structs that are passed to the kernel via an ioctl. You could use memset(..., 0, ...) instead, but people tend to forget that, too, so we settled on using struct initializers at the beginning instead. So please stick to that. Thanks, Thomas > if (!kvm_check_extension(kvm_state, KVM_CAP_S390_IRQ_STATE)) { > return; > } > diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c > index bc767f044381..e396a89d5540 100644 > --- a/target/s390x/tcg/excp_helper.c > +++ b/target/s390x/tcg/excp_helper.c > @@ -321,7 +321,7 @@ static void do_ext_interrupt(CPUS390XState *env) > if ((env->pending_int & INTERRUPT_EMERGENCY_SIGNAL) && > (env->cregs[0] & CR0_EMERGENCY_SIGNAL_SC)) { > MachineState *ms = MACHINE(qdev_get_machine()); > - unsigned int max_cpus = ms->smp.max_cpus; > + unsigned int max_cpus = machine_topo_get_max_cpus(ms); > > lowcore->ext_int_code = cpu_to_be16(EXT_EMERGENCY); > cpu_addr = find_first_bit(env->emergency_signals, S390_MAX_CPUS);
On Thu, Feb 16, 2023 at 02:38:55PM +0100, Thomas Huth wrote: > Date: Thu, 16 Feb 2023 14:38:55 +0100 > From: Thomas Huth <thuth@redhat.com> > Subject: Re: [RFC 20/52] s390x: Replace MachineState.smp access with > topology helpers > > On 13/02/2023 10.50, Zhao Liu wrote: > > From: Zhao Liu <zhao1.liu@intel.com> > > > > When MachineState.topo is introduced, the topology related structures > > become complicated. So we wrapped the access to topology fields of > > MachineState.topo into some helpers, and we are using these helpers > > to replace the use of MachineState.smp. > > > > In hw/s390x/s390-virtio-ccw.c, s390_init_cpus() needs "threads per core". > > Before s390x supports hybrid, here we use smp-specific interface to get > > "threads per core". > > > > For other cases, it's straightforward to replace topology access with > > wrapped generic interfaces. > ... > > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c > > index 3ac7ec9acf4e..d297daed1117 100644 > > --- a/target/s390x/kvm/kvm.c > > +++ b/target/s390x/kvm/kvm.c > > @@ -406,9 +406,11 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu) > > int kvm_arch_init_vcpu(CPUState *cs) > > { > > - unsigned int max_cpus = MACHINE(qdev_get_machine())->smp.max_cpus; > > + unsigned int max_cpus; > > S390CPU *cpu = S390_CPU(cs); > > + > > kvm_s390_set_cpu_state(cpu, cpu->env.cpu_state); > > + max_cpus = machine_topo_get_max_cpus(MACHINE(qdev_get_machine())); > > cpu->irqstate = g_malloc0(VCPU_IRQ_BUF_SIZE(max_cpus)); > > return 0; > > } > > @@ -2097,14 +2099,15 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) > > void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) > > { > > - unsigned int max_cpus = MACHINE(qdev_get_machine())->smp.max_cpus; > > - struct kvm_s390_irq_state irq_state = { > > - .buf = (uint64_t) cpu->irqstate, > > - .len = VCPU_IRQ_BUF_SIZE(max_cpus), > > - }; > > + unsigned int max_cpus; > > + struct kvm_s390_irq_state irq_state; > > CPUState *cs = CPU(cpu); > > int32_t bytes; > > + max_cpus = machine_topo_get_max_cpus(MACHINE(qdev_get_machine())); > > + irq_state.buf = (uint64_t) cpu->irqstate; > > + irq_state.len = VCPU_IRQ_BUF_SIZE(max_cpus); > > Hi! > > Please don't replace struct initializers like this. There's a reason why > these structs like irq_state are directly initialized with "= { ... }" at > the beginning of the function: This automatically clears all fields that are > not mentioned, e.g. also the "flags" field of struct kvm_s390_irq_state, > which can be very important for structs that are passed to the kernel via an > ioctl. > You could use memset(..., 0, ...) instead, but people tend to forget that, > too, so we settled on using struct initializers at the beginning instead. So > please stick to that. Thanks Thomas! Sorry I didn't notice this, I'll fix it and be careful in the future. Zhao > > Thanks, > Thomas > > > > if (!kvm_check_extension(kvm_state, KVM_CAP_S390_IRQ_STATE)) { > > return; > > } > > diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c > > index bc767f044381..e396a89d5540 100644 > > --- a/target/s390x/tcg/excp_helper.c > > +++ b/target/s390x/tcg/excp_helper.c > > @@ -321,7 +321,7 @@ static void do_ext_interrupt(CPUS390XState *env) > > if ((env->pending_int & INTERRUPT_EMERGENCY_SIGNAL) && > > (env->cregs[0] & CR0_EMERGENCY_SIGNAL_SC)) { > > MachineState *ms = MACHINE(qdev_get_machine()); > > - unsigned int max_cpus = ms->smp.max_cpus; > > + unsigned int max_cpus = machine_topo_get_max_cpus(ms); > > lowcore->ext_int_code = cpu_to_be16(EXT_EMERGENCY); > > cpu_addr = find_first_bit(env->emergency_signals, S390_MAX_CPUS); >
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index f22f61b8b6ac..9b5020a0d395 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -88,7 +88,7 @@ static void s390_init_cpus(MachineState *machine) S390CcwMachineClass *s390mc = S390_CCW_MACHINE_CLASS(mc); int i; - if (machine->smp.threads > s390mc->max_threads) { + if (machine_topo_get_smp_threads(machine) > s390mc->max_threads) { error_report("S390 does not support more than %d threads.", s390mc->max_threads); exit(1); @@ -97,7 +97,7 @@ static void s390_init_cpus(MachineState *machine) /* initialize possible_cpus */ mc->possible_cpu_arch_ids(machine); - for (i = 0; i < machine->smp.cpus; i++) { + for (i = 0; i < machine_topo_get_cpus(machine); i++) { s390x_new_cpu(machine->cpu_type, i, &error_fatal); } } @@ -540,7 +540,7 @@ static CpuInstanceProperties s390_cpu_index_to_props(MachineState *ms, static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms) { int i; - unsigned int max_cpus = ms->smp.max_cpus; + unsigned int max_cpus = machine_topo_get_max_cpus(ms); if (ms->possible_cpus) { g_assert(ms->possible_cpus && ms->possible_cpus->len == max_cpus); diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c index eff74479f458..2aef1a57a591 100644 --- a/hw/s390x/sclp.c +++ b/hw/s390x/sclp.c @@ -127,7 +127,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb) prepare_cpu_entries(machine, entries_start, &cpu_count); read_info->entries_cpu = cpu_to_be16(cpu_count); read_info->offset_cpu = cpu_to_be16(offset_cpu); - read_info->highest_cpu = cpu_to_be16(machine->smp.max_cpus - 1); + read_info->highest_cpu = + cpu_to_be16(machine_topo_get_max_cpus(machine) - 1); read_info->ibc_val = cpu_to_be32(s390_get_ibc_val()); diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c index 948e4bd3e09e..617f23b39913 100644 --- a/target/s390x/cpu-sysemu.c +++ b/target/s390x/cpu-sysemu.c @@ -125,7 +125,7 @@ bool s390_cpu_realize_sysemu(DeviceState *dev, Error **errp) { S390CPU *cpu = S390_CPU(dev); MachineState *ms = MACHINE(qdev_get_machine()); - unsigned int max_cpus = ms->smp.max_cpus; + unsigned int max_cpus = machine_topo_get_max_cpus(ms); if (cpu->env.core_id >= max_cpus) { error_setg(errp, "Unable to add CPU with core-id: %" PRIu32 diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c index 3ac7ec9acf4e..d297daed1117 100644 --- a/target/s390x/kvm/kvm.c +++ b/target/s390x/kvm/kvm.c @@ -406,9 +406,11 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu) int kvm_arch_init_vcpu(CPUState *cs) { - unsigned int max_cpus = MACHINE(qdev_get_machine())->smp.max_cpus; + unsigned int max_cpus; S390CPU *cpu = S390_CPU(cs); + kvm_s390_set_cpu_state(cpu, cpu->env.cpu_state); + max_cpus = machine_topo_get_max_cpus(MACHINE(qdev_get_machine())); cpu->irqstate = g_malloc0(VCPU_IRQ_BUF_SIZE(max_cpus)); return 0; } @@ -2097,14 +2099,15 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state) void kvm_s390_vcpu_interrupt_pre_save(S390CPU *cpu) { - unsigned int max_cpus = MACHINE(qdev_get_machine())->smp.max_cpus; - struct kvm_s390_irq_state irq_state = { - .buf = (uint64_t) cpu->irqstate, - .len = VCPU_IRQ_BUF_SIZE(max_cpus), - }; + unsigned int max_cpus; + struct kvm_s390_irq_state irq_state; CPUState *cs = CPU(cpu); int32_t bytes; + max_cpus = machine_topo_get_max_cpus(MACHINE(qdev_get_machine())); + irq_state.buf = (uint64_t) cpu->irqstate; + irq_state.len = VCPU_IRQ_BUF_SIZE(max_cpus); + if (!kvm_check_extension(kvm_state, KVM_CAP_S390_IRQ_STATE)) { return; } diff --git a/target/s390x/tcg/excp_helper.c b/target/s390x/tcg/excp_helper.c index bc767f044381..e396a89d5540 100644 --- a/target/s390x/tcg/excp_helper.c +++ b/target/s390x/tcg/excp_helper.c @@ -321,7 +321,7 @@ static void do_ext_interrupt(CPUS390XState *env) if ((env->pending_int & INTERRUPT_EMERGENCY_SIGNAL) && (env->cregs[0] & CR0_EMERGENCY_SIGNAL_SC)) { MachineState *ms = MACHINE(qdev_get_machine()); - unsigned int max_cpus = ms->smp.max_cpus; + unsigned int max_cpus = machine_topo_get_max_cpus(ms); lowcore->ext_int_code = cpu_to_be16(EXT_EMERGENCY); cpu_addr = find_first_bit(env->emergency_signals, S390_MAX_CPUS);