Re: [PATCH v8 02/12] s390x/cpu_topology: CPU topology objects and structures

[Pierre Morel <pmorel@linux.ibm.com>]

On 7/14/22 14:50, Janis Schoetterl-Glausch wrote:
> 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.

I would like to first not support multi-thread and do a fatal error if 
threads are defined or implicitly defined as different of 1.

I prefer to keep multithreading for later, I did not have a look at all 
the implications for the moment.

>>>
>>>>>> +
>>>>>> +/*
>>>>>> + * 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.

You already convinced me, it is a non sense and, anyway, building every 
container when a cpu is added is how it works with the current 
implementation.


-- 
Pierre Morel
IBM Lab Boeblingen