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

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

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?

Regards,

Pierre