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

[Thomas Huth]

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