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

[Pierre Morel <pmorel@linux.ibm.com>]

On 9/6/22 13:49, Janis Schoetterl-Glausch wrote:
> On Fri, 2022-09-02 at 09:55 +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>
>> ---
>>   hw/s390x/cpu-topology.c         |   4 ++
>>   include/hw/s390x/cpu-topology.h |   5 ++
>>   target/s390x/cpu.h              |  49 +++++++++++++++
>>   target/s390x/cpu_topology.c     | 108 ++++++++++++++++++++++++++++++++
>>   target/s390x/kvm/kvm.c          |   6 +-
>>   target/s390x/meson.build        |   1 +
>>   6 files changed, 172 insertions(+), 1 deletion(-)
>>   create mode 100644 target/s390x/cpu_topology.c
>>
>> 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 could use a reader writer lock for this, if qemu has that (I didn't
> find any tho).

I can use RCU but we could also consider that the write path is very short.

> 
>>       topo->socket[socket_id].active_count++;
>>       topo->tle[socket_id].active_count++;
>>       set_bit(bit, &topo->tle[socket_id].mask[origin]);
>> +    qemu_mutex_unlock(&topo->topo_mutex);
>>   }
>>   
>>   /**
>> @@ -104,6 +106,8 @@ static void s390_topology_realize(DeviceState *dev, Error **errp)
>>       n = topo->sockets;
>>       topo->socket = g_malloc0(n * sizeof(S390TopoContainer));
>>       topo->tle = g_malloc0(topo->tles * sizeof(S390TopoTLE));
>> +
>> +    qemu_mutex_init(&topo->topo_mutex);
>>   }
>>   
>>   /**
>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>> index 6911f975f4..0b7f3d10b2 100644
>> --- a/include/hw/s390x/cpu-topology.h
>> +++ b/include/hw/s390x/cpu-topology.h
>> @@ -10,6 +10,10 @@
>>   #ifndef HW_S390X_CPU_TOPOLOGY_H
>>   #define HW_S390X_CPU_TOPOLOGY_H
>>   
>> +#define S390_TOPOLOGY_CPU_TYPE    0x03
> 
> IMO you should add the name of cpu type 0x03 to the name of the
> constant, even if there is only one right now.
> You did the same for the polarity after all.

OK right
#define S390_TOPOLOGY_CPU_IFL 0x03

> 
>> +
>> +#define S390_TOPOLOGY_POLARITY_H  0x00
>> +
>>   typedef struct S390TopoContainer {
>>       int active_count;
>>   } S390TopoContainer;
>> @@ -30,6 +34,7 @@ struct S390Topology {
>>       int tles;
>>       S390TopoContainer *socket;
>>       S390TopoTLE *tle;
>> +    QemuMutex topo_mutex;
>>   };
>>   typedef struct S390Topology S390Topology;
>>   
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 7d6d01325b..c61fe9b563 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -565,6 +565,53 @@ typedef union SysIB {
>>   } SysIB;
>>   QEMU_BUILD_BUG_ON(sizeof(SysIB) != 4096);
>>   
>> +/* CPU type Topology List Entry */
>> +typedef struct SysIBTl_cpu {
>> +        uint8_t nl;
>> +        uint8_t reserved0[3];
>> +        uint8_t reserved1:5;
>> +        uint8_t dedicated:1;
>> +        uint8_t polarity:2;
>> +        uint8_t type;
>> +        uint16_t origin;
>> +        uint64_t mask;
>> +} SysIBTl_cpu;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16);
>> +
>> +/* Container type Topology List Entry */
>> +typedef struct SysIBTl_container {
>> +        uint8_t nl;
>> +        uint8_t reserved[6];
>> +        uint8_t id;
>> +} QEMU_PACKED SysIBTl_container;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIBTl_container) != 8);
>> +
>> +/* Generic Topology List Entry */
>> +typedef union SysIBTl_entry {
>> +        uint8_t nl;
>> +        SysIBTl_container container;
>> +        SysIBTl_cpu cpu;
>> +} SysIBTl_entry;
> 
> This isn't used for anything but the declaration in SysIB_151x, is it?

right.

