Quoting Pierre Morel (2022-09-02 09:55:24)
> 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.
I like this. It is so much simpler. Thanks.
[...]
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index a6ca006ec5..e2fd5c7e44 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -76,9 +76,11 @@ void s390_topology_new_cpu(int core_id)
> * in the CPU container allows to represent up to the maximal number of
> * CPU inside several CPU containers inside the socket container.
> */
> + qemu_mutex_lock(&topo->topo_mutex);
You access topo->cores above. Do you need the mutex for that? I guess not since
it can't change at runtime (right?), so maybe it is worth documenting what the
topo_mutex actually protects or you just take the mutex at the start of the
function.
[...]
> diff --git a/target/s390x/cpu_topology.c b/target/s390x/cpu_topology.c
> new file mode 100644
> index 0000000000..56865dafc6
> --- /dev/null
> +++ b/target/s390x/cpu_topology.c
[...]
> +static char *fill_tle_cpu(char *p, uint64_t mask, int origin)
> +{
> + SysIBTl_cpu *tle = (SysIBTl_cpu *)p;
> +
> + tle->nl = 0;
> + tle->dedicated = 1;
> + tle->polarity = S390_TOPOLOGY_POLARITY_H;
> + tle->type = S390_TOPOLOGY_CPU_TYPE;
> + tle->origin = origin * 64;
origin would also need a byte order conversion.
> + tle->mask = be64_to_cpu(mask);
cpu_to_be64()
[...]
> +static char *s390_top_set_level2(S390Topology *topo, char *p)
> +{
> + int i, origin;
> +
> + for (i = 0; i < topo->sockets; i++) {
> + if (!topo->socket[i].active_count) {
> + continue;
> + }
> + p = fill_container(p, 1, i);
> + for (origin = 0; origin < S390_TOPOLOGY_MAX_ORIGIN; origin++) {
> + uint64_t mask = 0L;
> +
> + mask = be64_to_cpu(topo->tle[i].mask[origin]);
Don't you already do the endianness conversion in fill_tle_cpu()?
[...]
> +void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar)
> +{
> + SysIB_151x *sysib;
> + int len = sizeof(*sysib);
> +
> + if (s390_is_pv() || sel2 < 2 || sel2 > S390_TOPOLOGY_MAX_MNEST) {
> + setcc(cpu, 3);
> + return;
> + }
> +
> + sysib = g_malloc0(TARGET_PAGE_SIZE);
> +
> + len += setup_stsi(sysib, sel2);
> + if (len > TARGET_PAGE_SIZE) {
> + setcc(cpu, 3);
> + goto out_free;
> + }
Maybe I don't get it, but isn't it kind of late for this check? You would
already have written beyond the end of the buffer at this point in time...
> +
> + sysib->length = be16_to_cpu(len);
cpu_to_be16()
