| Message ID | 20240530101539.768484-4-zhao1.liu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce SMP Cache Topology | expand |
Zhao Liu <zhao1.liu@intel.com> writes: > Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in > -smp to define the cache topology for SMP system. > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> [...] > diff --git a/qapi/machine.json b/qapi/machine.json > index 7ac5a05bb9c9..8fa5af69b1bf 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -1746,6 +1746,23 @@ > # > # @threads: number of threads per core > # > +# @l1d-cache: topology hierarchy of L1 data cache. It accepts the CPU > +# topology enumeration as the parameter, i.e., CPUs in the same > +# topology container share the same L1 data cache. (since 9.1) > +# > +# @l1i-cache: topology hierarchy of L1 instruction cache. It accepts > +# the CPU topology enumeration as the parameter, i.e., CPUs in the > +# same topology container share the same L1 instruction cache. > +# (since 9.1) > +# > +# @l2-cache: topology hierarchy of L2 unified cache. It accepts the CPU > +# topology enumeration as the parameter, i.e., CPUs in the same > +# topology container share the same L2 unified cache. (since 9.1) > +# > +# @l3-cache: topology hierarchy of L3 unified cache. It accepts the CPU > +# topology enumeration as the parameter, i.e., CPUs in the same > +# topology container share the same L3 unified cache. (since 9.1) > +# > # Since: 6.1 > ## The new members are all optional. What does "absent" mean? No such cache? Some default topology? Is this sufficiently general? Do all machines of interest have a split level 1 cache, a level 2 cache, and a level 3 cache? Is the CPU topology level the only cache property we'll want to configure here? If the answer isn't "yes", then we should perhaps wrap it in an object, so we can easily add more members later. Two spaces between sentences for consistency, please. > { 'struct': 'SMPConfiguration', 'data': { > @@ -1758,7 +1775,11 @@ > '*modules': 'int', > '*cores': 'int', > '*threads': 'int', > - '*maxcpus': 'int' } } > + '*maxcpus': 'int', > + '*l1d-cache': 'CPUTopoLevel', > + '*l1i-cache': 'CPUTopoLevel', > + '*l2-cache': 'CPUTopoLevel', > + '*l3-cache': 'CPUTopoLevel' } } > > ## > # @x-query-irq: > diff --git a/system/vl.c b/system/vl.c [...]
On Tue, Jun 04, 2024 at 10:54:51AM +0200, Markus Armbruster wrote: > Zhao Liu <zhao1.liu@intel.com> writes: > > > Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in > > -smp to define the cache topology for SMP system. > > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > [...] > > > diff --git a/qapi/machine.json b/qapi/machine.json > > index 7ac5a05bb9c9..8fa5af69b1bf 100644 > > --- a/qapi/machine.json > > +++ b/qapi/machine.json > > @@ -1746,6 +1746,23 @@ > > # > > # @threads: number of threads per core > > # > > +# @l1d-cache: topology hierarchy of L1 data cache. It accepts the CPU > > +# topology enumeration as the parameter, i.e., CPUs in the same > > +# topology container share the same L1 data cache. (since 9.1) > > +# > > +# @l1i-cache: topology hierarchy of L1 instruction cache. It accepts > > +# the CPU topology enumeration as the parameter, i.e., CPUs in the > > +# same topology container share the same L1 instruction cache. > > +# (since 9.1) > > +# > > +# @l2-cache: topology hierarchy of L2 unified cache. It accepts the CPU > > +# topology enumeration as the parameter, i.e., CPUs in the same > > +# topology container share the same L2 unified cache. (since 9.1) > > +# > > +# @l3-cache: topology hierarchy of L3 unified cache. It accepts the CPU > > +# topology enumeration as the parameter, i.e., CPUs in the same > > +# topology container share the same L3 unified cache. (since 9.1) > > +# > > # Since: 6.1 > > ## > > The new members are all optional. What does "absent" mean? No such > cache? Some default topology? > > Is this sufficiently general? Do all machines of interest have a split > level 1 cache, a level 2 cache, and a level 3 cache? Level 4 cache is apparently a thing https://www.guru3d.com/story/intel-confirms-l4-cache-in-upcoming-meteor-lake-cpus/ but given that any new cache levels will require new code in QEMU to wire up, its not a big deal to add new properties at the same time. That said see my reply just now to the cover letter, where I suggest we should have a "caches" property that takes an array of cache info objects. > > Is the CPU topology level the only cache property we'll want to > configure here? If the answer isn't "yes", then we should perhaps wrap > it in an object, so we can easily add more members later. Cache size is a piece of info I could see us wanting to express > Two spaces between sentences for consistency, please. > > > { 'struct': 'SMPConfiguration', 'data': { > > @@ -1758,7 +1775,11 @@ > > '*modules': 'int', > > '*cores': 'int', > > '*threads': 'int', > > - '*maxcpus': 'int' } } > > + '*maxcpus': 'int', > > + '*l1d-cache': 'CPUTopoLevel', > > + '*l1i-cache': 'CPUTopoLevel', > > + '*l2-cache': 'CPUTopoLevel', > > + '*l3-cache': 'CPUTopoLevel' } } > > > > ## > > # @x-query-irq: > > diff --git a/system/vl.c b/system/vl.c > > [...] > With regards, Daniel
Hi Markus and Daniel, (I replied to both of you because I discussed the idea of cache object at the end) On Tue, Jun 04, 2024 at 10:32:14AM +0100, Daniel P. Berrangé wrote: > Date: Tue, 4 Jun 2024 10:32:14 +0100 > From: "Daniel P. Berrangé" <berrange@redhat.com> > Subject: Re: [RFC v2 3/7] hw/core: Add cache topology options in -smp > > On Tue, Jun 04, 2024 at 10:54:51AM +0200, Markus Armbruster wrote: > > Zhao Liu <zhao1.liu@intel.com> writes: > > > > > Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in > > > -smp to define the cache topology for SMP system. > > > > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > > > > [...] > > > > > diff --git a/qapi/machine.json b/qapi/machine.json > > > index 7ac5a05bb9c9..8fa5af69b1bf 100644 > > > --- a/qapi/machine.json > > > +++ b/qapi/machine.json > > > @@ -1746,6 +1746,23 @@ > > > # > > > # @threads: number of threads per core > > > # > > > +# @l1d-cache: topology hierarchy of L1 data cache. It accepts the CPU > > > +# topology enumeration as the parameter, i.e., CPUs in the same > > > +# topology container share the same L1 data cache. (since 9.1) > > > +# > > > +# @l1i-cache: topology hierarchy of L1 instruction cache. It accepts > > > +# the CPU topology enumeration as the parameter, i.e., CPUs in the > > > +# same topology container share the same L1 instruction cache. > > > +# (since 9.1) > > > +# > > > +# @l2-cache: topology hierarchy of L2 unified cache. It accepts the CPU > > > +# topology enumeration as the parameter, i.e., CPUs in the same > > > +# topology container share the same L2 unified cache. (since 9.1) > > > +# > > > +# @l3-cache: topology hierarchy of L3 unified cache. It accepts the CPU > > > +# topology enumeration as the parameter, i.e., CPUs in the same > > > +# topology container share the same L3 unified cache. (since 9.1) > > > +# > > > # Since: 6.1 > > > ## > > > > The new members are all optional. What does "absent" mean? No such > > cache? Some default topology? I suppose "absent" means using the the default arch-specific cache topo. For example, x86 has its own default cache topo. my purpose in providing this interface is to allow users to override the default (arch-specific) cache topology. Daniel suggested in cover letter's reply that I could add the value “default”, I think omitting it would have the same effect as setting it to “default”, and omitting it shouldn't be regarded as disabling a particular cache, since the interface is only about the topology! > > Is this sufficiently general? Do all machines of interest have a split > > level 1 cache, a level 2 cache, and a level 3 cache? The different cache support can be extended by such control fields in MachineClass: typedef struct { ... bool l1_separated_cache_supported; bool l2_unified_cache_supported; bool l3_unified_cache_supported; } SMPCompatProps; For example, if a machine with l1 unified cache appears, it can add the "l1_unified_cache_supported", and add "l1" option in QAPI. Then separate L1 cache has “l1i” and “l1d” options, and unified L1 cache should have “l1” option. > Level 4 cache is apparently a thing > > https://www.guru3d.com/story/intel-confirms-l4-cache-in-upcoming-meteor-lake-cpus/ > > but given that any new cache levels will require new code in QEMU to > wire up, its not a big deal to add new properties at the same time. > > That said see my reply just now to the cover letter, where I suggest > we should have a "caches" property that takes an array of cache > info objects. Yes, I agree. > > > > Is the CPU topology level the only cache property we'll want to > > configure here? If the answer isn't "yes", then we should perhaps wrap > > it in an object, so we can easily add more members later. > > Cache size is a piece of info I could see us wanting to express For the simplicity and clarity, I think it's better to only cover topology in -smp, including the current CPU topology and the cache topology I'm working on. I've thought about the idea of converting the cache into the device, which in turn could define more properties for the cache. This one is mostly in connection with the previous qom-topo idea (aka abstracting CPU topology device [1]): -device cpu-socket,id=sock0 \ -device cpu-die,id=die0,parent=sock0 \ -device cpu-core,id=core0,parent=die0,nr-threads=2 \ -device cpu-cache,id=cache0,parent=core0,level=l1,type=inst \ -device cpu-cache,id=cache1,parent=core0,level=l1,type=data \ -device cpu-cache,id=cache2,parent=core0,level=l2,type=unif \ -device cpu-cache,id=cache3,parent=die0,level=l3,type=unif Cache device idea was mainly to support hybrid cache topology, because Intel client hybrid architecture has hybrid cache topology (on MTL, compute die has a L3 and SOC die has no L3). Based on such a cache device, we can define more properties for the cache and also meet flexible topology requirements at the same time. This cache device I had planned to implement after the cpu topo device. It's a long and hard way to go, I wonder if this future I'm portraying will alleviate your concerns about more cache properties support. ;-) Soon I'm also planning to refresh the qom-topo series again, reducing hack changes, deleting dead codes, etc. Maybe it shouldn't be called qom-topo, because I want to display topology tree in qom path by qdev_set_id(). [1]: https://lore.kernel.org/qemu-devel/20231130144203.2307629-1-zhao1.liu@linux.intel.com/ > > Two spaces between sentences for consistency, please. > > Sure, thanks! Regards, Zhao
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c index 5d8d7edcbd3f..c79464cf3d2c 100644 --- a/hw/core/machine-smp.c +++ b/hw/core/machine-smp.c @@ -61,6 +61,150 @@ static char *cpu_hierarchy_to_string(MachineState *ms) return g_string_free(s, false); } +static bool machine_check_topo_support(MachineState *ms, + CPUTopoLevel topo) +{ + MachineClass *mc = MACHINE_GET_CLASS(ms); + + if (topo == CPU_TOPO_LEVEL_MODULE && !mc->smp_props.modules_supported) { + return false; + } + + if (topo == CPU_TOPO_LEVEL_CLUSTER && !mc->smp_props.clusters_supported) { + return false; + } + + if (topo == CPU_TOPO_LEVEL_DIE && !mc->smp_props.dies_supported) { + return false; + } + + if (topo == CPU_TOPO_LEVEL_BOOK && !mc->smp_props.books_supported) { + return false; + } + + if (topo == CPU_TOPO_LEVEL_DRAWER && !mc->smp_props.drawers_supported) { + return false; + } + + return true; +} + +static bool machine_check_cache_topo(MachineState *ms, + CPUTopoLevel topo, + Error **errp) +{ + if (topo == CPU_TOPO_LEVEL__MAX || topo == CPU_TOPO_LEVEL_INVALID) { + error_setg(errp, + "Invalid cache topology level: %s. " + "The cache topology should match the " + "valid CPU topology level", + cpu_topo_to_string(topo)); + return false; + } + + if (!machine_check_topo_support(ms, topo)) { + error_setg(errp, + "Invalid cache topology level: %s. " + "The topology level is not supported by this machine", + cpu_topo_to_string(topo)); + return false; + } + + return true; +} + +static void machine_parse_smp_cache_config(MachineState *ms, + const SMPConfiguration *config, + Error **errp) +{ + MachineClass *mc = MACHINE_GET_CLASS(ms); + + /* + * The cache topology does not support a default entry similar to + * CPU topology with parameters=1. So when the machine explicitly + * does not support cache topology, return the error. + */ + if (config->has_l1d_cache) { + if (!mc->smp_props.l1_separated_cache_supported) { + error_setg(errp, + "L1 D-cache topology not supported by this machine"); + return; + } + + if (!machine_check_cache_topo(ms, config->l1d_cache, errp)) { + return; + } + + ms->smp_cache.l1d = config->l1d_cache; + } + + if (config->has_l1i_cache) { + if (!mc->smp_props.l1_separated_cache_supported) { + error_setg(errp, + "L1 I-cache topology not supported by this machine"); + return; + } + + if (!machine_check_cache_topo(ms, config->l1i_cache, errp)) { + return; + } + + ms->smp_cache.l1i = config->l1i_cache; + } + + if (config->has_l2_cache) { + if (!mc->smp_props.l2_unified_cache_supported) { + error_setg(errp, + "L2 cache topology not supported by this machine"); + return; + } + + if (!machine_check_cache_topo(ms, config->l2_cache, errp)) { + return; + } + + ms->smp_cache.l2 = config->l2_cache; + + /* + * Cache topology is initialized by default to CPU_TOPO_LEVEL_INVALID, + * which is the lowest level, so such a check is OK, even if the config + * doesn't override that field. + */ + if (ms->smp_cache.l1d > ms->smp_cache.l2 || + ms->smp_cache.l1i > ms->smp_cache.l2) { + error_setg(errp, + "Invalid L2 cache topology. " + "L2 cache topology level should not be lower than " + "L1 D-cache/L1 I-cache"); + return; + } + } + + if (config->has_l3_cache) { + if (!mc->smp_props.l2_unified_cache_supported) { + error_setg(errp, + "L3 cache topology not supported by this machine"); + return; + } + + if (!machine_check_cache_topo(ms, config->l3_cache, errp)) { + return; + } + + ms->smp_cache.l3 = config->l3_cache; + + if (ms->smp_cache.l1d > ms->smp_cache.l3 || + ms->smp_cache.l1i > ms->smp_cache.l3 || + ms->smp_cache.l2 > ms->smp_cache.l3) { + error_setg(errp, + "Invalid L3 cache topology. " + "L3 cache topology level should not be lower than " + "L1 D-cache/L1 I-cache/L2 cache"); + return; + } + } +} + /* * machine_parse_smp_config: Generic function used to parse the given * SMP configuration @@ -259,6 +403,8 @@ void machine_parse_smp_config(MachineState *ms, mc->name, mc->max_cpus); return; } + + machine_parse_smp_cache_config(ms, config, errp); } unsigned int machine_topo_get_cores_per_socket(const MachineState *ms) diff --git a/hw/core/machine.c b/hw/core/machine.c index e31d0f3cb4b0..f705485f83c0 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -900,6 +900,26 @@ static void machine_get_smp(Object *obj, Visitor *v, const char *name, .has_maxcpus = true, .maxcpus = ms->smp.max_cpus, }; + if (ms->smp_cache.l1d != CPU_TOPO_LEVEL_INVALID) { + config->has_l1d_cache = true; + config->l1d_cache = ms->smp_cache.l1d; + } + + if (ms->smp_cache.l1i != CPU_TOPO_LEVEL_INVALID) { + config->has_l1i_cache = true; + config->l1i_cache = ms->smp_cache.l1i; + } + + if (ms->smp_cache.l2 != CPU_TOPO_LEVEL_INVALID) { + config->has_l2_cache = true; + config->l2_cache = ms->smp_cache.l2; + } + + if (ms->smp_cache.l3 != CPU_TOPO_LEVEL_INVALID) { + config->has_l3_cache = true; + config->l3_cache = ms->smp_cache.l3; + } + if (!visit_type_SMPConfiguration(v, name, &config, &error_abort)) { return; } diff --git a/qapi/machine.json b/qapi/machine.json index 7ac5a05bb9c9..8fa5af69b1bf 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -1746,6 +1746,23 @@ # # @threads: number of threads per core # +# @l1d-cache: topology hierarchy of L1 data cache. It accepts the CPU +# topology enumeration as the parameter, i.e., CPUs in the same +# topology container share the same L1 data cache. (since 9.1) +# +# @l1i-cache: topology hierarchy of L1 instruction cache. It accepts +# the CPU topology enumeration as the parameter, i.e., CPUs in the +# same topology container share the same L1 instruction cache. +# (since 9.1) +# +# @l2-cache: topology hierarchy of L2 unified cache. It accepts the CPU +# topology enumeration as the parameter, i.e., CPUs in the same +# topology container share the same L2 unified cache. (since 9.1) +# +# @l3-cache: topology hierarchy of L3 unified cache. It accepts the CPU +# topology enumeration as the parameter, i.e., CPUs in the same +# topology container share the same L3 unified cache. (since 9.1) +# # Since: 6.1 ## { 'struct': 'SMPConfiguration', 'data': { @@ -1758,7 +1775,11 @@ '*modules': 'int', '*cores': 'int', '*threads': 'int', - '*maxcpus': 'int' } } + '*maxcpus': 'int', + '*l1d-cache': 'CPUTopoLevel', + '*l1i-cache': 'CPUTopoLevel', + '*l2-cache': 'CPUTopoLevel', + '*l3-cache': 'CPUTopoLevel' } } ## # @x-query-irq: diff --git a/system/vl.c b/system/vl.c index a3eede5fa5b8..c7c94d41bd01 100644 --- a/system/vl.c +++ b/system/vl.c @@ -753,6 +753,18 @@ static QemuOptsList qemu_smp_opts = { }, { .name = "maxcpus", .type = QEMU_OPT_NUMBER, + }, { + .name = "l1d-cache", + .type = QEMU_OPT_STRING, + }, { + .name = "l1i-cache", + .type = QEMU_OPT_STRING, + }, { + .name = "l2-cache", + .type = QEMU_OPT_STRING, + }, { + .name = "l3-cache", + .type = QEMU_OPT_STRING, }, { /*End of list */ } }, diff --git a/tests/unit/meson.build b/tests/unit/meson.build index 26c109c968ce..8877dbbc00c9 100644 --- a/tests/unit/meson.build +++ b/tests/unit/meson.build @@ -138,7 +138,8 @@ if have_system 'test-util-sockets': ['socket-helpers.c'], 'test-base64': [], 'test-bufferiszero': [], - 'test-smp-parse': [qom, meson.project_source_root() / 'hw/core/machine-smp.c'], + 'test-smp-parse': [qom, meson.project_source_root() / 'hw/core/machine-smp.c', + meson.project_source_root() / 'hw/core/cpu-topology.c'], 'test-vmstate': [migration, io], 'test-yank': ['socket-helpers.c', qom, io, chardev] }
Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in -smp to define the cache topology for SMP system. Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- Changes since RFC v1: * Set has_*_cache field in machine_get_smp(). (JeeHeng) * Adjust string breaking style in error_setg() for more semantic sentence breaking conventions. (Jonathan) * Add more description about cache options. (Markus) * Now in v2, config->*_cache field stores topology enumeration instead of string, no need to parse, so just make machine_check_cache_topo() return boolean. --- hw/core/machine-smp.c | 146 +++++++++++++++++++++++++++++++++++++++++ hw/core/machine.c | 20 ++++++ qapi/machine.json | 23 ++++++- system/vl.c | 12 ++++ tests/unit/meson.build | 3 +- 5 files changed, 202 insertions(+), 2 deletions(-)