[v15,02/11] s390x/cpu topology: add topology entries on CPU hotplug

The topology information are attributes of the CPU and are
specified during the CPU device creation.

On hot plug we:
- calculate the default values for the topology for drawers,
  books and sockets in the case they are not specified.
- verify the CPU attributes
- check that we have still room on the desired socket

The possibility to insert a CPU in a mask is dependent on the
number of cores allowed in a socket, a book or a drawer, the
checking is done during the hot plug of the CPU to have an
immediate answer.

If the complete topology is not specified, the core is added
in the physical topology based on its core ID and it gets
defaults values for the modifier attributes.

This way, starting QEMU without specifying the topology can
still get some advantage of the CPU topology.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/hw/s390x/cpu-topology.h |  24 +++
 hw/s390x/cpu-topology.c         | 256 ++++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c      |  23 ++-
 hw/s390x/meson.build            |   1 +
 4 files changed, 302 insertions(+), 2 deletions(-)
 create mode 100644 hw/s390x/cpu-topology.c

On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
> The topology information are attributes of the CPU and are
> specified during the CPU device creation.
> 
> On hot plug we:
> - calculate the default values for the topology for drawers,
>   books and sockets in the case they are not specified.
> - verify the CPU attributes
> - check that we have still room on the desired socket
> 
> The possibility to insert a CPU in a mask is dependent on the
> number of cores allowed in a socket, a book or a drawer, the
> checking is done during the hot plug of the CPU to have an
> immediate answer.
> 
> If the complete topology is not specified, the core is added
> in the physical topology based on its core ID and it gets
> defaults values for the modifier attributes.
> 
> This way, starting QEMU without specifying the topology can
> still get some advantage of the CPU topology.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/hw/s390x/cpu-topology.h |  24 +++
>  hw/s390x/cpu-topology.c         | 256 ++++++++++++++++++++++++++++++++
>  hw/s390x/s390-virtio-ccw.c      |  23 ++-
>  hw/s390x/meson.build            |   1 +
>  4 files changed, 302 insertions(+), 2 deletions(-)
>  create mode 100644 hw/s390x/cpu-topology.c
> 
[...]
> 
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> new file mode 100644
> index 0000000000..12df4eca6c
> --- /dev/null
> +++ b/hw/s390x/cpu-topology.c
> @@ -0,0 +1,256 @@
> +/*
> + * CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> +
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/error-report.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/boards.h"
> +#include "qemu/typedefs.h"
> +#include "target/s390x/cpu.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
> +#include "hw/s390x/cpu-topology.h"
> +
> +/*
> + * s390_topology is used to keep the topology information.
> + * .list: queue the topology entries inside which
> + *        we keep the information on the CPU topology.

.list doesn't exist yet.

> + * .socket: tracks information on the count of cores per socket.
> + * .smp: keeps track of the machine topology.
> + *
> + */
> +S390Topology s390_topology = {
> +    /* will be initialized after the cpu model is realized */
> +    .cores_per_socket = NULL,
> +    .smp = NULL,
> +};
> 
[...]
> +
> +/**
> + * s390_topology_cpu_default:
> + * @cpu: pointer to a S390CPU
> + * @errp: Error pointer
> + *
> + * Setup the default topology for unset attributes.

My suggestion:
Setup the default topology if no attributes are already set.
Passing a CPU with some, but not all, attributes set is considered an error.

> + *
> + * The function accept only all all default values or all set values
> + * for the geometry topology.

acceptS, all all
If you take my suggestion, you can just drop this sentence.

> + *
> + * The function calculates the (drawer_id, book_id, socket_id)
> + * topology by filling the cores starting from the first socket
> + * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).
> + *
> + * CPU type, polarity and dedication have defaults values set in the
> + * s390x_cpu_properties.
> + */
> +static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
> +{
> +    CpuTopology *smp = s390_topology.smp;
> +    CPUS390XState *env = &cpu->env;
> +
> +    /* All geometry topology attributes must be set or all unset */
> +    if ((env->socket_id < 0 || env->book_id < 0 || env->drawer_id < 0) &&
> +        (env->socket_id >= 0 || env->book_id >= 0 || env->drawer_id >= 0)) {
> +        error_setg(errp,
> +                   "Please define all or none of the topology geometry attributes");
> +        return;
> +    }
> +
> +    /* Check if one of the geometry topology is unset */
> +    if (env->socket_id < 0) {
> +        /* Calculate default geometry topology attributes */
> +        env->socket_id = (env->core_id / smp->cores) % smp->sockets;
> +        env->book_id = (env->core_id / (smp->sockets * smp->cores)) %
> +                       smp->books;
> +        env->drawer_id = (env->core_id /
> +                          (smp->books * smp->sockets * smp->cores)) %
> +                         smp->drawers;
> +    }
> +}
> 
[...]
> +
> +/**
> + * s390_set_core_in_socket:
> + * @cpu: the new S390CPU to insert in the topology structure
> + * @drawer_id: new drawer_id
> + * @book_id: new book_id
> + * @socket_id: new socket_id
> + * @creation: if is true the CPU is a new CPU and there is no old socket
> + *            to handle.
> + *            if is false, this is a moving the CPU and old socket count
> + *            must be decremented.
> + * @errp: the error pointer
> + *
> + */
> +static void s390_set_core_in_socket(S390CPU *cpu, int drawer_id, int book_id,

Maybe name it s390_(topology_)?add_core_to_socket instead.

> +                                    int socket_id, bool creation, Error **errp)
> +{
> +    int old_socket = s390_socket_nb(cpu);
> +    int new_socket;
> +
> +    if (creation) {
> +        new_socket = old_socket;
> +    } else {

You need parentheses here.

> +        new_socket = drawer_id * s390_topology.smp->books +
                       (
> +                     book_id * s390_topology.smp->sockets +
                               )
> +                     socket_id;
> +    }
> +
> +    /* Check for space on new socket */
> +    if ((new_socket != old_socket) &&
> +        (s390_topology.cores_per_socket[new_socket] >=
> +         s390_topology.smp->cores)) {
> +        error_setg(errp, "No more space on this socket");
> +        return;
> +    }
> +
> +    /* Update the count of cores in sockets */
> +    s390_topology.cores_per_socket[new_socket] += 1;
> +    if (!creation) {
> +        s390_topology.cores_per_socket[old_socket] -= 1;
> +    }
> +}
> 
[...]

