Re: [PATCH 1/8] hw/core: Make CPU topology enumeration arch-agnostic

[Markus Armbruster]

Zhao Liu <zhao1.liu@intel.com> writes:

> Hi Markus,
>
> On Mon, Jul 22, 2024 at 03:24:24PM +0200, Markus Armbruster wrote:
>> Date: Mon, 22 Jul 2024 15:24:24 +0200
>> From: Markus Armbruster <armbru@redhat.com>
>> Subject: Re: [PATCH 1/8] hw/core: Make CPU topology enumeration
>>  arch-agnostic
>> 
>> One little thing...
>> 
>> Zhao Liu <zhao1.liu@intel.com> writes:
>> 
>> > Cache topology needs to be defined based on CPU topology levels. Thus,
>> > define CPU topology enumeration in qapi/machine.json to make it generic
>> > for all architectures.
>> >
>> > To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT
>> > and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and
>> > CPU_TOPO_LEVEL_SOCKET.
>> >
>> > Also, enumerate additional topology levels for non-i386 arches, and add
>> > a CPU_TOPO_LEVEL_DEFAULT to help future smp-cache object de-compatibilize
>> > arch-specific cache topology settings.
>> >
>> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
>> 
>> [...]
>> 
>> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
>> > index fa6bd71d1280..82413c668bdb 100644
>> > --- a/qapi/machine-common.json
>> > +++ b/qapi/machine-common.json
>> > @@ -5,7 +5,7 @@
>> >  # See the COPYING file in the top-level directory.
>> >  
>> >  ##
>> > -# = Machines S390 data types
>> > +# = Common machine types
>> >  ##
>> >  
>> >  ##
>> > @@ -19,3 +19,48 @@
>> >  { 'enum': 'CpuS390Entitlement',
>> >    'prefix': 'S390_CPU_ENTITLEMENT',
>> >    'data': [ 'auto', 'low', 'medium', 'high' ] }
>> > +
>> > +##
>> > +# @CpuTopologyLevel:
>> > +#
>> > +# An enumeration of CPU topology levels.
>> > +#
>> > +# @invalid: Invalid topology level.
>> > +#
>> > +# @thread: thread level, which would also be called SMT level or
>> > +#     logical processor level.  The @threads option in
>> > +#     SMPConfiguration is used to configure the topology of this
>> > +#     level.
>> > +#
>> > +# @core: core level.  The @cores option in SMPConfiguration is used
>> > +#     to configure the topology of this level.
>> > +#
>> > +# @module: module level.  The @modules option in SMPConfiguration is
>> > +#     used to configure the topology of this level.
>> > +#
>> > +# @cluster: cluster level.  The @clusters option in SMPConfiguration
>> > +#     is used to configure the topology of this level.
>> > +#
>> > +# @die: die level.  The @dies option in SMPConfiguration is used to
>> > +#     configure the topology of this level.
>> > +#
>> > +# @socket: socket level, which would also be called package level.
>> > +#     The @sockets option in SMPConfiguration is used to configure
>> > +#     the topology of this level.
>> > +#
>> > +# @book: book level.  The @books option in SMPConfiguration is used
>> > +#     to configure the topology of this level.
>> > +#
>> > +# @drawer: drawer level.  The @drawers option in SMPConfiguration is
>> > +#     used to configure the topology of this level.
>> > +#
>> > +# @default: default level.  Some architectures will have default
>> > +#     topology settings (e.g., cache topology), and this special
>> > +#     level means following the architecture-specific settings.
>> > +#
>> > +# Since: 9.1
>> > +##
>> > +{ 'enum': 'CpuTopologyLevel',
>> > +  'prefix': 'CPU_TOPO_LEVEL',
>> 
>> Why set a 'prefix'?
>> 
>
> Because my previous i386 commit 6ddeb0ec8c29 ("i386/cpu: Introduce bitmap
> to cache available CPU topology levels") introduced the level
> enumeration with such prefix. For naming consistency, and to shorten the
> length of the name, I've used the same prefix here as well.
>
> I've sensed that you don't like the TOPO abbreviation and I'll remove the
> prefix :-).

Consistency is good, but I'd rather achieve it by consistently using
"topology".

I never liked the 'prefix' feature much.  We have it because the mapping
from camel case to upper case with underscores is heuristical, and can
result in something undesirable.  See commit 351d36e454c (qapi: allow
override of default enum prefix naming).  Using it just to shorten
generated identifiers is a bad idea.