Re: [PATCH v9 05/10] s390x/cpu: reporting drawers and books topology to the guest

[Pierre Morel]

On 9/7/22 12:36, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +0200, Pierre Morel wrote:
> > The guest can ask for a topology report on drawer's or book's
> > level.
> > Let's implement the STSI instruction's handling for the corresponding
> > selector values.
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   hw/s390x/cpu-topology.c         | 19 +++++++---
> >   hw/s390x/s390-virtio-ccw.c      |  2 ++
> >   include/hw/s390x/cpu-topology.h |  7 +++-
> >   target/s390x/cpu_topology.c     | 64 +++++++++++++++++++++++++++------
> >   4 files changed, 76 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > index e2fd5c7e44..bb9ae63483 100644
> > --- a/hw/s390x/cpu-topology.c
> > +++ b/hw/s390x/cpu-topology.c
> [...]
>
> > @@ -99,13 +103,20 @@ static void s390_topology_realize(DeviceState *dev, Error 
> > **errp)
> >       S390Topology *topo = S390_CPU_TOPOLOGY(dev);
> >       int n;
> >   
> > +    topo->drawers = ms->smp.drawers;
> > +    topo->books = ms->smp.books;
> > +    topo->total_books = topo->books * topo->drawers;
> >       topo->sockets = ms->smp.sockets;
> > +    topo->total_sockets = topo->sockets * topo->books * topo->drawers;
> >       topo->cores = ms->smp.cores;
> > -    topo->tles = ms->smp.max_cpus;
> >   
> > -    n = topo->sockets;
> > +    n = topo->drawers;
> > +    topo->drawer = g_malloc0(n * sizeof(S390TopoContainer));
> > +    n *= topo->books;
> > +    topo->book = g_malloc0(n * sizeof(S390TopoContainer));
> > +    n *= topo->sockets;
> >       topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
> > -    topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
> > +    topo->tle = g_malloc0(n * sizeof(S390TopoTLE));
>
> Same question here about using g_new0.

yes, g_new0 is better here

