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@xxxxxxxxxxxxx>
> ---
> 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@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/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()
