Re: [PATCH v9 03/10] s390x/cpu topology: reporting the CPU topology to the guest

[Nico Boehr]

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()