On 4/4/23 09:31, Cédric Le Goater wrote:
On 4/3/23 18:28, Pierre Morel wrote:
[...]
+
+/**
+ * s390_socket_nb:
+ * @cpu: s390x CPU
+ *
+ * Returns the socket number used inside the cores_per_socket array
+ * for a cpu.
+ */
+int s390_socket_nb(S390CPU *cpu)
s390_socket_nb() doesn't seem to be used anywhere else than in
hw/s390x/cpu-topology.c. It should be static.
right
[...]
+/**
+ * s390_topology_add_core_to_socket:
+ * @cpu: the new S390CPU to insert in the topology structure
+ * @drawer_id: new drawer_id
+ * @book_id: new book_id
+ * @socket_id: new socket_id
+ * @creation: if is true the CPU is a new CPU and there is no old
socket
+ * to handle.
+ * if is false, this is a moving the CPU and old socket
count
+ * must be decremented.
+ * @errp: the error pointer
+ *
+ */
+static void s390_topology_add_core_to_socket(S390CPU *cpu, int
drawer_id,
+ int book_id, int
socket_id,
+ bool creation, Error
**errp)
+{
Since this routine is called twice, in s390_topology_setup_cpu() for
creation, and in s390_change_topology() for socket migration, we could
duplicate the code in two distinct routines.
I think this would simplify a bit each code path and avoid the 'creation'
parameter which is confusing.
right
Thanks.
+ int old_socket_entry = s390_socket_nb(cpu);
+ int new_socket_entry;
+
+ if (creation) {
+ new_socket_entry = old_socket_entry;
+ } else {
+ new_socket_entry = (drawer_id * s390_topology.smp->books +
book_id) *
+ s390_topology.smp->sockets + socket_id;
A helper common routine that s390_socket_nb() could use also would be
a plus.
Yes, thanks
+ }
+
+ /* Check for space on new socket */
+ if ((new_socket_entry != old_socket_entry) &&
+ (s390_topology.cores_per_socket[new_socket_entry] >=
+ s390_topology.smp->cores)) {
+ error_setg(errp, "No more space on this socket");
+ return;
+ }
+
+ /* Update the count of cores in sockets */
+ s390_topology.cores_per_socket[new_socket_entry] += 1;
+ if (!creation) {
+ s390_topology.cores_per_socket[old_socket_entry] -= 1;
+ }
+}
+
+/**
+ * s390_update_cpu_props:
+ * @ms: the machine state
+ * @cpu: the CPU for which to update the properties from the
environment.
+ *
+ */
+static void s390_update_cpu_props(MachineState *ms, S390CPU *cpu)
+{
+ CpuInstanceProperties *props;
+
+ props = &ms->possible_cpus->cpus[cpu->env.core_id].props;
+
+ props->socket_id = cpu->env.socket_id;
+ props->book_id = cpu->env.book_id;
+ props->drawer_id = cpu->env.drawer_id;
+}
+
+/**
+ * s390_topology_setup_cpu:
+ * @ms: MachineState used to initialize the topology structure on
+ * first call.
+ * @cpu: the new S390CPU to insert in the topology structure
+ * @errp: the error pointer
+ *
+ * Called from CPU Hotplug to check and setup the CPU attributes
+ * before to insert the CPU in the topology.
... before the CPU is inserted in the topology.
OK
+ * There is no use to update the MTCR explicitely here because it
... is no need ... sounds better.
OK
+ * will be updated by KVM on creation of the new vCPU.
"CPU" is used everywhere else.
OK
+ */
+void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error
**errp)
+{
+ ERRP_GUARD();
+
+ /*
+ * We do not want to initialize the topology if the cpu model
+ * does not support topology, consequently, we have to wait for
+ * the first CPU to be realized, which realizes the CPU model
+ * to initialize the topology structures.
+ *
+ * s390_topology_setup_cpu() is called from the cpu hotplug.
+ */
+ if (!s390_topology.cores_per_socket) {
+ s390_topology_init(ms);
+ }
+
+ s390_topology_cpu_default(cpu, errp);
+ if (*errp) {
May be having s390_topology_cpu_default() return a bool would be cleaner.
Same comment for the routines below. This is minor.
Yes and it is more readable. I do it.
Thanks for the comments.
Regards,
Pierre
