Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object

[Markus Armbruster <armbru@redhat.com>]

Zhao Liu <zhao1.liu@intel.com> 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@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

[...]

> 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?

The more idiomatic QAPI name would be SmpCacheName.  Likewise for the
other type names below.

> +#
> +# 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?

> +{ 'enum': 'SMPCacheName',
> +  'prefix': 'SMP_CACHE',

Why not call it SmpCache, and ditch 'prefix'?

> +  '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'.

> +
> +##
> +# @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.

SmpCachesProperties and SmpCacheProperties would put the singular
vs. plural where it belongs.  Sounds a bit awkward to me, though.
Naming is hard.

Other ideas, anybody?

> 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.

> diff --git a/qapi/qom.json b/qapi/qom.json
> index 8bd299265e39..797dd58a61f5 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -8,6 +8,7 @@
>  { 'include': 'block-core.json' }
>  { 'include': 'common.json' }
>  { 'include': 'crypto.json' }
> +{ 'include': 'machine-common.json' }
>  
>  ##
>  # = QEMU Object Model (QOM)
> @@ -1064,6 +1065,7 @@
>        'if': 'CONFIG_SECRET_KEYRING' },
>      'sev-guest',
>      'sev-snp-guest',
> +    'smp-cache',
>      'thread-context',
>      's390-pv-guest',
>      'throttle-group',
> @@ -1135,6 +1137,7 @@
>                                        'if': 'CONFIG_SECRET_KEYRING' },
>        'sev-guest':                  'SevGuestProperties',
>        'sev-snp-guest':              'SevSnpGuestProperties',
> +      'smp-cache':                  'SMPCacheProperties',
>        'thread-context':             'ThreadContextProperties',
>        'throttle-group':             'ThrottleGroupProperties',
>        'tls-creds-anon':             'TlsCredsAnonProperties',