On Tue, Jul 23, 2024 at 12:14:30PM +0200, Markus Armbruster wrote:
> Date: Tue, 23 Jul 2024 12:14:30 +0200
> From: Markus Armbruster <armbru@xxxxxxxxxx>
> Subject: Re: [PATCH 1/8] hw/core: Make CPU topology enumeration
> arch-agnostic
>
> Zhao Liu <zhao1.liu@xxxxxxxxx> 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@xxxxxxxxxx>
> >> Subject: Re: [PATCH 1/8] hw/core: Make CPU topology enumeration
> >> arch-agnostic
> >>
> >> One little thing...
> >>
> >> Zhao Liu <zhao1.liu@xxxxxxxxx> 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@xxxxxxxxx>
> >>
> >> [...]
> >>
> >> > 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.
Thanks for your clarification! I see, I will drop the prefix.
Regards,
Zhao
