diff mbox series

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

Message ID 20230213095035.158240-21-zhao1.liu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Introduce hybrid CPU topology | expand

Commit Message

Zhao Liu Feb. 13, 2023, 9:50 a.m. UTC 
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.

Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@linux.ibm.com>
Cc: Eric Farman <farman@linux.ibm.com>
Cc: Thomas Huth <thuth@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 hw/s390x/s390-virtio-ccw.c     |  6 +++---
 hw/s390x/sclp.c                |  3 ++-
 target/s390x/cpu-sysemu.c      |  2 +-
 target/s390x/kvm/kvm.c         | 15 +++++++++------
 target/s390x/tcg/excp_helper.c |  2 +-
 5 files changed, 16 insertions(+), 12 deletions(-)

Comments

Thomas Huth Feb. 16, 2023, 1:38 p.m. UTC | #1 
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);
Zhao Liu Feb. 17, 2023, 3:38 a.m. UTC | #2 
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 mbox series

Patch

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