On 7/14/22 13:25, Pierre Morel wrote:
[...]
>
> 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.
>
Yeah, could be a separate series, but then the question remains what you in this one, that is
if you change the code so it would be correct if multithreading were supported.
>>
>>>>> +
>>>>> +/*
>>>>> + * 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.
So you want to be able to plug in, for example, a socket without any cpus in it?
I'm not seeing anything in the description of STSI that forbids having empty containers
or containers with a cpu entry without any cpus. But I don't know why that would be useful.
And if you don't want empty containers, then the container will just show up when plugging in the cpu.
>
>>>
>>> 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
>
[...]
