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

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

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
>