| Message ID | 20240704031603.1744546-3-zhao1.liu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce SMP Cache Topology | expand |
> diff --git a/hw/core/smp-cache.c b/hw/core/smp-cache.c > new file mode 100644 > index 000000000000..c0157ce51c8f > --- /dev/null > +++ b/hw/core/smp-cache.c > @@ -0,0 +1,103 @@ > +/* > + * Cache Object for SMP machine > + * > + * Copyright (C) 2024 Intel Corporation. > + * > + * Author: Zhao Liu <zhao1.liu@intel.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or > + * later. See the COPYING file in the top-level directory. > + */ > + > +#include "qemu/osdep.h" > + > +#include "hw/core/smp-cache.h" > +#include "qapi/error.h" > +#include "qapi/qapi-visit-machine-common.h" > +#include "qom/object_interfaces.h" > + > +static void > +smp_cache_get_cache_prop(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + SMPCache *cache = SMP_CACHE(obj); > + SMPCachePropertyList *head = NULL; > + SMPCachePropertyList **tail = &head; > + > + for (int i = 0; i < SMP_CACHE__MAX; i++) { > + SMPCacheProperty *node = g_new(SMPCacheProperty, 1); > + > + node->name = cache->props[i].name; > + node->topo = cache->props[i].topo; > + QAPI_LIST_APPEND(tail, node); > + } > + > + if (!visit_type_SMPCachePropertyList(v, name, &head, errp)) { > + return; Oops, here I shouldn't return. Whether it succeeds or not, I should continue with the following free(). > + } > + qapi_free_SMPCachePropertyList(head); > +}
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',
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@redhat.com> > Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object > > 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? 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@intel.com/ Thanks, Zhao
Zhao Liu <zhao1.liu@intel.com> writes: > 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@redhat.com> >> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object >> >> 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? > > 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? I don't know enough to meaningfully agree or disagree. I know just enough to annoy you with questions :) This series adds a way to configure caches. Structure of the configuration data: a list [{"name": N, "topo": T}, ...] where N can be "l1d", "l1i", "l2", or "l3", and T can be "invalid", "thread", "core", "module", "cluster", "die", "socket", "book", "drawer", or "default". What's the use case? The commit messages don't tell. Why does that use case make no sense without SMP? Can the same @name occur multiple times? Documentation doesn't tell. If yes, what does that mean? Say we later add value "l1" for unified level 1 cache. Would "l1" then conflict with "l1d" and "l1u"? May @topo be "invalid"? Documentation doesn't tell. If yes, what does that mean? >> 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, Oh yes. > 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? Having both QAPI enum SmpCache and handwritten type SMPCache is clearly undesirable. I retract my suggestion to name the enum SmpCache. The thing clearly is not a cache. SmpCacheName is better. If you drop 'prefix', the generated C enum values look like SMP_CACHE_NAME_FOO. Would that work for you? The "name" part bothers me a bit. A name doesn't define what something is. A type does. SmpCacheType? >> > + '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 "of all caches" (can't type). >> 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? I wonder why we have a single QOM object to configure all caches, and not one QOM object per cache. >> > 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@intel.com/ > > Thanks, > Zhao
On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote: > Zhao Liu <zhao1.liu@intel.com> writes: > > > Hi Markus, > >> 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? > > I wonder why we have a single QOM object to configure all caches, and > not one QOM object per cache. Previous versions of this series were augmenting the existing -smp command line. Now the design has switched to use -object, I agree that it'd be simplest to just have one -object flag added per cache level we want to defnie. With regards, Daniel
On Wed, Jul 24, 2024 at 01:47:16PM +0100, Daniel P. Berrangé wrote: > Date: Wed, 24 Jul 2024 13:47:16 +0100 > From: "Daniel P. Berrangé" <berrange@redhat.com> > Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object > > On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote: > > Zhao Liu <zhao1.liu@intel.com> writes: > > > > > Hi Markus, > > >> 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? > > > > I wonder why we have a single QOM object to configure all caches, and > > not one QOM object per cache. > > Previous versions of this series were augmenting the existing > -smp command line. Ah, yes, since -smp, as a sugar option of -machine, doesn't support JSON. In -smp, we need to use keyval's style to configure as: -smp caches.0.name=l1i,caches.0.topo=core I think JSON is the more elegant way to go, so I chose -object. > Now the design has switched to use -object, > I agree that it'd be simplest to just have one -object flag > added per cache level we want to defnie. > OK. Thanks, Zhao
Hi Markus, I realized I should reply this mail first... On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote: > Date: Wed, 24 Jul 2024 13:35:17 +0200 > From: Markus Armbruster <armbru@redhat.com> > Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object > > Zhao Liu <zhao1.liu@intel.com> writes: > > > 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@redhat.com> > >> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object > >> > >> 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? > > > > 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? > > I don't know enough to meaningfully agree or disagree. I know just > enough to annoy you with questions :) Welcome and no problem! > This series adds a way to configure caches. > > Structure of the configuration data: a list > > [{"name": N, "topo": T}, ...] > > where N can be "l1d", "l1i", "l2", or "l3", > and T can be "invalid", "thread", "core", "module", "cluster", > "die", "socket", "book", "drawer", or "default". > > What's the use case? The commit messages don't tell. i386 has the default cache topology model: l1 per core/l2 per core/l3 per die. Cache topology affects scheduler performance, e.g., kernel's cluster scheduling. Of course I can hardcode some cache topology model in the specific cpu model that corresponds to the actual hardware, but for -cpu host/max, the default i386 cache topology model has no flexibility, and the host-cpu-cache option doesn't have enough fine-grained control over the cache topology. So I want to provide a way to allow user create more fleasible cache topology. Just like cpu topology. > Why does that use case make no sense without SMP? As the example I mentioned, for Intel hyrbid architecture, P cores has l2 per core and E cores has l2 per module. Then either setting the l2 topology level as core nor module, can emulate the real case. Even considering the more extreme case of Intel 14th MTL CPU, where some E cores have L3 and some don't even have L3. As well as the last time you and Daniel mentioned that in the future we could consider covering more cache properties such as cache size. But the l3 size can be different in the same system, like AMD's x3D technology. So generally configuring properties for @name in a list can't take into account the differences of heterogeneous caches with the same @name. Hope my poor english explains the problem well. :-) > Can the same @name occur multiple times? Documentation doesn't tell. > If yes, what does that mean? Yes, this means the later one will override the previous one with the same name. > Say we later add value "l1" for unified level 1 cache. Would "l1" then > conflict with "l1d" and "l1u"? Yes, we should check in smp/machine code and ban l1 and l1i/l1d at the same time. This check I suppose is easy to add. > May @topo be "invalid"? Documentation doesn't tell. If yes, what does > that mean? Yes, just follow the intel's spec, invalid means the current topology information is invalid, which is used to encode x86 CPUIDs. So when I move this level to qapi, I just keeped this. Otherwise, I need to re-implement the i386 specific invalid level. > >> 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, > > Oh yes. > > > 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? > > Having both QAPI enum SmpCache and handwritten type SMPCache is clearly > undesirable. > > I retract my suggestion to name the enum SmpCache. The thing clearly is > not a cache. SmpCacheName is better. > > If you drop 'prefix', the generated C enum values look like > SMP_CACHE_NAME_FOO. Would that work for you? I think the SmpCacheName is ok, since there's no other better names. > The "name" part bothers me a bit. A name doesn't define what something > is. A type does. SmpCacheType? Ah, I also considerred this. I didn't use "type" because people usually uses cache type to indicate INSTRUCTION/DATA/UNIFIED and cache level to indicate LEVEL 1/LEVEL 2/LEVEL 3. The enumeration here is a combination of type+level. So I think it's better to avoid the type term. > >> > + '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 > > "of all caches" (can't type). Sorry I didn't get your point? > >> 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? > > I wonder why we have a single QOM object to configure all caches, and > not one QOM object per cache. I have the thoughts and questions here: https://lore.kernel.org/qemu-devel/20240704031603.1744546-1-zhao1.liu@intel.com/T/#m8adba8ba14ebac0c9935fbf45983cc71e53ccf45 We could discuss this issue in that thread :-). > >> > 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@intel.com/ > > > > Thanks, > > Zhao >
Hi Daniel, On Wed, Jul 24, 2024 at 10:03:02PM +0800, Zhao Liu wrote: > Date: Wed, 24 Jul 2024 22:03:02 +0800 > From: Zhao Liu <zhao1.liu@intel.com> > Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object > > On Wed, Jul 24, 2024 at 01:47:16PM +0100, Daniel P. Berrang? wrote: > > Date: Wed, 24 Jul 2024 13:47:16 +0100 > > From: "Daniel P. Berrang?" <berrange@redhat.com> > > Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object > > > > On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote: > > > Zhao Liu <zhao1.liu@intel.com> writes: > > > > > > > Hi Markus, > > > >> 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? > > > > > > I wonder why we have a single QOM object to configure all caches, and > > > not one QOM object per cache. > > > > Previous versions of this series were augmenting the existing > > -smp command line. > > Ah, yes, since -smp, as a sugar option of -machine, doesn't support > JSON. In -smp, we need to use keyval's style to configure as: > > -smp caches.0.name=l1i,caches.0.topo=core > > I think JSON is the more elegant way to go, so I chose -object. I may have to retract this assertion considering more issues, I could fall back to -smp and support it in keyval format, I think it's also ok for me if you also like keyval format, sorry for my repetition, we can discuss this in this thread: https://lore.kernel.org/qemu-devel/20240704031603.1744546-1-zhao1.liu@intel.com/T/#m8adba8ba14ebac0c9935fbf45983cc71e53ccf45 Thanks, Zhao
Zhao Liu <zhao1.liu@intel.com> writes: > Hi Markus, > > I realized I should reply this mail first... > > On Wed, Jul 24, 2024 at 01:35:17PM +0200, Markus Armbruster wrote: >> Date: Wed, 24 Jul 2024 13:35:17 +0200 >> From: Markus Armbruster <armbru@redhat.com> >> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object >> >> Zhao Liu <zhao1.liu@intel.com> writes: >> >> > 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@redhat.com> >> >> Subject: Re: [PATCH 2/8] qapi/qom: Introduce smp-cache object >> >> >> >> 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? >> > >> > 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? >> >> I don't know enough to meaningfully agree or disagree. I know just >> enough to annoy you with questions :) > > Welcome and no problem! > >> This series adds a way to configure caches. >> >> Structure of the configuration data: a list >> >> [{"name": N, "topo": T}, ...] >> >> where N can be "l1d", "l1i", "l2", or "l3", >> and T can be "invalid", "thread", "core", "module", "cluster", >> "die", "socket", "book", "drawer", or "default". >> >> What's the use case? The commit messages don't tell. > > i386 has the default cache topology model: l1 per core/l2 per core/l3 > per die. > > Cache topology affects scheduler performance, e.g., kernel's cluster > scheduling. > > Of course I can hardcode some cache topology model in the specific cpu > model that corresponds to the actual hardware, but for -cpu host/max, > the default i386 cache topology model has no flexibility, and the > host-cpu-cache option doesn't have enough fine-grained control over the > cache topology. > > So I want to provide a way to allow user create more fleasible cache > topology. Just like cpu topology. So the use case is exposing a configurable cache topology to the guest in order to increase performance. Performance can increase when the configured virtual topology is closer to the physical topology than a default topology would be. This can be the case with CPU host or max. Correct? >> Why does that use case make no sense without SMP? > > As the example I mentioned, for Intel hyrbid architecture, P cores has > l2 per core and E cores has l2 per module. Then either setting the l2 > topology level as core nor module, can emulate the real case. > > Even considering the more extreme case of Intel 14th MTL CPU, where > some E cores have L3 and some don't even have L3. As well as the last > time you and Daniel mentioned that in the future we could consider > covering more cache properties such as cache size. But the l3 size can > be different in the same system, like AMD's x3D technology. So > generally configuring properties for @name in a list can't take into > account the differences of heterogeneous caches with the same @name. > > Hope my poor english explains the problem well. :-) I think I understand why you want to configure caches. My question was about the connection to SMP. Say we run a guest with a single core, no SMP. Could configuring caches still be useful then? >> Can the same @name occur multiple times? Documentation doesn't tell. >> If yes, what does that mean? > > Yes, this means the later one will override the previous one with the same > name. Needs documenting. If you make it an error, you don't have to document it :) >> Say we later add value "l1" for unified level 1 cache. Would "l1" then >> conflict with "l1d" and "l1u"? > > Yes, we should check in smp/machine code and ban l1 and l1i/l1d at the > same time. This check I suppose is easy to add. > >> May @topo be "invalid"? Documentation doesn't tell. If yes, what does >> that mean? > > Yes, just follow the intel's spec, invalid means the current topology > information is invalid, which is used to encode x86 CPUIDs. So when I > move this level to qapi, I just keeped this. Otherwise, I need to > re-implement the i386 specific invalid level. I'm afraid I don't understand what is supposed to happen when I tell QEMU to make a cache's topology invalid. >> >> 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, >> >> Oh yes. >> >> > 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? >> >> Having both QAPI enum SmpCache and handwritten type SMPCache is clearly >> undesirable. >> >> I retract my suggestion to name the enum SmpCache. The thing clearly is >> not a cache. SmpCacheName is better. >> >> If you drop 'prefix', the generated C enum values look like >> SMP_CACHE_NAME_FOO. Would that work for you? > > I think the SmpCacheName is ok, since there's no other better names. > >> The "name" part bothers me a bit. A name doesn't define what something >> is. A type does. SmpCacheType? > > Ah, I also considerred this. I didn't use "type" because people usually > uses cache type to indicate INSTRUCTION/DATA/UNIFIED and cache level to > indicate LEVEL 1/LEVEL 2/LEVEL 3. The enumeration here is a combination of > type+level. So I think it's better to avoid the type term. SmpCacheLevelAndType is quite a mouthful. >> >> > + '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 >> >> "of all caches" (can't type). > > Sorry I didn't get your point? I had typoed "of call caches", which makes no sense, so I corrected it. >> >> 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? >> >> I wonder why we have a single QOM object to configure all caches, and >> not one QOM object per cache. > > I have the thoughts and questions here: > > https://lore.kernel.org/qemu-devel/20240704031603.1744546-1-zhao1.liu@intel.com/T/#m8adba8ba14ebac0c9935fbf45983cc71e53ccf45 > > We could discuss this issue in that thread :-). Okay. [...]
Hi Markus, On Thu, Jul 25, 2024 at 10:51:49AM +0200, Markus Armbruster wrote: [snip] > >> What's the use case? The commit messages don't tell. > > > > i386 has the default cache topology model: l1 per core/l2 per core/l3 > > per die. > > > > Cache topology affects scheduler performance, e.g., kernel's cluster > > scheduling. > > > > Of course I can hardcode some cache topology model in the specific cpu > > model that corresponds to the actual hardware, but for -cpu host/max, > > the default i386 cache topology model has no flexibility, and the > > host-cpu-cache option doesn't have enough fine-grained control over the > > cache topology. > > > > So I want to provide a way to allow user create more fleasible cache > > topology. Just like cpu topology. > > > So the use case is exposing a configurable cache topology to the guest > in order to increase performance. Performance can increase when the > configured virtual topology is closer to the physical topology than a > default topology would be. This can be the case with CPU host or max. > > Correct? Yes! That's x86 use case. Jonathan also helped me explain his ARM use case. > >> Why does that use case make no sense without SMP? > > > > As the example I mentioned, for Intel hyrbid architecture, P cores has > > l2 per core and E cores has l2 per module. Then either setting the l2 > > topology level as core nor module, can emulate the real case. > > > > Even considering the more extreme case of Intel 14th MTL CPU, where > > some E cores have L3 and some don't even have L3. As well as the last > > time you and Daniel mentioned that in the future we could consider > > covering more cache properties such as cache size. But the l3 size can > > be different in the same system, like AMD's x3D technology. So > > generally configuring properties for @name in a list can't take into > > account the differences of heterogeneous caches with the same @name. > > > > Hope my poor english explains the problem well. :-) > > I think I understand why you want to configure caches. My question was > about the connection to SMP. > > Say we run a guest with a single core, no SMP. Could configuring caches > still be useful then? No, for this case the CPU topology (of x86) would be 1 core per module, 1 module per die, 1 die per socket. Then this core actually owns the l1/l2/l3. > >> Can the same @name occur multiple times? Documentation doesn't tell. > >> If yes, what does that mean? > > > > Yes, this means the later one will override the previous one with the same > > name. > > Needs documenting. > > If you make it an error, you don't have to document it :) OK! > >> Say we later add value "l1" for unified level 1 cache. Would "l1" then > >> conflict with "l1d" and "l1u"? > > > > Yes, we should check in smp/machine code and ban l1 and l1i/l1d at the > > same time. This check I suppose is easy to add. > > > >> May @topo be "invalid"? Documentation doesn't tell. If yes, what does > >> that mean? > > > > Yes, just follow the intel's spec, invalid means the current topology > > information is invalid, which is used to encode x86 CPUIDs. So when I > > move this level to qapi, I just keeped this. Otherwise, I need to > > re-implement the i386 specific invalid level. > > I'm afraid I don't understand what is supposed to happen when I tell > QEMU to make a cache's topology invalid. Currently this series doesn't allow users to set invalid, if they do, QEMU reports an error. So this invalid is just for QEMU internal use. Do you think it's okay? [snip] > > Ah, I also considerred this. I didn't use "type" because people usually > > uses cache type to indicate INSTRUCTION/DATA/UNIFIED and cache level to > > indicate LEVEL 1/LEVEL 2/LEVEL 3. The enumeration here is a combination of > > type+level. So I think it's better to avoid the type term. > > SmpCacheLevelAndType is quite a mouthful. Better name! Thanks! Regards, Zhao
Thanks Jonathon! On Thu, Jul 25, 2024 at 11:59:02AM +0100, Jonathan Cameron wrote: [snip] > > > I think I understand why you want to configure caches. My question was > > > about the connection to SMP. > > > > > > Say we run a guest with a single core, no SMP. Could configuring caches > > > still be useful then? > > > > Probably not useful to configure topology (sizes are a separate question) > > - any sensible default should be fine. > > Yes, I agree. Regards, Zhao
diff --git a/MAINTAINERS b/MAINTAINERS index 6725913c8b3a..b5391a7538de 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1885,12 +1885,14 @@ F: hw/core/machine.c F: hw/core/machine-smp.c F: hw/core/null-machine.c F: hw/core/numa.c +F: hw/core/smp-cache.c F: hw/cpu/cluster.c F: qapi/machine.json F: qapi/machine-common.json F: qapi/machine-target.json F: include/hw/boards.h F: include/hw/core/cpu.h +F: include/hw/core/smp-cache.h F: include/hw/cpu/cluster.h F: include/sysemu/numa.h F: tests/unit/test-smp-parse.c diff --git a/hw/core/meson.build b/hw/core/meson.build index a3d9bab9f42a..6d3dae3af62e 100644 --- a/hw/core/meson.build +++ b/hw/core/meson.build @@ -14,6 +14,7 @@ hwcore_ss.add(files( common_ss.add(files('cpu-common.c')) common_ss.add(files('machine-smp.c')) +common_ss.add(files('smp-cache.c')) system_ss.add(when: 'CONFIG_FITLOADER', if_true: files('loader-fit.c')) system_ss.add(when: 'CONFIG_GENERIC_LOADER', if_true: files('generic-loader.c')) system_ss.add(when: 'CONFIG_GUEST_LOADER', if_true: files('guest-loader.c')) diff --git a/hw/core/smp-cache.c b/hw/core/smp-cache.c new file mode 100644 index 000000000000..c0157ce51c8f --- /dev/null +++ b/hw/core/smp-cache.c @@ -0,0 +1,103 @@ +/* + * Cache Object for SMP machine + * + * Copyright (C) 2024 Intel Corporation. + * + * Author: Zhao Liu <zhao1.liu@intel.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or + * later. See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" + +#include "hw/core/smp-cache.h" +#include "qapi/error.h" +#include "qapi/qapi-visit-machine-common.h" +#include "qom/object_interfaces.h" + +static void +smp_cache_get_cache_prop(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + SMPCache *cache = SMP_CACHE(obj); + SMPCachePropertyList *head = NULL; + SMPCachePropertyList **tail = &head; + + for (int i = 0; i < SMP_CACHE__MAX; i++) { + SMPCacheProperty *node = g_new(SMPCacheProperty, 1); + + node->name = cache->props[i].name; + node->topo = cache->props[i].topo; + QAPI_LIST_APPEND(tail, node); + } + + if (!visit_type_SMPCachePropertyList(v, name, &head, errp)) { + return; + } + qapi_free_SMPCachePropertyList(head); +} + +static void +smp_cache_set_cache_prop(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + SMPCache *cache = SMP_CACHE(obj); + SMPCachePropertyList *list, *node; + + if (!visit_type_SMPCachePropertyList(v, name, &list, errp)) { + return; + } + + for (node = list; node; node = node->next) { + if (node->value->topo == CPU_TOPO_LEVEL_INVALID) { + error_setg(errp, + "Invalid topology level: %s. " + "The topology should match the valid CPU topology level", + CpuTopologyLevel_str(node->value->topo)); + goto out; + } + cache->props[node->value->name].topo = node->value->topo; + } + +out: + qapi_free_SMPCachePropertyList(list); +} + +static void smp_cache_class_init(ObjectClass *oc, void *data) +{ + object_class_property_add(oc, "caches", "SMPCacheProperties", + smp_cache_get_cache_prop, + smp_cache_set_cache_prop, + NULL, NULL); + object_class_property_set_description(oc, "caches", + "Cache property list for SMP machine"); +} + +static void smp_cache_instance_init(Object *obj) +{ + SMPCache *cache = SMP_CACHE(obj); + for (int i = 0; i < SMP_CACHE__MAX; i++) { + cache->props[i].name = (SMPCacheName)i; + cache->props[i].topo = CPU_TOPO_LEVEL_DEFAULT; + } +} + +static const TypeInfo smp_cache_info = { + .parent = TYPE_OBJECT, + .name = TYPE_SMP_CACHE, + .class_init = smp_cache_class_init, + .instance_size = sizeof(SMPCache), + .instance_init = smp_cache_instance_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_USER_CREATABLE }, + { } + } +}; + +static void smp_cache_register_type(void) +{ + type_register_static(&smp_cache_info); +} + +type_init(smp_cache_register_type); diff --git a/include/hw/core/smp-cache.h b/include/hw/core/smp-cache.h new file mode 100644 index 000000000000..c6b4d9efc290 --- /dev/null +++ b/include/hw/core/smp-cache.h @@ -0,0 +1,27 @@ +/* + * Cache Object for SMP machine + * + * Copyright (C) 2024 Intel Corporation. + * + * Author: Zhao Liu <zhao1.liu@intel.com> + * + * This work is licensed under the terms of the GNU GPL, version 2 or + * later. See the COPYING file in the top-level directory. + */ + +#ifndef SMP_CACHE_H +#define SMP_CACHE_H + +#include "qapi/qapi-types-machine-common.h" +#include "qom/object.h" + +#define TYPE_SMP_CACHE "smp-cache" +OBJECT_DECLARE_SIMPLE_TYPE(SMPCache, SMP_CACHE) + +struct SMPCache { + Object parent_obj; + + SMPCacheProperty props[SMP_CACHE__MAX]; +}; + +#endif /* SMP_CACHE_H */ 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: +# +# An enumeration of cache for SMP systems. The cache name here is +# a combination of cache level and cache type. +# +# @l1d: L1 data cache. +# +# @l1i: L1 instruction cache. +# +# @l2: L2 (unified) cache. +# +# @l3: L3 (unified) cache +# +# Since: 9.1 +## +{ 'enum': 'SMPCacheName', + 'prefix': 'SMP_CACHE', + 'data': [ 'l1d', 'l1i', 'l2', 'l3' ] } + +## +# @SMPCacheProperty: +# +# 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' } } + +## +# @SMPCacheProperties: +# +# List wrapper of SMPCacheProperty. +# +# @caches: the SMPCacheProperty list. +# +# Since 9.1 +## +{ 'struct': 'SMPCacheProperties', + 'data': { 'caches': ['SMPCacheProperty'] } } 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' } 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',
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> --- Suggested by credit: * Referred to Daniel's suggestion to introduce cache JSON list, though as a standalone object since -smp/-machine can't support JSON. --- Changes since RFC v2: * New commit to implement cache list with JSON format instead of multiple sub-options in -smp. --- MAINTAINERS | 2 + hw/core/meson.build | 1 + hw/core/smp-cache.c | 103 ++++++++++++++++++++++++++++++++++++ include/hw/core/smp-cache.h | 27 ++++++++++ qapi/machine-common.json | 50 +++++++++++++++++ qapi/qapi-schema.json | 4 +- qapi/qom.json | 3 ++ 7 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 hw/core/smp-cache.c create mode 100644 include/hw/core/smp-cache.h