Hi Markus,
On Mon, Jul 22, 2024 at 03:33:13PM +0200, Markus Armbruster wrote:
> Date: Mon, 22 Jul 2024 15:33:13 +0200
> From: Markus Armbruster <armbru@xxxxxxxxxx>
> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object
>
> Zhao Liu <zhao1.liu@xxxxxxxxx> writes:
>
> > Introduce smp-cache object so that user could define cache properties.
> >
> > In smp-cache object, define cache topology based on CPU topology level
> > with two reasons:
> >
> > 1. In practice, a cache will always be bound to the CPU container
> > (either private in the CPU container or shared among multiple
> > containers), and CPU container is often expressed in terms of CPU
> > topology level.
> > 2. The x86's cache-related CPUIDs encode cache topology based on APIC
> > ID's CPU topology layout. And the ACPI PPTT table that ARM/RISCV
> > relies on also requires CPU containers to help indicate the private
> > shared hierarchy of the cache. Therefore, for SMP systems, it is
> > natural to use the CPU topology hierarchy directly in QEMU to define
> > the cache topology.
> >
> > Currently, separated L1 cache (L1 data cache and L1 instruction cache)
> > with unified higher-level cache (e.g., unified L2 and L3 caches), is the
> > most common cache architectures.
> >
> > Therefore, enumerate the L1 D-cache, L1 I-cache, L2 cache and L3 cache
> > with smp-cache object to add the basic cache topology support.
> >
> > Suggested-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> > Signed-off-by: Zhao Liu <zhao1.liu@xxxxxxxxx>
>
> [...]
>
> > diff --git a/qapi/machine-common.json b/qapi/machine-common.json
> > index 82413c668bdb..8b8c0e9eeb86 100644
> > --- a/qapi/machine-common.json
> > +++ b/qapi/machine-common.json
> > @@ -64,3 +64,53 @@
> > 'prefix': 'CPU_TOPO_LEVEL',
> > 'data': [ 'invalid', 'thread', 'core', 'module', 'cluster',
> > 'die', 'socket', 'book', 'drawer', 'default' ] }
> > +
> > +##
> > +# @SMPCacheName:
>
> Why the SMP in this name? Because it's currently only used by SMP
> stuff? Or is there another reason I'm missing?
Yes, I suppose it can only be used in SMP case.
Because Intel's heterogeneous CPUs have different topologies for cache,
for example, Alderlake's L2, for P core, L2 is per P-core, but for E
core, L2 is per module (4 E cores per module). Thus I would like to keep
the topology semantics of this object and -smp as consistent as possible.
Do you agree?
> The more idiomatic QAPI name would be SmpCacheName. Likewise for the
> other type names below.
I hesitated here as well, but considering that SMPConfiguration is "SMP"
and not "Smp", it has that name. I'll change to SmpCacheName for strict
initial capitalization.
> > +#
> > +# An enumeration of cache for SMP systems. The cache name here is
> > +# a combination of cache level and cache type.
>
> The first sentence feels awkward. Maybe
>
> # Caches an SMP system may have.
>
> > +#
> > +# @l1d: L1 data cache.
> > +#
> > +# @l1i: L1 instruction cache.
> > +#
> > +# @l2: L2 (unified) cache.
> > +#
> > +# @l3: L3 (unified) cache
> > +#
> > +# Since: 9.1
> > +##
>
> This assumes the L1 cache is split, and L2 and L3 are unified.
>
> If we model a system with say a unified L1 cache, we'd simply extend
> this enum. No real difference to extending it for additional levels.
> Correct?
Yes. For unified L1, we just need add a "l1" which is opposed to l1i/l1d.
> > +{ 'enum': 'SMPCacheName',
> > + 'prefix': 'SMP_CACHE',
>
> Why not call it SmpCache, and ditch 'prefix'?
Because the SMPCache structure in smp_cache.h uses the similar name:
+#define TYPE_SMP_CACHE "smp-cache"
+OBJECT_DECLARE_SIMPLE_TYPE(SMPCache, SMP_CACHE)
+
+struct SMPCache {
+ Object parent_obj;
+
+ SMPCacheProperty props[SMP_CACHE__MAX];
+};
Naming is always difficult, so I would use Smpcache here if you feel that
SmpCache is sufficient to distinguish it from SMPCache, or I would also
rename the SMPCache structure to SMPCacheState in smp_cache.h.
Which way do you prefer?
> > + 'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
>
> > +
> > +##
> > +# @SMPCacheProperty:
>
> Sure we want to call this "property" (singular) and not "properties"?
> What if we add members to this type?
>
> > +#
> > +# Cache information for SMP systems.
> > +#
> > +# @name: Cache name.
> > +#
> > +# @topo: Cache topology level. It accepts the CPU topology
> > +# enumeration as the parameter, i.e., CPUs in the same
> > +# topology container share the same cache.
> > +#
> > +# Since: 9.1
> > +##
> > +{ 'struct': 'SMPCacheProperty',
> > + 'data': {
> > + 'name': 'SMPCacheName',
> > + 'topo': 'CpuTopologyLevel' } }
>
> We tend to avoid abbreviations in the QAPI schema. Please consider
> naming this 'topology'.
Sure!
> > +
> > +##
> > +# @SMPCacheProperties:
> > +#
> > +# List wrapper of SMPCacheProperty.
> > +#
> > +# @caches: the SMPCacheProperty list.
> > +#
> > +# Since 9.1
> > +##
> > +{ 'struct': 'SMPCacheProperties',
> > + 'data': { 'caches': ['SMPCacheProperty'] } }
>
> Ah, now I see why you used the singular above!
>
> However, this type holds the properties of call caches. It is a list
> where each element holds the properties of a single cache. Calling the
> former "cache property" and the latter "cache properties" is confusing.
Yes...
> SmpCachesProperties and SmpCacheProperties would put the singular
> vs. plural where it belongs. Sounds a bit awkward to me, though.
> Naming is hard.
For SmpCachesProperties, it's easy to overlook the first "s".
> Other ideas, anybody?
Maybe SmpCacheOptions or SmpCachesPropertyWrapper?
> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
> > index b1581988e4eb..25394f2cda50 100644
> > --- a/qapi/qapi-schema.json
> > +++ b/qapi/qapi-schema.json
> > @@ -64,11 +64,11 @@
> > { 'include': 'compat.json' }
> > { 'include': 'control.json' }
> > { 'include': 'introspect.json' }
> > -{ 'include': 'qom.json' }
> > -{ 'include': 'qdev.json' }
> > { 'include': 'machine-common.json' }
> > { 'include': 'machine.json' }
> > { 'include': 'machine-target.json' }
> > +{ 'include': 'qom.json' }
> > +{ 'include': 'qdev.json' }
> > { 'include': 'replay.json' }
> > { 'include': 'yank.json' }
> > { 'include': 'misc.json' }
>
> Worth explaining in the commit message, I think.
Because of the include relationship between the json files, I need to
change the order. I had a "crazy" proposal to clean up this:
https://lore.kernel.org/qemu-devel/20240517062748.782366-1-zhao1.liu@xxxxxxxxx/
Thanks,
Zhao