> +/**
> + * s390_topology_set_cpu:
> + * @ms: MachineState used to initialize the topology structure on
> + *      first call.
> + * @cpu: the new S390CPU to insert in the topology structure
> + * @errp: the error pointer
> + *
> + * Called from CPU Hotplug to check and setup the CPU attributes
> + * before to insert the CPU in the topology.
> + */
> +void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp)

The name is rather non informative.
s390_topology_setup_cpu ?

> +{
> +    ERRP_GUARD();
> +
> +    /*
> +     * We do not want to initialize the topology if the cpu model
> +     * does not support topology consequently, we have to wait for
                                   ^
Still think there should be a comma here.

> +     * the first CPU to be realized, which realizes the CPU model
> +     * to initialize the topology structures.
> +     *
> +     * s390_topology_set_cpu() is called from the cpu hotplug.
> +     */
> +    if (!s390_topology.cores_per_socket) {
> +        s390_topology_init(ms);
> +    }
> +
> +    s390_topology_check(cpu, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
> +    /* Set the CPU inside the socket */
> +    s390_set_core_in_socket(cpu, 0, 0, 0, true, errp);
> +    if (*errp) {
> +        return;
> +    }
> +
> +    /* topology tree is reflected in props */
> +    s390_update_cpu_props(ms, cpu);
> +}
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index f3cc845d3b..9bc51a83f4 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -44,6 +44,7 @@
>  #include "hw/s390x/pv.h"
>  #include "migration/blocker.h"
>  #include "qapi/visitor.h"
> +#include "hw/s390x/cpu-topology.h"
>  
>  static Error *pv_mig_blocker;
>  
> @@ -310,10 +311,18 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
>  {
>      MachineState *ms = MACHINE(hotplug_dev);
>      S390CPU *cpu = S390_CPU(dev);
> +    ERRP_GUARD();
>  
>      g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
>      ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
>  
> +    if (s390_has_topology()) {
> +        s390_topology_set_cpu(ms, cpu, errp);
> +        if (*errp) {
> +            return;
> +        }
> +    }
> +
>      if (dev->hotplugged) {
>          raise_irq_cpu_hotplug();
>      }
> @@ -551,11 +560,21 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
>                                    sizeof(CPUArchId) * max_cpus);
>      ms->possible_cpus->len = max_cpus;
>      for (i = 0; i < ms->possible_cpus->len; i++) {
> +        CpuInstanceProperties *props = &ms->possible_cpus->cpus[i].props;
> +
>          ms->possible_cpus->cpus[i].type = ms->cpu_type;
>          ms->possible_cpus->cpus[i].vcpus_count = 1;
>          ms->possible_cpus->cpus[i].arch_id = i;
> -        ms->possible_cpus->cpus[i].props.has_core_id = true;
> -        ms->possible_cpus->cpus[i].props.core_id = i;
> +
> +        props->has_core_id = true;
> +        props->core_id = i;
> +        props->has_socket_id = true;
> +        props->socket_id = i / ms->smp.cores;
> +        props->has_book_id = true;
> +        props->book_id = i / (ms->smp.cores * ms->smp.sockets);
> +        props->has_drawer_id = true;
> +        props->drawer_id = i /
> +                           (ms->smp.cores * ms->smp.sockets * ms->smp.books);

You need to calculate the modulus like in s390_topology_cpu_default, right?

>      }
>  
>      return ms->possible_cpus;
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index f291016fee..58dfbdff4f 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -24,6 +24,7 @@ s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
>    's390-stattrib-kvm.c',
>    'pv.c',
>    's390-pci-kvm.c',
> +  'cpu-topology.c',
>  ))
>  s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
>    'tod-tcg.c',

