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

[Pierre Morel <pmorel@linux.ibm.com>]

On 2/3/23 18:36, Nina Schoetterl-Glausch wrote:
> 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.

OK

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

Considering your comment of the length in insert_stsi_15_1_x(), I think 
the best would be to return 0.
Because:
- We do not need to report any error reason.
- The value will be forwarded to insert_stsi_15_1_x() as the length of 
the SYSIB to write
- On error we do not write the SYSIB (len = 0)
- In normal case the return value is always non zero and positive.

> 
>>> +        }                               \
>>> +    } 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/

OK, thx,

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

Right.

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

Thanks, looks good to me .

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


OK with stsi_topology_fill_sysib()

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

As discussed above, it seems to me that return 0 is even better.

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

OK

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

it is OK

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


OK, thx

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

OK

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

OK

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

yes, thx

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

OK, better thanks.

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

Why ? it is the length used to fill the length field of the SYSIB.

May be it would be clearer if we give back the length to write and 0 on 
error then we have here:

	if (!len) {
		setcc(cpu, 3);
		return;
	}

> 
>>> +        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();
>>> +}

Thanks for the comments.
If there are only comments and cosmetic changes will I get your RB if I 
change them according to your wishes?

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen