Re: [PATCH v8 03/12] s390x/cpu_topology: implementating Store Topology System Information

[Pierre Morel]

On 7/20/22 21:34, Janis Schoetterl-Glausch wrote:
> 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@linux.ibm.com>
> > ---
> >   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@linux.ibm.com>
> > + *
> > + * 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.

I prefer to put that during the creation process as here there is no 
control but just fill the SysIB with the data.
>   
> > +    QTAILQ_FOREACH_REVERSE(kid, &socket->bus->children, sibling) {
>
> Is there no synchronization/RCU read section necessary to guard against a 
> concurrent hotplug?

definitively.
I think there must be a synchronization point around the all topology 
creation and the all SysIB creation

> Since the children are ordered by creation, not core_id, the order of the 
> entries is incorrect.
> Ditto for the other equivalent loops.

right will change this during creation

> > +        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 ?

OK and I will add some s390_ prefix too to make the function easier to find.

> Why don't you pass a SysIB_151x* instead of a void*?

OK

> > +{
> > +    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.

Right now that we have threads == 1 being forced

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

right but I better check on creation I guess, because here I have no way 
to tell that the topology is not available.

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

!!! exact I lost the
s390x-SCLP-reporting-the-maximum-nested-topology-.patch
during transition from v6 to v7 !!

strange, must be a rebase error.

> > +    if (sel2 != 2) {
> > +        setcc(cpu, 3);
> > +        return;
> > +    }
> > +
> > +    p = g_malloc0(TARGET_PAGE_SIZE);
>
> Any reason not to stack allocate the sysib?

No this can be done

> > +
> > +    setup_stsi(machine, p, 2);
> > +
> > +    if (s390_is_pv()) {
> > +        ret = s390_cpu_pv_mem_write(cpu, 0, p, TARGET_PAGE_SIZE);

This goes away for now, STSI(15,x,x) is not supported by PV


> > +    } 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.

right

> > +
> > +    setcc(cpu, ret ? 3 : 0);
>
> Shouldn't this result in an exception instead? Not sure if you should call

right,

> s390_cpu_virt_mem_handle_exc thereafter.

I think it is for TCG only but it should not arm.

Thanks,

regards,
Pierre

--
Pierre Morel
IBM Lab Boeblingen