Re: [PATCH v10 2/9] s390x/cpu topology: reporting the CPU topology to the guest

Janis Schoetterl-Glausch
2022-10-27T20:43:20Z

On Wed, 2022-10-12 at 18:21 +0200, Pierre Morel wrote:
> The guest can use the STSI instruction to get a buffer filled
> with the CPU topology description.
>
> Let us implement the STSI instruction for the basis CPU topology
> level, level 2.
>
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
> include/hw/s390x/cpu-topology.h | 3 +
> target/s390x/cpu.h | 48 ++++++++++++++
> hw/s390x/cpu-topology.c | 8 ++-
> target/s390x/cpu_topology.c | 109 ++++++++++++++++++++++++++++++++
> target/s390x/kvm/kvm.c | 6 +-
> target/s390x/meson.build | 1 +
> 6 files changed, 172 insertions(+), 3 deletions(-)
> create mode 100644 target/s390x/cpu_topology.c
>
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index 66c171d0bc..61c11db017 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -13,6 +13,8 @@
> #include "hw/qdev-core.h"
> #include "qom/object.h"
>
> +#define S390_TOPOLOGY_POLARITY_H 0x00
> +
> typedef struct S390TopoContainer {
> int active_count;
> } S390TopoContainer;
> @@ -29,6 +31,7 @@ struct S390Topology {
> S390TopoContainer *socket;
> S390TopoTLE *tle;
> MachineState *ms;
> + QemuMutex topo_mutex;
> };
>
> #define TYPE_S390_CPU_TOPOLOGY "s390-topology"
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..d604aa9c78 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
>
[...]
> +
> +/* Maxi size of a SYSIB structure is when all CPU are alone in a container */

Max or Maximum.

> +#define S390_TOPOLOGY_SYSIB_SIZE (sizeof(SysIB_151x) + \
> + S390_MAX_CPUS * (sizeof(SysIBTl_container) + \
> + sizeof(SysIBTl_cpu)))

Currently this is 16+248*3*8 == 5968 and will grow with books, drawer support to
16+248*5*8 == 9936 ...

[...]
>
> +
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> + uint64_t page[S390_TOPOLOGY_SYSIB_SIZE / sizeof(uint64_t)] = {};

... so calling this page is a bit misleading. Also why not make it a char[]?
And maybe use a union for type punning.

> + SysIB_151x *sysib = (SysIB_151x *) page;
> + int len;
> +
> + if (s390_is_pv() || !s390_has_topology() ||
> + sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> + setcc(cpu, 3);
> + return;
> + }
> +
> + len = setup_stsi(sysib, sel2);

This should now be memory safe, but might be larger than 4k,
the maximum size of the SYSIB. I guess you want to set cc code 3
in this case and return.
> +
> + sysib->length = cpu_to_be16(len);
> + s390_cpu_virt_mem_write(cpu, addr, ar, sysib, len);
> + setcc(cpu, 0);
> +}
> +
> …