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

[Thomas Huth]

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