On Thu, 2023-01-05 at 15:53 +0100, Pierre Morel wrote:
> On interception of STSI(15.1.x) the System Information Block
> (SYSIB) is built from the list of pre-ordered topology entries.
>
> Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> ---
> include/hw/s390x/cpu-topology.h | 3 +
> include/hw/s390x/sclp.h | 1 +
> target/s390x/cpu.h | 78 ++++++++++++++++++
> target/s390x/kvm/cpu_topology.c | 136 ++++++++++++++++++++++++++++++++
> target/s390x/kvm/kvm.c | 5 +-
> target/s390x/kvm/meson.build | 3 +-
> 6 files changed, 224 insertions(+), 2 deletions(-)
> create mode 100644 target/s390x/kvm/cpu_topology.c
>
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index b3fd752d8d..9571aa70e5 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -41,6 +41,9 @@ typedef union s390_topology_id {
> };
> } s390_topology_id;
> #define TOPO_CPU_MASK 0x000000000000003fUL
> +#define TOPO_SOCKET_MASK 0x0000ffffff000000UL
> +#define TOPO_BOOK_MASK 0x0000ffff00000000UL
> +#define TOPO_DRAWER_MASK 0x0000ff0000000000UL
>
> typedef struct S390TopologyEntry {
> s390_topology_id id;
> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> index d3ade40a5a..712fd68123 100644
> --- a/include/hw/s390x/sclp.h
> +++ b/include/hw/s390x/sclp.h
> @@ -112,6 +112,7 @@ typedef struct CPUEntry {
> } QEMU_PACKED CPUEntry;
>
> #define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET 128
> +#define SCLP_READ_SCP_INFO_MNEST 2
> typedef struct ReadInfo {
> SCCBHeader h;
> uint16_t rnmax;
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 39ea63a416..78988048dd 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -561,6 +561,25 @@ typedef struct SysIB_322 {
> } SysIB_322;
> QEMU_BUILD_BUG_ON(sizeof(SysIB_322) != 4096);
>
> +#define S390_TOPOLOGY_MAG 6
> +#define S390_TOPOLOGY_MAG6 0
> +#define S390_TOPOLOGY_MAG5 1
> +#define S390_TOPOLOGY_MAG4 2
> +#define S390_TOPOLOGY_MAG3 3
> +#define S390_TOPOLOGY_MAG2 4
> +#define S390_TOPOLOGY_MAG1 5
> +/* Configuration topology */
> +typedef struct SysIB_151x {
> + uint8_t reserved0[2];
> + uint16_t length;
> + uint8_t mag[S390_TOPOLOGY_MAG];
> + uint8_t reserved1;
> + uint8_t mnest;
> + uint32_t reserved2;
> + char tle[];
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIB_151x;
> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
> +
> typedef union SysIB {
> SysIB_111 sysib_111;
> SysIB_121 sysib_121;
> @@ -568,9 +587,68 @@ typedef union SysIB {
> SysIB_221 sysib_221;
> SysIB_222 sysib_222;
> SysIB_322 sysib_322;
> + SysIB_151x sysib_151x;
> } SysIB;
> QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>
> +/*
> + * CPU Topology List provided by STSI with fc=15 provides a list
> + * of two different Topology List Entries (TLE) types to specify
> + * the topology hierarchy.
> + *
> + * - Container Topology List Entry
> + * Defines a container to contain other Topology List Entries
> + * of any type, nested containers or CPU.
> + * - CPU Topology List Entry
> + * Specifies the CPUs position, type, entitlement and polarization
> + * of the CPUs contained in the last Container TLE.
> + *
> + * There can be theoretically up to five levels of containers, QEMU
> + * uses only one level, the socket level.
> + *
> + * A container of with a nesting level (NL) greater than 1 can only
> + * contain another container of nesting level NL-1.
> + *
> + * A container of nesting level 1 (socket), contains as many CPU TLE
> + * as needed to describe the position and qualities of all CPUs inside
> + * the container.
> + * The qualities of a CPU are polarization, entitlement and type.
> + *
> + * The CPU TLE defines the position of the CPUs of identical qualities
> + * using a 64bits mask which first bit has its offset defined by
> + * the CPU address orgin field of the CPU TLE like in:
> + * CPU address = origin * 64 + bit position within the mask
> + *
> + */
> +/* Container type Topology List Entry */
> +/* Container type Topology List Entry */
> +typedef struct SysIBTl_container {
> + uint8_t nl;
> + uint8_t reserved[6];
> + uint8_t id;
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_container;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
> +
> +/* CPU type Topology List Entry */
> +typedef struct SysIBTl_cpu {
> + uint8_t nl;
> + uint8_t reserved0[3];
> + uint8_t reserved1:5;
> + uint8_t dedicated:1;
> + uint8_t polarity:2;
> + uint8_t type;
> + uint16_t origin;
> + uint64_t mask;
> +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_cpu;
> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
> +
> +/* Max size of a SYSIB structure is when all CPU are alone in a container */
> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) + \
> + S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
> + sizeof(SysIBTl_cpu)))
I don't think this is accurate anymore, if you have drawers and books.
In that case you could have 3 containers per 1 cpu.
You could also use the maxcpus number at runtime instead of S390_MAX_CPUS.
I also think you could do sizeof(SysIB) + sizeof(SysIBTl_cpu) if you check
if the sysib overflows 4k while building it.
> +
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
> +
> /* MMU defines */
> #define ASCE_ORIGIN (~0xfffULL) /* segment table origin */
> #define ASCE_SUBSPACE 0x200 /* subspace group control */
> diff --git a/target/s390x/kvm/cpu_topology.c b/target/s390x/kvm/cpu_topology.c
> new file mode 100644
> index 0000000000..3831a3264c
> --- /dev/null
> +++ b/target/s390x/kvm/cpu_topology.c
> @@ -0,0 +1,136 @@
> +/*
> + * QEMU S390x CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * Author(s): Pierre Morel <pmorel@xxxxxxxxxxxxx>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "hw/s390x/pv.h"
> +#include "hw/sysbus.h"
> +#include "hw/s390x/sclp.h"
> +#include "hw/s390x/cpu-topology.h"
> +
> +static char *fill_container(char *p, int level, int id)
> +{
> + SysIBTl_container *tle = (SysIBTl_container *)p;
> +
> + tle->nl = level;
> + tle->id = id;
> + return p + sizeof(*tle);
> +}
> +
> +static char *fill_tle_cpu(char *p, S390TopologyEntry *entry)
> +{
> + SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
> + s390_topology_id topology_id = entry->id;
> +
> + tle->nl = 0;
> + tle->dedicated = topology_id.d;
> + tle->polarity = topology_id.p;
> + tle->type = topology_id.type;
> + tle->origin = topology_id.origin;
You need to multiply that value by 64, no?
And convert it to BE.
> + tle->mask = cpu_to_be64(entry->mask);
> + return p + sizeof(*tle);
> +}
> +
> +static char *s390_top_set_level(char *p, int level)
> +{
> + S390TopologyEntry *entry;
> + uint64_t last_socket = -1UL;
> + uint64_t last_book = -1UL;
> + uint64_t last_drawer = -1UL;
-1UL looks funny to me, but there is nothing wrong with it.
But I don't see a reason not to use int and initialize it with -1.
> + int drawer_cnt = 0;
> + int book_cnt = 0;
> + int socket_cnt = 0;
> +
> + QTAILQ_FOREACH(entry, &s390_topology.list, next) {
> +
> + if (level > 3 && (last_drawer != entry->id.drawer)) {
> + book_cnt = 0;
> + socket_cnt = 0;
> + p = fill_container(p, 3, drawer_cnt++);
> + last_drawer = entry->id.id & TOPO_DRAWER_MASK;
> + p = fill_container(p, 2, book_cnt++);
> + last_book = entry->id.id & TOPO_BOOK_MASK;
> + p = fill_container(p, 1, socket_cnt++);
> + last_socket = entry->id.id & TOPO_SOCKET_MASK;
> + p = fill_tle_cpu(p, entry);
> + } else if (level > 2 && (last_book !=
> + (entry->id.id & TOPO_BOOK_MASK))) {
> + socket_cnt = 0;
> + p = fill_container(p, 2, book_cnt++);
> + last_book = entry->id.id & TOPO_BOOK_MASK;
> + p = fill_container(p, 1, socket_cnt++);
> + last_socket = entry->id.id & TOPO_SOCKET_MASK;
> + p = fill_tle_cpu(p, entry);
> + } else if (last_socket != (entry->id.id & TOPO_SOCKET_MASK)) {
> + p = fill_container(p, 1, socket_cnt++);
> + last_socket = entry->id.id & TOPO_SOCKET_MASK;
> + p = fill_tle_cpu(p, entry);
> + } else {
> + p = fill_tle_cpu(p, entry);
> + }
> + }
> +
> + return p;
> +}
I think you can do this a bit more readable and reduce redundancy.
Pseudo code:
foreach entry:
bool drawer_change = last_drawer != current_drawer
bool book_change = drawer_change || last_book != current_book
bool socket_change = book_change || last_socket != current_socket
if (level > 3 && drawer_change)
reset book id
fill drawer container
drawer id++
if (level > 2 && book_change)
reset socket id
fill book container
book id++
if (socket_change)
fill socket container
socket id++
fill cpu entry
update last_drawer, _book, _socket
You can also check after after every fill if the buffer has been overflowed,
that is if the function wrote more than sizeof(SysIB) - sizeof(SysIB_151x) bytes.
Or you check it once at the end if you increase the size of the buffer a bit.
Then you don't need to allocate the absolute maximum.
I think you could also use global ids for the containers.
So directly use the drawer id from the entry,
use (drawer id * smp.books) + book id, and so on.
If you update last_* after setting *_changed you don't need to maintain ids,
you can just use last_*.
[...]