> >   
> >       qemu_mutex_init(&topo->topo_mutex);
> >   }
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index 15cefd104b..3f28e28d47 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -626,6 +626,8 @@ static void ccw_machine_class_init(ObjectClass *oc, void 
> > *data)
> >       hc->unplug_request = s390_machine_device_unplug_request;
> >       nc->nmi_monitor_handler = s390_nmi;
> >       mc->default_ram_id = "s390.ram";
> > +    mc->smp_props.books_supported = true;
> > +    mc->smp_props.drawers_supported = true;
> >   }
> >   
> >   static inline bool machine_get_aes_key_wrap(Object *obj, Error **errp)
> > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> > index 0b7f3d10b2..4f8ac39ca0 100644
> > --- a/include/hw/s390x/cpu-topology.h
> > +++ b/include/hw/s390x/cpu-topology.h
> > @@ -29,9 +29,14 @@ typedef struct S390TopoTLE {
> >   
> >   struct S390Topology {
> >       SysBusDevice parent_obj;
> > +    int total_books;
> > +    int total_sockets;
>
> What are these used for? I'm not seeing anything.
>
> > +    int drawers;
> > +    int books;
> >       int sockets;
> >       int cores;
> > -    int tles;
>
> You remove this in this patch and you didn't really need it before.
> As far as I can tell it was just used for calculating the number of
> tles to allocate and you could use a local variable instead.
> So I would get rid of it in the patch that introduced it.

yes


> > +    S390TopoContainer *drawer;
> > +    S390TopoContainer *book;
> >       S390TopoContainer *socket;
> >       S390TopoTLE *tle;
> >       QemuMutex topo_mutex;
> > diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> > index 56865dafc6..305fbb9734 100644
> > --- a/target/s390x/cpu_topology.c
> > +++ b/target/s390x/cpu_topology.c
> > @@ -37,19 +37,18 @@ static char *fill_tle_cpu(char *p, uint64_t mask, int 
> > origin)
> >       return p + sizeof(*tle);
> >   }
> >   
> > -static char *s390_top_set_level2(S390Topology *topo, char *p)
> > +static char *s390_top_set_level2(S390Topology *topo, char *p, int fs, int ns)
> >   {
>
> I wouldn't hate more verbose names for fs and ns. start_socket,
> num_socket maybe? Same for fb, nb, but it is your call, it's not really
> hard to understand the code.

I prefer to keep the names short but I will have a second thought.

> > -    int i, origin;
> > +    int socket, origin;
> > +    uint64_t mask;
> >   
> > -    for (i = 0; i < topo->sockets; i++) {
> > -        if (!topo->socket[i].active_count) {
> > +    for (socket = fs; socket < fs + ns; socket++) {
> > +        if (!topo->socket[socket].active_count) {
> >               continue;
> >           }
> > -        p = fill_container(p, 1, i);
> > +        p = fill_container(p, 1, socket);
>
> Have you considered using an enum for the level constants?

did not but yes it would be better.

> >           for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
> > -            uint64_t mask = 0L;
> > -
> > -            mask = be64_to_cpu(topo->tle[i].mask[origin]);
> > +            mask = be64_to_cpu(topo->tle[socket].mask[origin]);
> >               if (mask) {
> >                   p = fill_tle_cpu(p, mask, origin);
> >               }
> > @@ -58,19 +57,63 @@ static char *s390_top_set_level2(S390Topology *topo, char 
> > *p)
> >       return p;
> >   }
> >   
> > +static char *s390_top_set_level3(S390Topology *topo, char *p, int fb, int nb)
> > +{
> > +    int book, fs = 0;
> > +
> > +    for (book = fb; book < fb + nb; book++, fs += topo->sockets) {
> > +        if (!topo->book[book].active_count) {
> > +            continue;
> > +        }
> > +        p = fill_container(p, 2, book);
> > +    p = s390_top_set_level2(topo, p, fs, topo->sockets);
>
> Indent is off.

thx

> > +    }
> > +    return p;
> > +}
> > +
> > +static char *s390_top_set_level4(S390Topology *topo, char *p)
> > +{
> > +    int drawer, fb = 0;
> > +
> > +    for (drawer = 0; drawer < topo->drawers; drawer++, fb += topo->books) {
> > +        if (!topo->drawer[drawer].active_count) {
> > +            continue;
> > +        }
> > +        p = fill_container(p, 3, drawer);
> > +        p = s390_top_set_level3(topo, p, fb, topo->books);
> > +    }
> > +    return p;
> > +}
> > +
> >   static int setup_stsi(SysIB_151x *sysib, int level)
> >   {
> >       S390Topology *topo = s390_get_topology();
> >       char *p = (char *)sysib->tle;
> > +    int max_containers;
> >   
> >       qemu_mutex_lock(&topo->topo_mutex);
> >   
> >       sysib->mnest = level;
> >       switch (level) {
> >       case 2:
> > +        max_containers = topo->sockets * topo->books * topo->drawers;
> > +        sysib->mag[TOPOLOGY_NR_MAG2] = max_containers;
> > +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
> > +        p = s390_top_set_level2(topo, p, 0, max_containers);
>
> Isn't this logic change already required for the patch that introduced
> stsi 15.1.2 handling?

yes, thx

> > +        break;
> > +    case 3:
> > +        max_containers = topo->books * topo->drawers;
> > +        sysib->mag[TOPOLOGY_NR_MAG3] = max_containers;
> >           sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
> >           sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
> > -        p = s390_top_set_level2(topo, p);
> > +        p = s390_top_set_level3(topo, p, 0, max_containers);
> > +        break;
> > +    case 4:
> > +        sysib->mag[TOPOLOGY_NR_MAG4] = topo->drawers;
> > +        sysib->mag[TOPOLOGY_NR_MAG3] = topo->books;
> > +        sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
> > +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
> > +        p = s390_top_set_level4(topo, p);
> >           break;
> >       }
> >   
> > @@ -79,7 +122,7 @@ static int setup_stsi(SysIB_151x *sysib, int level)
> >       return p - (char *)sysib->tle;
> >   }
> >   
> > -#define S390_TOPOLOGY_MAX_MNEST 2
> > +#define S390_TOPOLOGY_MAX_MNEST 4
>
> AFAIK you're only allowed to increase this if the maximum mnest
> facility is installed. If it isn't, only level 2 is supported.
> Which would mean that this patch doesn't do anything.

AFAIU this is model dependant but I have to rework the MNEST entry of 
the SYSIB anyway.

> >   void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> >   {
> >       SysIB_151x *sysib;
> > @@ -105,4 +148,3 @@ void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, 
> > uint8_t ar)
> >   out_free:
> >       g_free(sysib);
> >   }
> > -

Thanks for the comments

Regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen