Re: [PATCH v15 03/11] target/s390x/cpu topology: handle STSI(15) and build the SYSIB

[Nina Schoetterl-Glausch]

On Wed, 2023-02-01 at 14:20 +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@linux.ibm.com>
> > ---
> >  include/hw/s390x/cpu-topology.h |  22 +++
> >  include/hw/s390x/sclp.h         |   1 +
> >  target/s390x/cpu.h              |  72 +++++++
> >  hw/s390x/cpu-topology.c         |  10 +
> >  target/s390x/kvm/cpu_topology.c | 335 ++++++++++++++++++++++++++++++++
> >  target/s390x/kvm/kvm.c          |   5 +-
> >  target/s390x/kvm/meson.build    |   3 +-
> >  7 files changed, 446 insertions(+), 2 deletions(-)
> >  create mode 100644 target/s390x/kvm/cpu_topology.c
> > 
[...]
> > 
> > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> > index d654267a71..e1f6925856 100644
> > --- a/target/s390x/cpu.h
> > +++ b/target/s390x/cpu.h
[...]
> > +
> > +/* CPU type Topology List Entry */
> > +typedef struct SysIBTl_cpu {
> > +        uint8_t nl;
> > +        uint8_t reserved0[3];
> > +#define SYSIB_TLE_POLARITY_MASK 0x03
> > +#define SYSIB_TLE_DEDICATED     0x04
> > +        uint8_t entitlement;

I would just call this flags, since it's multiple fields.

> > +        uint8_t type;
> > +        uint16_t origin;
> > +        uint64_t mask;
> > +} QEMU_PACKED QEMU_ALIGNED(8) SysIBTl_cpu;
> > +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
> > 
> > +
> > 
[...]
> >  /**
> > diff --git a/target/s390x/kvm/cpu_topology.c 
> > b/target/s390x/kvm/cpu_topology.c
> > new file mode 100644
> > index 0000000000..aba141fb66
> > --- /dev/null
> > +++ b/target/s390x/kvm/cpu_topology.c
> > 
[...]
> > +
> > +/*
> > + * Macro to check that the size of data after increment
> > + * will not get bigger than the size of the SysIB.
> > + */
> > +#define SYSIB_GUARD(data, x) do {       \
> > +        data += x;                      \
> > +        if (data  > sizeof(SysIB)) {    \
> > +            return -ENOSPC;             \

I would go with ENOMEM here.

> > +        }                               \
> > +    } while (0)
> > +
> > +/**
> > + * stsi_set_tle:
> > + * @p: A pointer to the position of the first TLE
> > + * @level: The nested level wanted by the guest
> > + *
> > + * Loop inside the s390_topology.list until the sentinelle entry

s/sentinelle/sentinel/

> > + * is found and for each entry:
> > + *   - Check using SYSIB_GUARD() that the size of the SysIB is not
> > + *     reached.
> > + *   - Add all the container TLE needed for the level
> > + *   - Add the CPU TLE.

I'd focus more on *what* the function does instead of *how*.

Fill the SYSIB with the topology information as described in the PoP,
nesting containers as appropriate, with the maximum nesting limited by @level.

Or something similar.

> > + *
> > + * Return value:
> > + * s390_top_set_level returns the size of the SysIB_15x after being

You forgot to rename the function here, right?
How about stsi_fill_topology_sysib or stsi_topology_fill_sysib, instead?

> > + * filled with TLE on success.
> > + * It returns -ENOSPC in the case we would overrun the end of the SysIB.

You would have to change to ENOMEM here than also.

> > + */
> > +static int stsi_set_tle(char *p, int level)
> > +{
> > +    S390TopologyEntry *entry;
> > +    int last_drawer = -1;
> > +    int last_book = -1;
> > +    int last_socket = -1;
> > +    int drawer_id = 0;
> > +    int book_id = 0;
> > +    int socket_id = 0;
> > +    int n = sizeof(SysIB_151x);
> > +
> > +    QTAILQ_FOREACH(entry, &s390_topology.list, next) {
> > +        int current_drawer = entry->id.drawer;
> > +        int current_book = entry->id.book;
> > +        int current_socket = entry->id.socket;

This only saves two characters, so you could just use entry->id. ...

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

... but keep it if it would make this line too long.
You could also rename entry, to current or cur, if you want to emphasize that.

> > +
> > +        /* If we reach the guard get out */
> > +        if (entry->id.level5) {
> > +            break;
> > +        }
> > +
> > +        if (level > 3 && drawer_change) {
> > +            SYSIB_GUARD(n, sizeof(SysIBTl_container));
> > +            p = fill_container(p, 3, drawer_id++);
> > +            book_id = 0;
> > +        }
> > +        if (level > 2 && book_change) {
> > +            SYSIB_GUARD(n, sizeof(SysIBTl_container));
> > +            p = fill_container(p, 2, book_id++);
> > +            socket_id = 0;
> > +        }
> > +        if (socket_change) {
> > +            SYSIB_GUARD(n, sizeof(SysIBTl_container));
> > +            p = fill_container(p, 1, socket_id++);
> > +        }
> > +
> > +        SYSIB_GUARD(n, sizeof(SysIBTl_cpu));
> > +        p = fill_tle_cpu(p, entry);
> > +        last_drawer = entry->id.drawer;
> > +        last_book = entry->id.book;
> > +        last_socket = entry->id.socket;
> > +    }
> > +
> > +    return n;
> > +}
> > +
> > +/**
> > + * setup_stsi:
> > + * sysib: pointer to a SysIB to be filled with SysIB_151x data
> > + * level: Nested level specified by the guest
> > + *
> > + * Setup the SysIB_151x header before calling stsi_set_tle with
> > + * a pointer to the first TLE entry.

Same thing here with regards to describing the what.

Setup the SYSIB for STSI 15.1, the header as well as the description
of the topology.

> > + */
> > +static int setup_stsi(SysIB_151x *sysib, int level)
> > +{
> > +    sysib->mnest = level;
> > +    switch (level) {
> > +    case 4:
> > +        sysib->mag[S390_TOPOLOGY_MAG4] = current_machine->smp.drawers;
> > +        sysib->mag[S390_TOPOLOGY_MAG3] = current_machine->smp.books;
> > +        sysib->mag[S390_TOPOLOGY_MAG2] = current_machine->smp.sockets;
> > +        sysib->mag[S390_TOPOLOGY_MAG1] = current_machine->smp.cores;
> > +        break;
> > +    case 3:
> > +        sysib->mag[S390_TOPOLOGY_MAG3] = current_machine->smp.drawers *
> > +                                         current_machine->smp.books;
> > +        sysib->mag[S390_TOPOLOGY_MAG2] = current_machine->smp.sockets;
> > +        sysib->mag[S390_TOPOLOGY_MAG1] = current_machine->smp.cores;
> > +        break;
> > +    case 2:
> > +        sysib->mag[S390_TOPOLOGY_MAG2] = current_machine->smp.drawers *
> > +                                         current_machine->smp.books *
> > +                                         current_machine->smp.sockets;
> > +        sysib->mag[S390_TOPOLOGY_MAG1] = current_machine->smp.cores;
> > +        break;
> > +    }
> > +
> > +    return stsi_set_tle(sysib->tle, level);
> > +}
> > +
> > +/**
> > + * s390_topology_add_cpu_to_entry:
> > + * @entry: Topology entry to setup
> > + * @cpu: the S390CPU to add
> > + *
> > + * Set the core bit inside the topology mask and
> > + * increments the number of cores for the socket.
> > + */
> > +static void s390_topology_add_cpu_to_entry(S390TopologyEntry *entry,
> > +                                           S390CPU *cpu)
> > +{
> > +    set_bit(63 - (cpu->env.core_id % 64), &entry->mask);
> > +}
> > +
> > +/**
> > + * s390_topology_new_entry:
> > + * @id: s390_topology_id to add
> > + * @cpu: the S390CPU to add
> > + *
> > + * Allocate a new entry and initialize it.
> > + *
> > + * returns the newly allocated entry.
> > + */
> > +static S390TopologyEntry *s390_topology_new_entry(s390_topology_id id,
> > +                                                  S390CPU *cpu)

This is used only once, right?
I think I'd go ahead and inline it into s390_topology_insert, since I had
to go back and check if new_entry calls add_cpu when reading 
s390_topology_insert.

> > +{
> > +    S390TopologyEntry *entry;
> > +
> > +    entry = g_malloc0(sizeof(S390TopologyEntry));
> > +    entry->id.id = id.id;
> > +    s390_topology_add_cpu_to_entry(entry, cpu);
> > +
> > +    return entry;
> > +}
> > +
> > +/**
> > + * s390_topology_from_cpu:
> > + * @cpu: The S390CPU
> > + *
> > + * Initialize the topology id from the CPU environment.
> > + */
> > +static s390_topology_id s390_topology_from_cpu(S390CPU *cpu)
> > +{
> > +    s390_topology_id topology_id = {0};
> > +
> > +    topology_id.drawer = cpu->env.drawer_id;
> > +    topology_id.book = cpu->env.book_id;
> > +    topology_id.socket = cpu->env.socket_id;
> > +    topology_id.origin = cpu->env.core_id / 64;
> > +    topology_id.type = S390_TOPOLOGY_CPU_IFL;
> > +    topology_id.dedicated = cpu->env.dedicated;
> > +
> > +    if (s390_topology.polarity == POLARITY_VERTICAL) {
> > +        /*
> > +         * Vertical polarity with dedicated CPU implies
> > +         * vertical high entitlement.
> > +         */
> > +        if (topology_id.dedicated) {
> > +            topology_id.polarity |= POLARITY_VERTICAL_HIGH;
> > +        } else {
> > +            topology_id.polarity |= cpu->env.entitlement;
> > +        }
> > +    }
> > +
> > +    return topology_id;
> > +}
> > +
> > +/**
> > + * s390_topology_insert:
> > + * @cpu: s390CPU insert.
> > + *
> > + * Parse the topology list to find if the entry already
> > + * exist and add the core in it.
> > + * If it does not exist, allocate a new entry and insert
> > + * it in the queue from lower id to greater id.
> > + */
> > +static void s390_topology_insert(S390CPU *cpu)
> > +{
> > +    s390_topology_id id = s390_topology_from_cpu(cpu);
> > +    S390TopologyEntry *entry = NULL;
> > +    S390TopologyEntry *tmp = NULL;
> > +
> > +    QTAILQ_FOREACH(tmp, &s390_topology.list, next) {
> > +        if (id.id == tmp->id.id) {
> > +            s390_topology_add_cpu_to_entry(tmp, cpu);
> > +            return;
> > +        } else if (id.id < tmp->id.id) {
> > +            entry = s390_topology_new_entry(id, cpu);
> > +            QTAILQ_INSERT_BEFORE(tmp, entry, next);
> > +            return;
> > +        }
> > +    }
> > +}
> > +
> > +/**
> > + * s390_order_tle:
> > + *
> > + * Loop over all CPU and insert it at the right place
> > + * inside the TLE entry list.
> > + */

Suggestion:

s390_topology_fill_list_sorted

Fill the S390Topology list with entries according to the order specified
by the PoP.

> > +static void s390_order_tle(void)
> > +{
> > +    CPUState *cs;
> > +
> > +    CPU_FOREACH(cs) {
> > +        s390_topology_insert(S390_CPU(cs));
> > +    }
> > +}
> > +
> > +/**
> > + * s390_free_tle:
> > + *
> > + * Loop over all TLE entries and free them.
> > + * Keep the sentinelle which is the only one with level5 != 0

s/sentinelle/sentinel/

> > + */

Suggestion:
s390_topology_empty_list

Clear all entries in the S390Topology list except the sentinel.

> > +static void s390_free_tle(void)
> > +{
> > +    S390TopologyEntry *entry = NULL;
> > +    S390TopologyEntry *tmp = NULL;
> > +
> > +    QTAILQ_FOREACH_SAFE(entry, &s390_topology.list, next, tmp) {
> > +        if (!entry->id.level5) {
> > +            QTAILQ_REMOVE(&s390_topology.list, entry, next);
> > +            g_free(entry);
> > +        }
> > +    }
> > +}
> > +
> > +/**
> > + * insert_stsi_15_1_x:
> > + * cpu: the CPU doing the call for which we set CC
> > + * sel2: the selector 2, containing the nested level
> > + * addr: Guest logical address of the guest SysIB
> > + * ar: the access register number
> > + *
> > + * Reserve a zeroed SysIB, let setup_stsi to fill it and
> > + * copy the SysIB to the guest memory.
> > + *
> > + * In case of overflow set CC(3) and no copy is done.

Suggestion:

Emulate STSI 15.1.x, that is, perform all necessary checks and fill the SYSIB.
In case the topology description is too long to fit into the SYSIB,
set CC=3 and abort without writing the SYSIB.
 
> > + */
> > +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> > +{
> > +    SysIB sysib = {0};
> > +    int len;
> > +
> > +    if (!s390_has_topology() || sel2 < 2 || sel2 > 
> > SCLP_READ_SCP_INFO_MNEST) {
> > +        setcc(cpu, 3);
> > +        return;
> > +    }
> > +
> > +    s390_order_tle();
> > +
> > +    len = setup_stsi(&sysib.sysib_151x, sel2);
> > +
> > +    if (len < 0) {

I stumbled a bit over this, maybe rename len to r.

> > +        setcc(cpu, 3);
> > +        return;
> > +    }
> > +
> > +    sysib.sysib_151x.length = cpu_to_be16(len);
> > +    s390_cpu_virt_mem_write(cpu, addr, ar, &sysib, len);
> > +    setcc(cpu, 0);
> > +
> > +    s390_free_tle();
> > +}
> > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> > index 3ac7ec9acf..5ea358cbb0 100644
> > --- a/target/s390x/kvm/kvm.c
> > +++ b/target/s390x/kvm/kvm.c
> > @@ -1919,9 +1919,12 @@ static int handle_stsi(S390CPU *cpu)
> >          if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) {
> >              return 0;
> >          }
> > -        /* Only sysib 3.2.2 needs post-handling for now. */
> >          insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar);
> >          return 0;
> > +    case 15:
> > +        insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr,
> > +                           run->s390_stsi.ar);
> > +        return 0;
> >      default:
> >          return 0;
> >      }
> > diff --git a/target/s390x/kvm/meson.build b/target/s390x/kvm/meson.build
> > index aef52b6686..5daa5c6033 100644
> > --- a/target/s390x/kvm/meson.build
> > +++ b/target/s390x/kvm/meson.build
> > @@ -1,6 +1,7 @@
> >  
> >  s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
> > -  'kvm.c'
> > +  'kvm.c',
> > +  'cpu_topology.c'
> >  ), if_false: files(
> >    'stubs.c'
> >  ))