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
