Re: [PATCH v8 03/12] s390x/cpu_topology: implementating Store Topology System Information

[Janis Schoetterl-Glausch]

On 6/20/22 16:03, Pierre Morel wrote:
> The handling of STSI is enhanced with the interception of the
> function code 15 for storing CPU topology.
> 
> Using the objects built during the plugging of CPU, we build the
> SYSIB 15_1_x structures.
> 
> With this patch the maximum MNEST level is 2, this is also
> the only level allowed and only SYSIB 15_1_2 will be built.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  target/s390x/cpu.h          |   2 +
>  target/s390x/cpu_topology.c | 112 ++++++++++++++++++++++++++++++++++++
>  target/s390x/kvm/kvm.c      |   5 ++
>  target/s390x/meson.build    |   1 +
>  4 files changed, 120 insertions(+)
>  create mode 100644 target/s390x/cpu_topology.c
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 216adfde26..9d48087b71 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -890,4 +890,6 @@ S390CPU *s390_cpu_addr2state(uint16_t cpu_addr);
>  
>  #include "exec/cpu-all.h"
>  
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar);
> +
>  #endif
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> new file mode 100644
> index 0000000000..9f656d7e51
> --- /dev/null
> +++ b/target/s390x/cpu_topology.c
> @@ -0,0 +1,112 @@
> +/*
> + * QEMU S390x CPU Topology
> + *
> + * Copyright IBM Corp. 2022
> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
> + *
> + * 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/cpu-topology.h"
> +
> +static int stsi_15_container(void *p, int nl, int id)
> +{
> +    SysIBTl_container *tle = (SysIBTl_container *)p;
> +
> +    tle->nl = nl;
> +    tle->id = id;
> +
> +    return sizeof(*tle);
> +}
> +
> +static int stsi_15_cpus(void *p, S390TopologyCores *cd)
> +{
> +    SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
> +
> +    tle->nl = 0;
> +    tle->dedicated = cd->dedicated;
> +    tle->polarity = cd->polarity;
> +    tle->type = cd->cputype;
> +    tle->origin = be16_to_cpu(cd->origin);
> +    tle->mask = be64_to_cpu(cd->mask);
> +
> +    return sizeof(*tle);
> +}
> +
> +static int set_socket(const MachineState *ms, void *p,
> +                      S390TopologySocket *socket)
> +{
> +    BusChild *kid;
> +    int l, len = 0;
> +
> +    len += stsi_15_container(p, 1, socket->socket_id);
> +    p += len;
> +

You could put a comment here, TODO: different cpu types, polarizations not 
supported,
or similar, since those require a specific order.
 
> +    QTAILQ_FOREACH_REVERSE(kid, &socket->bus->children, sibling) {

Is there no synchronization/RCU read section necessary to guard against a 
concurrent hotplug?
Since the children are ordered by creation, not core_id, the order of the 
entries is incorrect.
Ditto for the other equivalent loops.

> +        l = stsi_15_cpus(p, S390_TOPOLOGY_CORES(kid->child));
> +        p += l;
> +        len += l;
> +    }
> +    return len;
> +}
> +
> +static void setup_stsi(const MachineState *ms, void *p, int level)

I don't love the name of this function, it's not very descriptive. 
fill_sysib_15_1_x ?
Why don't you pass a SysIB_151x* instead of a void*?

> +{
> +    S390TopologyBook *book;
> +    SysIB_151x *sysib;
> +    BusChild *kid;
> +    int len, l;
> +
> +    sysib = (SysIB_151x *)p;
> +    sysib->mnest = level;
> +    sysib->mag[TOPOLOGY_NR_MAG2] = ms->smp.sockets;
> +    sysib->mag[TOPOLOGY_NR_MAG1] = ms->smp.cores * ms->smp.threads;

If I understood STSI right, it doesn't care about threads, so there should not 
be a multiplication here.
> +
> +    book = s390_get_topology();
> +    len = sizeof(SysIB_151x);
> +    p += len;
> +
> +    QTAILQ_FOREACH_REVERSE(kid, &book->bus->children, sibling) {
> +        l = set_socket(ms, p, S390_TOPOLOGY_SOCKET(kid->child));
> +        p += l;

I'm uncomfortable with advancing the pointer without a check if the page is 
being overflowed.
With lots of cpus in lots of sockets and a deep hierarchy the topology list can 
get quite long.

> +        len += l;> +    }
> +
> +    sysib->length = be16_to_cpu(len);
> +}
> +
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> +    const MachineState *machine = MACHINE(qdev_get_machine());
> +    void *p;
> +    int ret;
> +
> +    /*
> +     * Until the SCLP STSI Facility reporting the MNEST value is used,
> +     * a sel2 value of 2 is the only value allowed in STSI 15.1.x.
> +     */

Do you actually implement the SCLP functionality in this series? You're changing
this check in subsequent patches, but I only see the definition of a new 
constant,
not that you're presenting it to the guest.

> +    if (sel2 != 2) {
> +        setcc(cpu, 3);
> +        return;
> +    }
> +
> +    p = g_malloc0(TARGET_PAGE_SIZE);

Any reason not to stack allocate the sysib?
> +
> +    setup_stsi(machine, p, 2);
> +
> +    if (s390_is_pv()) {
> +        ret = s390_cpu_pv_mem_write(cpu, 0, p, TARGET_PAGE_SIZE);
> +    } else {
> +        ret = s390_cpu_virt_mem_write(cpu, addr, ar, p, TARGET_PAGE_SIZE);
> +    }

Since we're allowed to not store the reserved space after the sysib, it seems 
more natural
to do so. I don't know if it makes any difference performance wise, but it 
doesn't harm.
> +
> +    setcc(cpu, ret ? 3 : 0);

Shouldn't this result in an exception instead? Not sure if you should call
s390_cpu_virt_mem_handle_exc thereafter.

> +    g_free(p);
> +}
> +
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 7bd8db0e7b..563bf5ac60 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -51,6 +51,7 @@
>  #include "hw/s390x/s390-virtio-ccw.h"
>  #include "hw/s390x/s390-virtio-hcall.h"
>  #include "hw/s390x/pv.h"
> +#include "hw/s390x/cpu-topology.h"
>  
>  #ifndef DEBUG_KVM
>  #define DEBUG_KVM  0
> @@ -1918,6 +1919,10 @@ static int handle_stsi(S390CPU *cpu)
>          /* 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/meson.build b/target/s390x/meson.build
> index 84c1402a6a..890ccfa789 100644
> --- a/target/s390x/meson.build
> +++ b/target/s390x/meson.build
> @@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files(
>    'sigp.c',
>    'cpu-sysemu.c',
>    'cpu_models_sysemu.c',
> +  'cpu_topology.c',
>  ))
>  
>  s390x_user_ss = ss.source_set()