[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v20 02/21] s390x/cpu topology: add topology entries on CPU ho

From: Pierre Morel
Subject: Re: [PATCH v20 02/21] s390x/cpu topology: add topology entries on CPU hotplug
Date: Wed, 21 Jun 2023 15:48:49 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0

On 5/3/23 12:23, Cédric Le Goater wrote:

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@linux.ibm.com>
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.


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(

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 ?



Hello Cédric,

The reason to have the s390_handle_ptf() here is to be ready for TCG.

We have the choice to let the cpu-topology.c file inside the KVM list of meson.build until TCG provides the CPU_TOPOLOGY facility and then move it to the general list
to move it now to the general list inside meson.build and keep code that will only be useful when TCG provides the CPU_TOPOLOGY facility

The option taken here is to not let the code active if it is not in use.

Did I answered the question or forgot something?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]