>> +
>> +#define TOPOLOGY_NR_MAG  6
>> +#define TOPOLOGY_NR_MAG6 0
>> +#define TOPOLOGY_NR_MAG5 1
>> +#define TOPOLOGY_NR_MAG4 2
>> +#define TOPOLOGY_NR_MAG3 3
>> +#define TOPOLOGY_NR_MAG2 4
>> +#define TOPOLOGY_NR_MAG1 5
>> +/* Configuration topology */
>> +typedef struct SysIB_151x {
>> +    uint8_t  reserved0[2];
>> +    uint16_t length;
>> +    uint8_t  mag[TOPOLOGY_NR_MAG];
>> +    uint8_t  reserved1;
>> +    uint8_t  mnest;
>> +    uint32_t reserved2;
>> +    SysIBTl_entry tle[0];
> 
> I would just use uint64_t[0] as type or uint64_t[] whichever is qemu
> style.

OK



> 
>> +} SysIB_151x;
>> +QEMU_BUILD_BUG_ON(sizeof(SysIB_151x) != 16);
>> +
>>   /* MMU defines */
>>   #define ASCE_ORIGIN           (~0xfffULL) /* segment table origin             */
>>   #define ASCE_SUBSPACE         0x200       /* subspace group control           */
>> @@ -843,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..56865dafc6
>> --- /dev/null
>> +++ b/target/s390x/cpu_topology.c
>> @@ -0,0 +1,108 @@
>> +/*
>> + * 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"
>> +#include "hw/s390x/sclp.h"
>> +
>> +static char *fill_container(char *p, int level, int id)
>> +{
>> +    SysIBTl_container *tle = (SysIBTl_container *)p;
>> +
>> +    tle->nl = level;
>> +    tle->id = id;
>> +    return p + sizeof(*tle);
>> +}
>> +
>> +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;
>> +    tle->mask = be64_to_cpu(mask);
> 
> You convert endianess for mask here...
> 
>> +    return p + sizeof(*tle);
>> +}
>> +
>> +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]);
> 
> ...and here. So one has to go, I guess this one.
> Also using cpu_to_be64 seems more intuitive to me.

cpu_to_be64 is the right thing to do.
Also yes, I suppress this one.

> 
>> +            if (mask) {
>> +                p = fill_tle_cpu(p, mask, origin);
>> +            }
>> +        }
>> +    }
>> +    return p;
>> +}
>> +
>> +static int setup_stsi(SysIB_151x *sysib, int level)
>> +{
>> +    S390Topology *topo = s390_get_topology();
>> +    char *p = (char *)sysib->tle;
>> +
>> +    qemu_mutex_lock(&topo->topo_mutex);
>> +
>> +    sysib->mnest = level;
>> +    switch (level) {
>> +    case 2:
>> +        sysib->mag[TOPOLOGY_NR_MAG2] = topo->sockets;
>> +        sysib->mag[TOPOLOGY_NR_MAG1] = topo->cores;
>> +        p = s390_top_set_level2(topo, p);
>> +        break;
>> +    }
>> +
>> +    qemu_mutex_unlock(&topo->topo_mutex);
>> +
>> +    return p - (char *)sysib->tle;
>> +}
>> +
>> +#define S390_TOPOLOGY_MAX_MNEST 2
>> +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);
> 
> What made you decide against stack allocating this?

I just did not think about it.
I will change it.

> 
>> +
>> +    len += setup_stsi(sysib, sel2);
>> +    if (len > TARGET_PAGE_SIZE) {
> 
> If you do the check here it's too late.

yes.

> 
>> +        setcc(cpu, 3);
>> +        goto out_free;
>> +    }
>> +
>> +    sysib->length = be16_to_cpu(len);
>> +    s390_cpu_virt_mem_write(cpu, addr, ar, sysib, len);
> 
> If the return value of this is <0 it's an error condition.
> If you ignore the value we'll keep running.

I understood that exceptions are handled inside 
s390_cpu_virt_mem_write() and we have no CC to report.

> 
>> +    setcc(cpu, 0);
> 
> Is it correct to set the cc value even if s390_cpu_virt_mem_write
> causes an exception?

I guess that if it caused an exception we do not get so far.
Am I wrong?

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen