Hello,
On 5/3/23 11:12, Thomas Huth wrote:
On 28/04/2023 14.35, Pierre Morel wrote:
On 4/27/23 15:38, Thomas Huth wrote:
On 25/04/2023 18.14, Pierre Morel wrote:
The topology information are attributes of the CPU and are
specified during the CPU device creation.
On hot plug we:
- calculate the default values for the topology for drawers,
books and sockets in the case they are not specified.
- verify the CPU attributes
- check that we have still room on the desired socket
The possibility to insert a CPU in a mask is dependent on the
number of cores allowed in a socket, a book or a drawer, the
checking is done during the hot plug of the CPU to have an
immediate answer.
If the complete topology is not specified, the core is added
in the physical topology based on its core ID and it gets
defaults values for the modifier attributes.
This way, starting QEMU without specifying the topology can
still get some advantage of the CPU topology.
Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
---
...
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
new file mode 100644
index 0000000000..471e0e7292
--- /dev/null
+++ b/hw/s390x/cpu-topology.c
@@ -0,0 +1,259 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * CPU Topology
Since you later introduce a file with almost the same name in the target/s390x/ folder, it would be fine to have some more explanation here what this file is all about (especially with regards to the other file in target/s390x/).
I first did put the interceptions in target/s390/ then moved them in target/s390x/kvm because it is KVM related then again only let STSI interception.
But to be honest I do not see any reason why not put everything in hw/s390x/ if CPU topology is implemented for TCG I think the code will call insert_stsi_15_1_x() too.
no?
Oh well, it's all so borderline ... whether you rather think of this as part of the CPU (like the STSI instruction) or rather part of the machine (drawers, books, ...).
I don't mind too much, as long as we don't have two files around with almost the same name (apart from "_" vs. "-"). So either keep the stsi part in target/s390x and use a better file name for that, or put everything together in one "cpu-topology.c" file.
Or what do others think about it?
Would it make sense to have a target/s390x/stsi.c file with the stsi
routines to be called from TCG insn helpers and from the KVM backend ?
This suggestion is based on the services found in the ioinst.c file.
So, target/s390x/kvm/cpu_topology.c would become target/s390x/stsi.c
and stsi services would be moved there, if that makes sense.
Or target/s390x/kvm/stsi.c to start with because services are only
active for KVM targets.
Looking at hw/s390x/meson.build :
s390x_ss.add(when: 'CONFIG_KVM', if_true: files(
'tod-kvm.c',
's390-skeys-kvm.c',
's390-stattrib-kvm.c',
'pv.c',
's390-pci-kvm.c',
'cpu-topology.c',
))
It seems cpu-topology.c should be named cpu-topology-kvm.c to follow
the same convention.
However, I don't see much reason for the KVM condition, apart from
the new polarization definitions in machine-target.json which depend
on KVM. cpu-topology.c could well be compiled without the KVM #ifdef,
all seems in place to detect support at runtime.
In this file, we find a s390_handle_ptf() which is called from
kvm_handle_ptf() in target/s390x/kvm/kvm.c. Is it the right place for
it ? Shouldn't we move the service under target/s390x/kvm/kvm.c ?
Thanks,
C.
