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@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 > > > [...] > > > > > +/** > > > + * 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 >