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 <th...@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....@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);
>