On 2/2/23 17:42, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
>> The topology information are attributes of the CPU and are
>> specified during the CPU device creation.
>>
>> On hot plug we:
>> - calculate the default values for the topology for drawers,
>>    books and sockets in the case they are not specified.
>> - verify the CPU attributes
>> - check that we have still room on the desired socket
>>
>> The possibility to insert a CPU in a mask is dependent on the
>> number of cores allowed in a socket, a book or a drawer, the
>> checking is done during the hot plug of the CPU to have an
>> immediate answer.
>>
>> If the complete topology is not specified, the core is added
>> in the physical topology based on its core ID and it gets
>> defaults values for the modifier attributes.
>>
>> This way, starting QEMU without specifying the topology can
>> still get some advantage of the CPU topology.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/hw/s390x/cpu-topology.h |  24 +++
>>   hw/s390x/cpu-topology.c         | 256 ++++++++++++++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c      |  23 ++-
>>   hw/s390x/meson.build            |   1 +
>>   4 files changed, 302 insertions(+), 2 deletions(-)
>>   create mode 100644 hw/s390x/cpu-topology.c
>>
> [...]
>>
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..12df4eca6c
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -0,0 +1,256 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
>> +
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu/error-report.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/boards.h"
>> +#include "qemu/typedefs.h"
>> +#include "target/s390x/cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>> +#include "hw/s390x/cpu-topology.h"
>> +
>> +/*
>> + * s390_topology is used to keep the topology information.
>> + * .list: queue the topology entries inside which
>> + *        we keep the information on the CPU topology.
> 
> .list doesn't exist yet.

right, I move it to the corresponding patch.

> 
>> + * .socket: tracks information on the count of cores per socket.
>> + * .smp: keeps track of the machine topology.
>> + *
>> + */
>> +S390Topology s390_topology = {
>> +    /* will be initialized after the cpu model is realized */
>> +    .cores_per_socket = NULL,
>> +    .smp = NULL,
>> +};
>>
> [...]
>> +
>> +/**
>> + * s390_topology_cpu_default:
>> + * @cpu: pointer to a S390CPU
>> + * @errp: Error pointer
>> + *
>> + * Setup the default topology for unset attributes.
> 
> My suggestion:
> Setup the default topology if no attributes are already set.
> Passing a CPU with some, but not all, attributes set is considered an error.

Thanks, I take your suggestion.

> 
>> + *
>> + * The function accept only all all default values or all set values
>> + * for the geometry topology.
> 
> acceptS, all all
> If you take my suggestion, you can just drop this sentence.
> 
>> + *
>> + * The function calculates the (drawer_id, book_id, socket_id)
>> + * topology by filling the cores starting from the first socket
>> + * (0, 0, 0) up to the last (smp->drawers, smp->books, smp->sockets).
>> + *
>> + * CPU type, polarity and dedication have defaults values set in the
>> + * s390x_cpu_properties.
>> + */
>> +static void s390_topology_cpu_default(S390CPU *cpu, Error **errp)
>> +{
>> +    CpuTopology *smp = s390_topology.smp;
>> +    CPUS390XState *env = &cpu->env;
>> +
>> +    /* All geometry topology attributes must be set or all unset */
>> +    if ((env->socket_id < 0 || env->book_id < 0 || env->drawer_id < 0) &&
>> +        (env->socket_id >= 0 || env->book_id >= 0 || env->drawer_id >= 0)) {
>> +        error_setg(errp,
>> +                   "Please define all or none of the topology geometry attributes");
>> +        return;
>> +    }
>> +
>> +    /* Check if one of the geometry topology is unset */
>> +    if (env->socket_id < 0) {
>> +        /* Calculate default geometry topology attributes */
>> +        env->socket_id = (env->core_id / smp->cores) % smp->sockets;
>> +        env->book_id = (env->core_id / (smp->sockets * smp->cores)) %
>> +                       smp->books;
>> +        env->drawer_id = (env->core_id /
>> +                          (smp->books * smp->sockets * smp->cores)) %
>> +                         smp->drawers;
>> +    }
>> +}
>>
> [...]
>> +
>> +/**
>> + * s390_set_core_in_socket:
>> + * @cpu: the new S390CPU to insert in the topology structure
>> + * @drawer_id: new drawer_id
>> + * @book_id: new book_id
>> + * @socket_id: new socket_id
>> + * @creation: if is true the CPU is a new CPU and there is no old socket
>> + *            to handle.
>> + *            if is false, this is a moving the CPU and old socket count
>> + *            must be decremented.
>> + * @errp: the error pointer
>> + *
>> + */
>> +static void s390_set_core_in_socket(S390CPU *cpu, int drawer_id, int book_id,
> 
> Maybe name it s390_(topology_)?add_core_to_socket instead.

OK, it is better

> 
>> +                                    int socket_id, bool creation, Error **errp)
>> +{
>> +    int old_socket = s390_socket_nb(cpu);
>> +    int new_socket;
>> +
>> +    if (creation) {
>> +        new_socket = old_socket;
>> +    } else {
> 
> You need parentheses here.
> 
>> +        new_socket = drawer_id * s390_topology.smp->books +
>                         (
>> +                     book_id * s390_topology.smp->sockets +
>                                 )
>> +                     socket_id;

If you prefer I can us parentheses.


>> +    }
>> +
>> +    /* Check for space on new socket */
>> +    if ((new_socket != old_socket) &&
>> +        (s390_topology.cores_per_socket[new_socket] >=
>> +         s390_topology.smp->cores)) {
>> +        error_setg(errp, "No more space on this socket");
>> +        return;
>> +    }
>> +
>> +    /* Update the count of cores in sockets */
>> +    s390_topology.cores_per_socket[new_socket] += 1;
>> +    if (!creation) {
>> +        s390_topology.cores_per_socket[old_socket] -= 1;
>> +    }
>> +}
>>
> [...]
> 
>> +/**
>> + * s390_topology_set_cpu:
>> + * @ms: MachineState used to initialize the topology structure on
>> + *      first call.
>> + * @cpu: the new S390CPU to insert in the topology structure
>> + * @errp: the error pointer
>> + *
>> + * Called from CPU Hotplug to check and setup the CPU attributes
>> + * before to insert the CPU in the topology.
>> + */
>> +void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp)
> 
> The name is rather non informative.
> s390_topology_setup_cpu ?

