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

[Nina Schoetterl-Glausch]

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
>