Re: [PATCH v9 02/10] s390x/cpu topology: core_id sets s390x CPU topology

[Pierre Morel]

On 9/6/22 07:58, Nico Boehr wrote:
> Quoting Pierre Morel (2022-09-02 09:55:23)
> [...]
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > new file mode 100644
> > index 0000000000..a6ca006ec5
> > --- /dev/null
> > +++ b/hw/s390x/cpu-topology.c
> [...]
> > +void s390_topology_new_cpu(int core_id)
> > +{
> [...]
> > +    socket_id = core_id / topo->cores;
>
> The comment below is essential for understanding all of this. Move it before 
> this line.

OK

> > +
> > +    bit = core_id;
> > +    origin = bit / 64;
> > +    bit %= 64;
> > +    bit = 63 - bit;
> > +
> > +    /*
> > +     * At the core level, each CPU is represented by a bit in a 64bit
> > +     * unsigned long. Set on plug and clear on unplug of a CPU.
>
> cleared                                  ^
>
> [...]
> > +     * In that case the origin field, representing the offset of the first CPU
> > +     * in the CPU container allows to represent up to the maximal number of
> > +     * CPU inside several CPU containers inside the socket container.
>
> How about:
> "In that case the origin variable represents the offset of the first CPU in the
> CPU container. More than 64 CPUs per socket are represented in several CPU
> containers inside the socket container."

Yes, better, thanks I take it


> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index b5ca154e2f..15cefd104b 100644
> [...]
> > @@ -247,6 +248,12 @@ static void ccw_init(MachineState *machine)
> >       /* init memory + setup max page size. Required for the CPU model */
> >       s390_memory_init(machine->ram);
> >   
> > +    /* Adding the topology must be done before CPU intialization*/
>
> space                                                              ^

Yes thanks

regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen