Re: [PATCH v24 04/21] target/s390x/cpu topology: handle STSI(15) and build the SYSIB

[Thomas Huth <thuth@redhat.com>]

On 26/09/2023 14.15, Nina Schoetterl-Glausch wrote:
> From: Pierre Morel <pmorel@linux.ibm.com>
> 
> 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>
> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
...
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 97b0af2795..350c7ea8aa 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -15,10 +15,33 @@
>   #include "hw/boards.h"
>   #include "qapi/qapi-types-machine-target.h"
>   
> +#define S390_TOPOLOGY_CPU_IFL   0x03
> +
> +typedef struct s390_topology_id {
> +    uint8_t sentinel;
> +    uint8_t drawer;
> +    uint8_t book;
> +    uint8_t socket;
> +    uint8_t type;
> +    uint8_t vertical:1;
> +    uint8_t entitlement:2;
> +    uint8_t dedicated;
> +    uint8_t origin;
> +} s390_topology_id;

Sorry for not noticing this earlier, but: Please adapt the name of the 
struct to the QEMU coding conventions, i.e. S390TopologyId instead of 
s390_topology_id.

...
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index dfcc1aa1fc..c1ba5c46d6 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -571,6 +571,29 @@ typedef struct SysIB_322 {
...
> +/*
> + * 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.

Please write Container with a small "c" at the beginning.

...
> diff --git a/target/s390x/kvm/stsi-topology.c b/target/s390x/kvm/stsi-topology.c
> new file mode 100644
> index 0000000000..67ecc67184
> --- /dev/null
> +++ b/target/s390x/kvm/stsi-topology.c
> @@ -0,0 +1,339 @@
...
> +/**
> + * s390_topology_fill_list_sorted:
> + * @topology_list: list to fill
> + *
> + * Loop over all CPU and insert it at the right place

I'd like to suggest to change that to:

"Loop over all CPUs and insert each of the at the right place"

> + * inside the TLE entry list.
> + * Fill the S390Topology list with entries according to the order
> + * specified by the PoP.
> + */
> +static void s390_topology_fill_list_sorted(S390TopologyList *topology_list)
> +{
> +    CPUState *cs;
> +    S390TopologyEntry sentinel = { .id.sentinel = 1 };
> +
> +    QTAILQ_INIT(topology_list);
> +
> +    QTAILQ_INSERT_HEAD(topology_list, &sentinel, next);
> +
> +    CPU_FOREACH(cs) {
> +        s390_topology_id id = s390_topology_from_cpu(S390_CPU(cs));
> +        S390TopologyEntry *entry = NULL, *tmp;
> +
> +        QTAILQ_FOREACH(tmp, topology_list, next) {
> +            if (s390_topology_id_eq(&id, &tmp->id)) {
> +                entry = tmp;
> +                break;
> +            } else if (s390_topology_id_lt(&id, &tmp->id)) {
> +                entry = g_malloc0(sizeof(*entry));
> +                entry->id = id;
> +                QTAILQ_INSERT_BEFORE(tmp, entry, next);
> +                break;
> +            }
> +        }
> +        assert(entry);
> +        s390_topology_add_cpu_to_entry(entry, S390_CPU(cs));
> +    }
> +
> +    QTAILQ_REMOVE(topology_list, &sentinel, next);
> +}
> +
> +/**
> + * s390_topology_empty_list:
> + *
> + * Clear all entries in the S390Topology list.
> + */
> +static void s390_topology_empty_list(S390TopologyList *topology_list)
> +{
> +    S390TopologyEntry *entry = NULL;
> +    S390TopologyEntry *tmp = NULL;
> +
> +    QTAILQ_FOREACH_SAFE(entry, topology_list, next, tmp) {
> +        QTAILQ_REMOVE(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
> + * @ra: the return address
> + *
> + * 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, uint64_t addr, uint8_t ar, uintptr_t ra)
> +{
> +    S390TopologyList topology_list;
> +    SysIB sysib = {0};
> +    int length;
> +
> +    if (!s390_has_topology() || sel2 < 2 || sel2 > SCLP_READ_SCP_INFO_MNEST) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    s390_topology_fill_list_sorted(&topology_list);
> +
> +    length = setup_stsi(&topology_list, &sysib.sysib_151x, sel2);
> +
> +    if (!length) {
> +        s390_topology_empty_list(&topology_list);
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    sysib.sysib_151x.length = cpu_to_be16(length);
> +    if (!s390_cpu_virt_mem_write(cpu, addr, ar, &sysib, length)) {
> +        setcc(cpu, 0);
> +    } else {
> +        s390_cpu_virt_mem_handle_exc(cpu, ra);
> +    }
> +
> +    s390_topology_empty_list(&topology_list);

In case this will ever be used with TCG: s390_cpu_virt_mem_handle_exc() 
might not return, so the clean-up has to be done before calling that function:

     ret = s390_cpu_virt_mem_write(cpu, addr, ar, &sysib, length);
     s390_topology_empty_list(&topology_list);
     if (!ret) {
          setcc(cpu, 0);
     } else {
         s390_cpu_virt_mem_handle_exc(cpu, ra);
     }
}

Or maybe even do it before the "if (!length)" statement, so you don't need 
it in the body of that statement?

  Thomas