On 7/14/22 12:38, Janis Schoetterl-Glausch wrote:
On 7/13/22 16:59, Pierre Morel wrote:
On 7/12/22 17:40, Janis Schoetterl-Glausch wrote:
On 6/20/22 16:03, Pierre Morel wrote:
[...]
+}
+
+/*
+ * s390_topology_new_cpu:
+ * @core_id: the core ID is machine wide
+ *
+ * We have a single book returned by s390_get_topology(),
+ * then we build the hierarchy on demand.
+ * Note that we do not destroy the hierarchy on error creating
+ * an entry in the topology, we just keep it empty.
+ * We do not need to worry about not finding a topology level
+ * entry this would have been caught during smp parsing.
+ */
+bool s390_topology_new_cpu(MachineState *ms, int core_id, Error **errp)
+{
+ S390TopologyBook *book;
+ S390TopologySocket *socket;
+ S390TopologyCores *cores;
+ int nb_cores_per_socket;
num_cores_per_socket instead?
+ int origin, bit;
+
+ book = s390_get_topology();
+
+ nb_cores_per_socket = ms->smp.cores * ms->smp.threads;
We don't support the multithreading facility, do we?
So, I think we should assert smp.threads == 1 somewhere.
In any case I think the correct expression would round the threads up to the next power of 2,
because the core_id has the thread id in the lower bits, but threads per core doesn't need to be
a power of 2 according to the architecture.
That is right.
I will add that.
Add the assert?
It should probably be somewhere else.
That is sure.
I thought about put a fatal error report during the initialization in
the s390_topology_setup()
And you can set thread > 1 today, so we'd need to handle that. (increase the number of cpus instead and print a warning?)
[...]
this would introduce arch dependencies in the hw/core/
I think that the error report for Z is enough.
So once we support Multithreading in the guest we can adjust it easier
without involving the common code.
Or we can introduce a thread_supported in SMPCompatProps, which would be
good.
I would prefer to propose this outside of the series and suppress the
fatal error once it is adopted.
+
+/*
+ * Setting the first topology: 1 book, 1 socket
+ * This is enough for 64 cores if the topology is flat (single socket)
+ */
+void s390_topology_setup(MachineState *ms)
+{
+ DeviceState *dev;
+
+ /* Create BOOK bridge device */
+ dev = qdev_new(TYPE_S390_TOPOLOGY_BOOK);
+ object_property_add_child(qdev_get_machine(),
+ TYPE_S390_TOPOLOGY_BOOK, OBJECT(dev));
Why add it to the machine instead of directly using a static?
For my opinion it is a characteristic of the machine.
So it's visible to the user via info qtree or something?
It is already visible to the user on info qtree.
Would that even be the appropriate location to show that?
That is a very good question and I really appreciate if we discuss on the design before diving into details.
The idea is to have the architecture details being on qtree as object so we can plug new drawers/books/socket/cores and in the future when the infrastructure allows it unplug them.
Would it not be more accurate to say that we plug in new cpus only?
Since you need to specify the topology up front with -smp and it cannot change after.
smp specify the maximum we can have.
I thought we can add dynamically elements inside this maximum set.
So that all is static, books/sockets might be completely unpopulated, but they still exist in a way.
As far as I understand, STSI only allows for cpus to change, nothing above it.
I thought we want to plug new books or drawers but I may be wrong.
There is a info numa (info cpus does not give a lot info) to give information on nodes but AFAIU, a node is more a theoritical that can be used above the virtual architecture, sockets/cores, to specify characteristics like distance and associated memory.
https://qemu.readthedocs.io/en/latest/interop/qemu-qmp-ref.html#qapidoc-2391
shows that the relevant information can be queried via qmp.
When I tried it on s390x it only showed the core_id, but we should be able to add the rest.
yes, sure.
Am I correct in my understanding, that there are two reasons to have the hierarchy objects:
1. Caching the topology instead of computing it when STSI is called
2. So they show up in info qtree
?
and have the possibility to add the objects dynamically. yes
[...]
+ /*
+ * Each CPU inside a socket will be represented by a bit in a 64bit
+ * unsigned long. Set on plug and clear on unplug of a CPU.
+ * All CPU inside a mask share the same dedicated, polarity and
+ * cputype values.
+ * The origin is the offset of the first CPU in a mask.
+ */
+struct S390TopologyCores {
+ DeviceState parent_obj;
+ int id;
+ bool dedicated;
+ uint8_t polarity;
+ uint8_t cputype;
Why not snake_case for cpu type?
I do not understand what you mean.
I'm suggesting s/cputype/cpu_type/
ok
Thanks,
regards,
Pierre
--
Pierre Morel
IBM Lab Boeblingen
