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

[Zhao Liu <zhao1.liu@intel.com>]

On Fri, Oct 18, 2024 at 08:55:35AM +0100, Daniel P. Berrangé wrote:
> Date: Fri, 18 Oct 2024 08:55:35 +0100
> From: "Daniel P. Berrangé" <berrange@redhat.com>
> Subject: Re: [PATCH v3 1/7] hw/core: Make CPU topology enumeration
>  arch-agnostic
> 
> On Fri, Oct 18, 2024 at 10:36:30AM +0800, Zhao Liu wrote:
> > Hi Daniel,
> > 
> > > > -/*
> > > > - * CPUTopoLevel is the general i386 topology hierarchical representation,
> > > > - * ordered by increasing hierarchical relationship.
> > > > - * Its enumeration value is not bound to the type value of Intel (CPUID[0x1F])
> > > > - * or AMD (CPUID[0x80000026]).
> > > > - */
> > > > -enum CPUTopoLevel {
> > > > -    CPU_TOPO_LEVEL_INVALID,
> > > > -    CPU_TOPO_LEVEL_SMT,
> > > > -    CPU_TOPO_LEVEL_CORE,
> > > > -    CPU_TOPO_LEVEL_MODULE,
> > > > -    CPU_TOPO_LEVEL_DIE,
> > > > -    CPU_TOPO_LEVEL_PACKAGE,
> > > > -    CPU_TOPO_LEVEL_MAX,
> > > > -};
> > > > -
> > > 
> > > snip
> > > 
> > > > @@ -18,3 +18,47 @@
> > > >  ##
> > > >  { 'enum': 'S390CpuEntitlement',
> > > >    'data': [ 'auto', 'low', 'medium', 'high' ] }
> > > > +
> > > > +##
> > > > +# @CpuTopologyLevel:
> > > > +#
> > > > +# An enumeration of CPU topology levels.
> > > > +#
> > > > +# @invalid: Invalid topology level.
> > > 
> > > Previously all topology levels were internal to QEMU, and IIUC
> > > this CPU_TOPO_LEVEL_INVALID appears to have been a special
> > > value to indicate  the cache was absent ?
> > 
> > Now I haven't support this logic.
> > x86 CPU has a "l3-cache" property, and maybe that property can be
> > implemented or replaced by the "invalid" level support you mentioned.
> > 
> > > Now we're exposing this directly to the user as a settable
> > > option. We need to explain what effect setting 'invalid'
> > > has on the CPU cache config.
> > 
> > If user set "invalid", QEMU will report the error message:
> > 
> > qemu-system-x86_64: Invalid cache topology level: invalid. The topology should match valid CPU topology level
> > 
> > Do you think this error message is sufficient?
> 
> If the user cannot set 'invalid' as an input, and no QEMU interface
> will emit, then ideally we would not define 'invalid' in the QAPI
> schema at all.
> 
> This woudl require us to have some internal only way to record
> "invalid", separately from the topology level, or with a magic
> internal only constant that doesn't clash with the public enum
> constants. I guess the latter would be less work e.g. we could
> "abuse" the 'MAX' constant value
> 
>    #define CPU_TOPOLOGY_LEVEL_INVALID CPU_TOPOLOGY_LEVEL_MAX
> 
> or separate it with a negative value
> 
>    #define CPU_TOPOLOGY_LEVEL_INVALID -1
>

This's a clever idea. Thank you!

Rgards,
Zhao