diff mbox series

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

Message ID 20220902075531.188916-6-pmorel@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390x: CPU Topology | expand

Commit Message

Pierre Morel Sept. 2, 2022, 7:55 a.m. UTC
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(-)

Comments

Janis Schoetterl-Glausch Sept. 7, 2022, 10:36 a.m. UTC | #1
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.
>  
>      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.

> +    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.

> -    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?

>          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.

> +    }
> +    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?

> +        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.

>  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);
>  }
> -
Pierre Morel Sept. 28, 2022, 8:55 a.m. UTC | #2
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
diff mbox series

Patch

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
@@ -46,7 +46,7 @@  S390Topology *s390_get_topology(void)
 void s390_topology_new_cpu(int core_id)
 {
     S390Topology *topo = s390_get_topology();
-    int socket_id;
+    int socket_id, book_id, drawer_id;
     int bit, origin;
 
     /* In the case no Topology is used nothing is to be done here */
@@ -55,6 +55,8 @@  void s390_topology_new_cpu(int core_id)
     }
 
     socket_id = core_id / topo->cores;
+    book_id = socket_id / topo->sockets;
+    drawer_id = book_id / topo->books;
 
     bit = core_id;
     origin = bit / 64;
@@ -77,6 +79,8 @@  void s390_topology_new_cpu(int core_id)
      * CPU inside several CPU containers inside the socket container.
      */
     qemu_mutex_lock(&topo->topo_mutex);
+    topo->drawer[drawer_id].active_count++;
+    topo->book[book_id].active_count++;
     topo->socket[socket_id].active_count++;
     topo->tle[socket_id].active_count++;
     set_bit(bit, &topo->tle[socket_id].mask[origin]);
@@ -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));
 
     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;
+    int drawers;
+    int books;
     int sockets;
     int cores;
-    int tles;
+    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)
 {
-    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);
         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);
+    }
+    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);
+        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
 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);
 }
-