Re: [RFC 20/52] s390x: Replace MachineState.smp access with topology helpers

[Zhao Liu]

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);
>