OK

> 
>> +{
>> +    ERRP_GUARD();
>> +
>> +    /*
>> +     * We do not want to initialize the topology if the cpu model
>> +     * does not support topology consequently, we have to wait for
>                                     ^
> Still think there should be a comma here.

OK

> 
>> +     * the first CPU to be realized, which realizes the CPU model
>> +     * to initialize the topology structures.
>> +     *
>> +     * s390_topology_set_cpu() is called from the cpu hotplug.
>> +     */
>> +    if (!s390_topology.cores_per_socket) {
>> +        s390_topology_init(ms);
>> +    }
>> +
>> +    s390_topology_check(cpu, errp);
>> +    if (*errp) {
>> +        return;
>> +    }
>> +
>> +    /* Set the CPU inside the socket */
>> +    s390_set_core_in_socket(cpu, 0, 0, 0, true, errp);
>> +    if (*errp) {
>> +        return;
>> +    }
>> +
>> +    /* topology tree is reflected in props */
>> +    s390_update_cpu_props(ms, cpu);
>> +}
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index f3cc845d3b..9bc51a83f4 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -44,6 +44,7 @@
>>   #include "hw/s390x/pv.h"
>>   #include "migration/blocker.h"
>>   #include "qapi/visitor.h"
>> +#include "hw/s390x/cpu-topology.h"
>>   
>>   static Error *pv_mig_blocker;
>>   
>> @@ -310,10 +311,18 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
>>   {
>>       MachineState *ms = MACHINE(hotplug_dev);
>>       S390CPU *cpu = S390_CPU(dev);
>> +    ERRP_GUARD();
>>   
>>       g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
>>       ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
>>   
>> +    if (s390_has_topology()) {
>> +        s390_topology_set_cpu(ms, cpu, errp);
>> +        if (*errp) {
>> +            return;
>> +        }
>> +    }
>> +
>>       if (dev->hotplugged) {
>>           raise_irq_cpu_hotplug();
>>       }
>> @@ -551,11 +560,21 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
>>                                     sizeof(CPUArchId) * max_cpus);
>>       ms->possible_cpus->len = max_cpus;
>>       for (i = 0; i < ms->possible_cpus->len; i++) {
>> +        CpuInstanceProperties *props = &ms->possible_cpus->cpus[i].props;
>> +
>>           ms->possible_cpus->cpus[i].type = ms->cpu_type;
>>           ms->possible_cpus->cpus[i].vcpus_count = 1;
>>           ms->possible_cpus->cpus[i].arch_id = i;
>> -        ms->possible_cpus->cpus[i].props.has_core_id = true;
>> -        ms->possible_cpus->cpus[i].props.core_id = i;
>> +
>> +        props->has_core_id = true;
>> +        props->core_id = i;
>> +        props->has_socket_id = true;
>> +        props->socket_id = i / ms->smp.cores;
>> +        props->has_book_id = true;
>> +        props->book_id = i / (ms->smp.cores * ms->smp.sockets);
>> +        props->has_drawer_id = true;
>> +        props->drawer_id = i /
>> +                           (ms->smp.cores * ms->smp.sockets * ms->smp.books);
> 
> You need to calculate the modulus like in s390_topology_cpu_default, right?

!!! yes of course, good catch, I forgot that.

Since there are two uses of this calculation, what about using inlines?
like:

static inline int s390_std_socket(int n, CpuTopology *smp)
{
     return (n / smp->cores) % smp->sockets;
}

static inline int s390_std_book(int n, CpuTopology *smp)
{
     return (n / (smp->cores * smp->sockets)) % smp->books;
}

static inline int s390_std_drawer(int n, CpuTopology *smp)
{
     return (n / (smp->cores * smp->sockets * smp->books)) % smp->books;
}


Thanks for the comments.

Regards,
Pierre

On Fri, 2023-02-03 at 10:21 +0100, Pierre Morel wrote:
> 
> On 2/2/23 17:42, Nina Schoetterl-Glausch wrote:
> > On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
> > > The topology information are attributes of the CPU and are
> > > specified during the CPU device creation.
> > > 
> > > On hot plug we:
> > > - calculate the default values for the topology for drawers,
> > >    books and sockets in the case they are not specified.
> > > - verify the CPU attributes
> > > - check that we have still room on the desired socket
> > > 
> > > The possibility to insert a CPU in a mask is dependent on the
> > > number of cores allowed in a socket, a book or a drawer, the
> > > checking is done during the hot plug of the CPU to have an
> > > immediate answer.
> > > 
> > > If the complete topology is not specified, the core is added
> > > in the physical topology based on its core ID and it gets
> > > defaults values for the modifier attributes.
> > > 
> > > This way, starting QEMU without specifying the topology can
> > > still get some advantage of the CPU topology.
> > > 
> > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > ---
> > >   include/hw/s390x/cpu-topology.h |  24 +++
> > >   hw/s390x/cpu-topology.c         | 256 ++++++++++++++++++++++++++++++++
> > >   hw/s390x/s390-virtio-ccw.c      |  23 ++-
> > >   hw/s390x/meson.build            |   1 +
> > >   4 files changed, 302 insertions(+), 2 deletions(-)
> > >   create mode 100644 hw/s390x/cpu-topology.c
> > > 
[...]
> > 
> > > +/**
> > > + * s390_set_core_in_socket:
> > > + * @cpu: the new S390CPU to insert in the topology structure
> > > + * @drawer_id: new drawer_id
> > > + * @book_id: new book_id
> > > + * @socket_id: new socket_id
> > > + * @creation: if is true the CPU is a new CPU and there is no old socket
> > > + *            to handle.
> > > + *            if is false, this is a moving the CPU and old socket count
> > > + *            must be decremented.
> > > + * @errp: the error pointer
> > > + *
> > > + */
> > > +static void s390_set_core_in_socket(S390CPU *cpu, int drawer_id, int book_id,
> > 
> > Maybe name it s390_(topology_)?add_core_to_socket instead.
> 
> OK, it is better
> 
> > 
> > > +                                    int socket_id, bool creation, Error **errp)
> > > +{
> > > +    int old_socket = s390_socket_nb(cpu);
> > > +    int new_socket;
> > > +
> > > +    if (creation) {
> > > +        new_socket = old_socket;
> > > +    } else {
> > 
> > You need parentheses here.
> > 
> > > +        new_socket = drawer_id * s390_topology.smp->books +
> >                         (
> > > +                     book_id * s390_topology.smp->sockets +
> >                                 )
> > > +                     socket_id;
> 
> If you prefer I can us parentheses.

It's necessary, otherwise the multiplication of book_id and smp->sockets takes precedence.
> 
> 
> > > +    }
> > > +
> > > +    /* Check for space on new socket */
> > > +    if ((new_socket != old_socket) &&
> > > +        (s390_topology.cores_per_socket[new_socket] >=
> > > +         s390_topology.smp->cores)) {
> > > +        error_setg(errp, "No more space on this socket");
> > > +        return;
> > > +    }
> > > +
> > > +    /* Update the count of cores in sockets */
> > > +    s390_topology.cores_per_socket[new_socket] += 1;
> > > +    if (!creation) {
> > > +        s390_topology.cores_per_socket[old_socket] -= 1;
> > > +    }
> > > +}
> > > 
[...]
> > 
> > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > > index f3cc845d3b..9bc51a83f4 100644
> > > --- a/hw/s390x/s390-virtio-ccw.c
> > > +++ b/hw/s390x/s390-virtio-ccw.c
> > > @@ -44,6 +44,7 @@
> > >   #include "hw/s390x/pv.h"
> > >   #include "migration/blocker.h"
> > >   #include "qapi/visitor.h"
> > > +#include "hw/s390x/cpu-topology.h"
> > >   
> > >   static Error *pv_mig_blocker;
> > >   
> > > @@ -310,10 +311,18 @@ static void s390_cpu_plug(HotplugHandler *hotplug_dev,
> > >   {
> > >       MachineState *ms = MACHINE(hotplug_dev);
> > >       S390CPU *cpu = S390_CPU(dev);
> > > +    ERRP_GUARD();
> > >   
> > >       g_assert(!ms->possible_cpus->cpus[cpu->env.core_id].cpu);
> > >       ms->possible_cpus->cpus[cpu->env.core_id].cpu = OBJECT(dev);
> > >   
> > > +    if (s390_has_topology()) {
> > > +        s390_topology_set_cpu(ms, cpu, errp);
> > > +        if (*errp) {
> > > +            return;
> > > +        }
> > > +    }
> > > +
> > >       if (dev->hotplugged) {
> > >           raise_irq_cpu_hotplug();
> > >       }
> > > @@ -551,11 +560,21 @@ static const CPUArchIdList *s390_possible_cpu_arch_ids(MachineState *ms)
> > >                                     sizeof(CPUArchId) * max_cpus);
> > >       ms->possible_cpus->len = max_cpus;
> > >       for (i = 0; i < ms->possible_cpus->len; i++) {
> > > +        CpuInstanceProperties *props = &ms->possible_cpus->cpus[i].props;
> > > +
> > >           ms->possible_cpus->cpus[i].type = ms->cpu_type;
> > >           ms->possible_cpus->cpus[i].vcpus_count = 1;
> > >           ms->possible_cpus->cpus[i].arch_id = i;
> > > -        ms->possible_cpus->cpus[i].props.has_core_id = true;
> > > -        ms->possible_cpus->cpus[i].props.core_id = i;
> > > +
> > > +        props->has_core_id = true;
> > > +        props->core_id = i;
> > > +        props->has_socket_id = true;
> > > +        props->socket_id = i / ms->smp.cores;
> > > +        props->has_book_id = true;
> > > +        props->book_id = i / (ms->smp.cores * ms->smp.sockets);
> > > +        props->has_drawer_id = true;
> > > +        props->drawer_id = i /
> > > +                           (ms->smp.cores * ms->smp.sockets * ms->smp.books);
> > 
> > You need to calculate the modulus like in s390_topology_cpu_default, right?
> 
> !!! yes of course, good catch, I forgot that.
> 
> Since there are two uses of this calculation, what about using inlines?
> like:

Fine by me, I just have no idea what std is short for.
Since this would be used in s390_topology_cpu_default and calculates the default assignment,
I would call it s390_topology_cpu_default_socket, book, etc. or similar

> 
> static inline int s390_std_socket(int n, CpuTopology *smp)
> {
>      return (n / smp->cores) % smp->sockets;
> }
> 
> static inline int s390_std_book(int n, CpuTopology *smp)
> {
>      return (n / (smp->cores * smp->sockets)) % smp->books;
> }
> 
> static inline int s390_std_drawer(int n, CpuTopology *smp)
> {
>      return (n / (smp->cores * smp->sockets * smp->books)) % smp->books;
> }
> 
> 
> Thanks for the comments.
> 
> Regards,
> Pierre
>

On 2/3/23 14:22, Nina Schoetterl-Glausch wrote:
> On Fri, 2023-02-03 at 10:21 +0100, Pierre Morel wrote:
>>
>> On 2/2/23 17:42, Nina Schoetterl-Glausch wrote:
>>> On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
>>>> The topology information are attributes of the CPU and are
>>>> specified during the CPU device creation.
>>>>
>>>> On hot plug we:
>>>> - calculate the default values for the topology for drawers,
>>>>     books and sockets in the case they are not specified.
>>>> - verify the CPU attributes
>>>> - check that we have still room on the desired socket
>>>>
>>>> The possibility to insert a CPU in a mask is dependent on the
>>>> number of cores allowed in a socket, a book or a drawer, the
>>>> checking is done during the hot plug of the CPU to have an
>>>> immediate answer.
>>>>
>>>> If the complete topology is not specified, the core is added
>>>> in the physical topology based on its core ID and it gets
>>>> defaults values for the modifier attributes.
>>>>
>>>> This way, starting QEMU without specifying the topology can
>>>> still get some advantage of the CPU topology.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>> ---
>>>>    include/hw/s390x/cpu-topology.h |  24 +++
>>>>    hw/s390x/cpu-topology.c         | 256 ++++++++++++++++++++++++++++++++
>>>>    hw/s390x/s390-virtio-ccw.c      |  23 ++-
>>>>    hw/s390x/meson.build            |   1 +
>>>>    4 files changed, 302 insertions(+), 2 deletions(-)
>>>>    create mode 100644 hw/s390x/cpu-topology.c
>>>>
> [...]
>>>
>>>> +/**
>>>> + * s390_set_core_in_socket:
>>>> + * @cpu: the new S390CPU to insert in the topology structure
>>>> + * @drawer_id: new drawer_id
>>>> + * @book_id: new book_id
>>>> + * @socket_id: new socket_id
>>>> + * @creation: if is true the CPU is a new CPU and there is no old socket
>>>> + *            to handle.
>>>> + *            if is false, this is a moving the CPU and old socket count
>>>> + *            must be decremented.
>>>> + * @errp: the error pointer
>>>> + *
>>>> + */
>>>> +static void s390_set_core_in_socket(S390CPU *cpu, int drawer_id, int book_id,
>>>
>>> Maybe name it s390_(topology_)?add_core_to_socket instead.
>>
>> OK, it is better
>>
>>>
>>>> +                                    int socket_id, bool creation, Error **errp)
>>>> +{
>>>> +    int old_socket = s390_socket_nb(cpu);
>>>> +    int new_socket;
>>>> +
>>>> +    if (creation) {
>>>> +        new_socket = old_socket;
>>>> +    } else {
>>>
>>> You need parentheses here.
>>>
>>>> +        new_socket = drawer_id * s390_topology.smp->books +
>>>                          (
>>>> +                     book_id * s390_topology.smp->sockets +
>>>                                  )
>>>> +                     socket_id;
>>
>> If you prefer I can us parentheses.
> 
> It's necessary, otherwise the multiplication of book_id and smp->sockets takes precedence.
>>
>>

Right, I did not understand where you want the parenthesis.
I think you mean:

         new_socket = (drawer_id * s390_topology.smp->books + book_id) *
                      s390_topology.smp->sockets + socket_id;

thanks,

Regards,
Pierre

On Fri, 2023-02-03 at 15:40 +0100, Pierre Morel wrote:
> 
> On 2/3/23 14:22, Nina Schoetterl-Glausch wrote:
> > On Fri, 2023-02-03 at 10:21 +0100, Pierre Morel wrote:
> > > 
> > > On 2/2/23 17:42, Nina Schoetterl-Glausch wrote:
> > > > On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote:
> > > > > The topology information are attributes of the CPU and are
> > > > > specified during the CPU device creation.
> > > > > 
> > > > > On hot plug we:
> > > > > - calculate the default values for the topology for drawers,
> > > > >     books and sockets in the case they are not specified.
> > > > > - verify the CPU attributes
> > > > > - check that we have still room on the desired socket
> > > > > 
> > > > > The possibility to insert a CPU in a mask is dependent on the
> > > > > number of cores allowed in a socket, a book or a drawer, the
> > > > > checking is done during the hot plug of the CPU to have an
> > > > > immediate answer.
> > > > > 
> > > > > If the complete topology is not specified, the core is added
> > > > > in the physical topology based on its core ID and it gets
> > > > > defaults values for the modifier attributes.
> > > > > 
> > > > > This way, starting QEMU without specifying the topology can
> > > > > still get some advantage of the CPU topology.
> > > > > 
> > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > > > > ---
> > > > >    include/hw/s390x/cpu-topology.h |  24 +++
> > > > >    hw/s390x/cpu-topology.c         | 256 ++++++++++++++++++++++++++++++++
> > > > >    hw/s390x/s390-virtio-ccw.c      |  23 ++-
> > > > >    hw/s390x/meson.build            |   1 +
> > > > >    4 files changed, 302 insertions(+), 2 deletions(-)
> > > > >    create mode 100644 hw/s390x/cpu-topology.c
> > > > > 
> > [...]
> > > > 
> > > > > +/**
> > > > > + * s390_set_core_in_socket:
> > > > > + * @cpu: the new S390CPU to insert in the topology structure
> > > > > + * @drawer_id: new drawer_id
> > > > > + * @book_id: new book_id
> > > > > + * @socket_id: new socket_id
> > > > > + * @creation: if is true the CPU is a new CPU and there is no old socket
> > > > > + *            to handle.
> > > > > + *            if is false, this is a moving the CPU and old socket count
> > > > > + *            must be decremented.
> > > > > + * @errp: the error pointer
> > > > > + *
> > > > > + */
> > > > > +static void s390_set_core_in_socket(S390CPU *cpu, int drawer_id, int book_id,
> > > > 
> > > > Maybe name it s390_(topology_)?add_core_to_socket instead.
> > > 
> > > OK, it is better
> > > 
> > > > 
> > > > > +                                    int socket_id, bool creation, Error **errp)
> > > > > +{
> > > > > +    int old_socket = s390_socket_nb(cpu);
> > > > > +    int new_socket;
> > > > > +
> > > > > +    if (creation) {
> > > > > +        new_socket = old_socket;
> > > > > +    } else {
> > > > 
> > > > You need parentheses here.
> > > > 
> > > > > +        new_socket = drawer_id * s390_topology.smp->books +
> > > >                          (
> > > > > +                     book_id * s390_topology.smp->sockets +
> > > >                                  )
> > > > > +                     socket_id;
> > > 
> > > If you prefer I can us parentheses.
> > 
> > It's necessary, otherwise the multiplication of book_id and smp->sockets takes precedence.
> > > 
> > > 
> 
> Right, I did not understand where you want the parenthesis.
> I think you mean:
> 
>          new_socket = (drawer_id * s390_topology.smp->books + book_id) *
>                       s390_topology.smp->sockets + socket_id;

Yes, exactly. I see how it was ambiguous.
> 
> thanks,
> 
> Regards,
> Pierre
>