Re: [PATCH v20 01/21] s390x/cpu topology: add s390 specifics to CPU topology

[Thomas Huth <thuth@redhat.com>]

On 03/05/2023 11.36, Pierre Morel wrote:
> 
> On 4/27/23 10:04, Thomas Huth wrote:
>> On 25/04/2023 18.14, Pierre Morel wrote:
>>> S390 adds two new SMP levels, drawers and books to the CPU
>>> topology.
>>> The S390 CPU have specific topology features like dedication
>>> and entitlement to give to the guest indications on the host
>>> vCPUs scheduling and help the guest take the best decisions
>>> on the scheduling of threads on the vCPUs.
>>>
>>> Let us provide the SMP properties with books and drawers levels
>>> and S390 CPU with dedication and entitlement,
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> 
> [...]
> 
> 
>>>   {
>>>       MachineClass *mc = MACHINE_GET_CLASS(ms);
>>>       unsigned cpus    = config->has_cpus ? config->cpus : 0;
>>> +    unsigned drawers = config->has_drawers ? config->drawers : 0;
>>> +    unsigned books   = config->has_books ? config->books : 0;
>>>       unsigned sockets = config->has_sockets ? config->sockets : 0;
>>>       unsigned dies    = config->has_dies ? config->dies : 0;
>>>       unsigned clusters = config->has_clusters ? config->clusters : 0;
>>> @@ -85,6 +98,8 @@ void machine_parse_smp_config(MachineState *ms,
>>>        * explicit configuration like "cpus=0" is not allowed.
>>>        */
>>>       if ((config->has_cpus && config->cpus == 0) ||
>>> +        (config->has_drawers && config->drawers == 0) ||
>>> +        (config->has_books && config->books == 0) ||
>>>           (config->has_sockets && config->sockets == 0) ||
>>>           (config->has_dies && config->dies == 0) ||
>>>           (config->has_clusters && config->clusters == 0) ||
>>> @@ -111,6 +126,19 @@ void machine_parse_smp_config(MachineState *ms,
>>>       dies = dies > 0 ? dies : 1;
>>>       clusters = clusters > 0 ? clusters : 1;
>>>   +    if (!mc->smp_props.books_supported && books > 1) {
>>> +        error_setg(errp, "books not supported by this machine's CPU 
>>> topology");
>>> +        return;
>>> +    }
>>> +    books = books > 0 ? books : 1;
>>
>> Could be shortened to:  book = books ?: 1;
>>
> More thinking about this, all other existing assignments are done so, 
> clusters, dies, sockets, cores and threads.
> 
> to keep the core consistent shouldn't we keep it the same way?

Fine for me, too. It just might happen that I forget about it and suggest it 
again in a future version of the patch ;-)

  Thomas