| Message ID | 20220902075531.188916-4-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
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()
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). > 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. > + > +#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? > + > +#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. > +} 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. > + 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? > + > + len += setup_stsi(sysib, sel2); > + if (len > TARGET_PAGE_SIZE) { If you do the check here it's too late. > + 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. > + setcc(cpu, 0); Is it correct to set the cc value even if s390_cpu_virt_mem_write causes an exception? > + > +out_free: > + g_free(sysib); > +} > + > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c > index 6a8dbadf7e..f96630440b 100644 > --- a/target/s390x/kvm/kvm.c > +++ b/target/s390x/kvm/kvm.c > @@ -51,6 +51,7 @@ > #include "hw/s390x/s390-virtio-ccw.h" > #include "hw/s390x/s390-virtio-hcall.h" > #include "hw/s390x/pv.h" > +#include "hw/s390x/cpu-topology.h" > > #ifndef DEBUG_KVM > #define DEBUG_KVM 0 > @@ -1917,9 +1918,12 @@ static int handle_stsi(S390CPU *cpu) > if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) { > return 0; > } > - /* Only sysib 3.2.2 needs post-handling for now. */ > insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar); > return 0; > + case 15: > + insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr, > + run->s390_stsi.ar); > + return 0; > default: > return 0; > } > diff --git a/target/s390x/meson.build b/target/s390x/meson.build > index 84c1402a6a..890ccfa789 100644 > --- a/target/s390x/meson.build > +++ b/target/s390x/meson.build > @@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files( > 'sigp.c', > 'cpu-sysemu.c', > 'cpu_models_sysemu.c', > + 'cpu_topology.c', > )) > > s390x_user_ss = ss.source_set()
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/target/s390x/cpu_topology.c 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 is a multibyte field too, so needs a conversion too. > + tle->mask = be64_to_cpu(mask); > + return p + sizeof(*tle); > +} > + [...]
On 9/7/22 12:26, 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/target/s390x/cpu_topology.c 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 is a multibyte field too, so needs a conversion too. right, Thanks, Pierre > >> + tle->mask = be64_to_cpu(mask); >> + return p + sizeof(*tle); >> +} >> + > [...]
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
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
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); 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 + +#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; + +#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]; +} 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); + 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]); + 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); + + len += setup_stsi(sysib, sel2); + if (len > TARGET_PAGE_SIZE) { + setcc(cpu, 3); + goto out_free; + } + + sysib->length = be16_to_cpu(len); + s390_cpu_virt_mem_write(cpu, addr, ar, sysib, len); + setcc(cpu, 0); + +out_free: + g_free(sysib); +} + diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c index 6a8dbadf7e..f96630440b 100644 --- a/target/s390x/kvm/kvm.c +++ b/target/s390x/kvm/kvm.c @@ -51,6 +51,7 @@ #include "hw/s390x/s390-virtio-ccw.h" #include "hw/s390x/s390-virtio-hcall.h" #include "hw/s390x/pv.h" +#include "hw/s390x/cpu-topology.h" #ifndef DEBUG_KVM #define DEBUG_KVM 0 @@ -1917,9 +1918,12 @@ static int handle_stsi(S390CPU *cpu) if (run->s390_stsi.sel1 != 2 || run->s390_stsi.sel2 != 2) { return 0; } - /* Only sysib 3.2.2 needs post-handling for now. */ insert_stsi_3_2_2(cpu, run->s390_stsi.addr, run->s390_stsi.ar); return 0; + case 15: + insert_stsi_15_1_x(cpu, run->s390_stsi.sel2, run->s390_stsi.addr, + run->s390_stsi.ar); + return 0; default: return 0; } diff --git a/target/s390x/meson.build b/target/s390x/meson.build index 84c1402a6a..890ccfa789 100644 --- a/target/s390x/meson.build +++ b/target/s390x/meson.build @@ -29,6 +29,7 @@ s390x_softmmu_ss.add(files( 'sigp.c', 'cpu-sysemu.c', 'cpu_models_sysemu.c', + 'cpu_topology.c', )) s390x_user_ss = ss.source_set()
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