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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx>
> +
> + * 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',
