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

[Pierre Morel]

On 9/6/22 10:17, Nico Boehr wrote:
> 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.

You are right one should always do that.
I will add this.

> [...]
> > 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.

yes

> > +    tle->mask = be64_to_cpu(mask);
>
> cpu_to_be64()

yes

> [...]
> > +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()?

yes

> [...]
> > +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...

it is


Thanks for your comments.

regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen