From: Zhao Liu <zhao1.liu@intel.com>
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>
---
hw/core/machine-smp.c | 128 ++++++++++++++++++++++++++++++++++++++++++
hw/core/machine.c | 4 ++
qapi/machine.json | 14 ++++-
system/vl.c | 15 +++++
4 files changed, 160 insertions(+), 1 deletion(-)
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 8a8296b0d05b..2cbd19f4aa57 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -61,6 +61,132 @@ 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 int smp_cache_string_to_topology(MachineState *ms,
+ char *topo_str,
+ CPUTopoLevel *topo,
+ Error **errp)
+{
+ *topo = string_to_cpu_topo(topo_str);
+
+ 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 CPU topology level", topo_str);
+ return -1;
+ }
+
+ if (!machine_check_topo_support(ms, *topo)) {
+ error_setg(errp, "Invalid cache topology level: %s. The topology "
+ "level is not supported by this machine", topo_str);
+ return -1;
+ }
+
+ return 0;
+}
+
+static void machine_parse_smp_cache_config(MachineState *ms,
+ const SMPConfiguration *config,
+ Error **errp)
+{
+ MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+ if (config->l1d_cache) {
+ if (!mc->smp_props.l1_separated_cache_supported) {
+ error_setg(errp, "L1 D-cache topology not "
+ "supported by this machine");
+ return;
+ }
+
+ if (smp_cache_string_to_topology(ms, config->l1d_cache,
+ &ms->smp_cache.l1d, errp)) {
+ return;
+ }
+ }
+
+ if (config->l1i_cache) {
+ if (!mc->smp_props.l1_separated_cache_supported) {
+ error_setg(errp, "L1 I-cache topology not "
+ "supported by this machine");
+ return;
+ }
+
+ if (smp_cache_string_to_topology(ms, config->l1i_cache,
+ &ms->smp_cache.l1i, errp)) {
+ return;
+ }
+ }
+
+ if (config->l2_cache) {
+ if (!mc->smp_props.l2_unified_cache_supported) {
+ error_setg(errp, "L2 cache topology not "
+ "supported by this machine");
+ return;
+ }
+
+ if (smp_cache_string_to_topology(ms, config->l2_cache,
+ &ms->smp_cache.l2, errp)) {
+ return;
+ }
+
+ 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->l3_cache) {
+ if (!mc->smp_props.l2_unified_cache_supported) {
+ error_setg(errp, "L3 cache topology not "
+ "supported by this machine");
+ return;
+ }
+
+ if (smp_cache_string_to_topology(ms, config->l3_cache,
+ &ms->smp_cache.l3, errp)) {
+ return;
+ }
+
+ 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
@@ -249,6 +375,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 426f71770a84..cb5173927b0d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -886,6 +886,10 @@ static void machine_get_smp(Object *obj, Visitor *v, const char *name,
.has_cores = true, .cores = ms->smp.cores,
.has_threads = true, .threads = ms->smp.threads,
.has_maxcpus = true, .maxcpus = ms->smp.max_cpus,
+ .l1d_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l1d)),
+ .l1i_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l1i)),
+ .l2_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l2)),
+ .l3_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l3)),
};
if (!visit_type_SMPConfiguration(v, name, &config, &error_abort)) {
diff --git a/qapi/machine.json b/qapi/machine.json
index d0e7f1f615f3..0a923ac38803 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1650,6 +1650,14 @@
#
# @threads: number of threads per core
#
+# @l1d-cache: topology hierarchy of L1 data cache (since 9.0)
+#
+# @l1i-cache: topology hierarchy of L1 instruction cache (since 9.0)
+#
+# @l2-cache: topology hierarchy of L2 unified cache (since 9.0)
+#
+# @l3-cache: topology hierarchy of L3 unified cache (since 9.0)
+#
# Since: 6.1
##
{ 'struct': 'SMPConfiguration', 'data': {
@@ -1662,7 +1670,11 @@
'*modules': 'int',
'*cores': 'int',
'*threads': 'int',
- '*maxcpus': 'int' } }
+ '*maxcpus': 'int',
+ '*l1d-cache': 'str',
+ '*l1i-cache': 'str',
+ '*l2-cache': 'str',
+ '*l3-cache': 'str' } }
##
# @x-query-irq:
diff --git a/system/vl.c b/system/vl.c
index a82555ae1558..ac95e5ddb656 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -741,6 +741,9 @@ static QemuOptsList qemu_smp_opts = {
}, {
.name = "clusters",
.type = QEMU_OPT_NUMBER,
+ }, {
+ .name = "modules",
+ .type = QEMU_OPT_NUMBER,
}, {
.name = "cores",
.type = QEMU_OPT_NUMBER,
@@ -750,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 */ }
},
--
2.34.1
> -----Original Message-----
> From: Zhao Liu <zhao1.liu@linux.intel.com>
> Sent: Tuesday, February 20, 2024 5:25 PM
> To: Daniel P . Berrangé <berrange@redhat.com>; Eduardo Habkost <eduardo@habkost.net>; Marcel Apfelbaum
> <marcel.apfelbaum@gmail.com>; Philippe Mathieu-Daudé <philmd@linaro.org>; Yanan Wang <wangyanan55@huawei.com>;
> Michael S . Tsirkin <mst@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson <richard.henderson@linaro.org>;
> Eric Blake <eblake@redhat.com>; Markus Armbruster <armbru@redhat.com>; Marcelo Tosatti <mtosatti@redhat.com>; Alex Bennée
> <alex.bennee@linaro.org>; Peter Maydell <peter.maydell@linaro.org>; Jonathan Cameron <Jonathan.Cameron@huawei.com>;
> JeeHeng Sia <jeeheng.sia@starfivetech.com>
> Cc: qemu-devel@nongnu.org; kvm@vger.kernel.org; qemu-riscv@nongnu.org; qemu-arm@nongnu.org; Zhenyu Wang
> <zhenyu.z.wang@intel.com>; Dapeng Mi <dapeng1.mi@linux.intel.com>; Yongwei Ma <yongwei.ma@intel.com>; Zhao Liu
> <zhao1.liu@intel.com>
> Subject: [RFC 4/8] hw/core: Add cache topology options in -smp
>
> From: Zhao Liu <zhao1.liu@intel.com>
>
> 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>
> ---
> hw/core/machine-smp.c | 128 ++++++++++++++++++++++++++++++++++++++++++
> hw/core/machine.c | 4 ++
> qapi/machine.json | 14 ++++-
> system/vl.c | 15 +++++
> 4 files changed, 160 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 8a8296b0d05b..2cbd19f4aa57 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -61,6 +61,132 @@ 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 int smp_cache_string_to_topology(MachineState *ms,
> + char *topo_str,
> + CPUTopoLevel *topo,
> + Error **errp)
> +{
> + *topo = string_to_cpu_topo(topo_str);
> +
> + 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 CPU topology level", topo_str);
> + return -1;
> + }
> +
> + if (!machine_check_topo_support(ms, *topo)) {
> + error_setg(errp, "Invalid cache topology level: %s. The topology "
> + "level is not supported by this machine", topo_str);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static void machine_parse_smp_cache_config(MachineState *ms,
> + const SMPConfiguration *config,
> + Error **errp)
> +{
> + MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> + if (config->l1d_cache) {
> + if (!mc->smp_props.l1_separated_cache_supported) {
> + error_setg(errp, "L1 D-cache topology not "
> + "supported by this machine");
> + return;
> + }
> +
> + if (smp_cache_string_to_topology(ms, config->l1d_cache,
> + &ms->smp_cache.l1d, errp)) {
> + return;
> + }
> + }
> +
> + if (config->l1i_cache) {
> + if (!mc->smp_props.l1_separated_cache_supported) {
> + error_setg(errp, "L1 I-cache topology not "
> + "supported by this machine");
> + return;
> + }
> +
> + if (smp_cache_string_to_topology(ms, config->l1i_cache,
> + &ms->smp_cache.l1i, errp)) {
> + return;
> + }
> + }
> +
> + if (config->l2_cache) {
> + if (!mc->smp_props.l2_unified_cache_supported) {
> + error_setg(errp, "L2 cache topology not "
> + "supported by this machine");
> + return;
> + }
> +
> + if (smp_cache_string_to_topology(ms, config->l2_cache,
> + &ms->smp_cache.l2, errp)) {
> + return;
> + }
> +
> + 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->l3_cache) {
> + if (!mc->smp_props.l2_unified_cache_supported) {
> + error_setg(errp, "L3 cache topology not "
> + "supported by this machine");
> + return;
> + }
> +
> + if (smp_cache_string_to_topology(ms, config->l3_cache,
> + &ms->smp_cache.l3, errp)) {
> + return;
> + }
> +
> + 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
> @@ -249,6 +375,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 426f71770a84..cb5173927b0d 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -886,6 +886,10 @@ static void machine_get_smp(Object *obj, Visitor *v, const char *name,
> .has_cores = true, .cores = ms->smp.cores,
> .has_threads = true, .threads = ms->smp.threads,
> .has_maxcpus = true, .maxcpus = ms->smp.max_cpus,
> + .l1d_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l1d)),
> + .l1i_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l1i)),
> + .l2_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l2)),
> + .l3_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l3)),
Let's standardize the code by adding the 'has_' prefix.
> };
>
> if (!visit_type_SMPConfiguration(v, name, &config, &error_abort)) {
> diff --git a/qapi/machine.json b/qapi/machine.json
> index d0e7f1f615f3..0a923ac38803 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1650,6 +1650,14 @@
> #
> # @threads: number of threads per core
> #
> +# @l1d-cache: topology hierarchy of L1 data cache (since 9.0)
> +#
> +# @l1i-cache: topology hierarchy of L1 instruction cache (since 9.0)
> +#
> +# @l2-cache: topology hierarchy of L2 unified cache (since 9.0)
> +#
> +# @l3-cache: topology hierarchy of L3 unified cache (since 9.0)
> +#
> # Since: 6.1
> ##
> { 'struct': 'SMPConfiguration', 'data': {
> @@ -1662,7 +1670,11 @@
> '*modules': 'int',
> '*cores': 'int',
> '*threads': 'int',
> - '*maxcpus': 'int' } }
> + '*maxcpus': 'int',
> + '*l1d-cache': 'str',
> + '*l1i-cache': 'str',
> + '*l2-cache': 'str',
> + '*l3-cache': 'str' } }
>
> ##
> # @x-query-irq:
> diff --git a/system/vl.c b/system/vl.c
> index a82555ae1558..ac95e5ddb656 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -741,6 +741,9 @@ static QemuOptsList qemu_smp_opts = {
> }, {
> .name = "clusters",
> .type = QEMU_OPT_NUMBER,
> + }, {
> + .name = "modules",
> + .type = QEMU_OPT_NUMBER,
> }, {
> .name = "cores",
> .type = QEMU_OPT_NUMBER,
> @@ -750,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 */ }
> },
> --
> 2.34.1
Hi JeeHeng,
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 426f71770a84..cb5173927b0d 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -886,6 +886,10 @@ static void machine_get_smp(Object *obj, Visitor *v, const char *name,
> > .has_cores = true, .cores = ms->smp.cores,
> > .has_threads = true, .threads = ms->smp.threads,
> > .has_maxcpus = true, .maxcpus = ms->smp.max_cpus,
> > + .l1d_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l1d)),
> > + .l1i_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l1i)),
> > + .l2_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l2)),
> > + .l3_cache = g_strdup(cpu_topo_to_string(ms->smp_cache.l3)),
>
> Let's standardize the code by adding the 'has_' prefix.
SMPConfiguration is automatically generated in the compilation, and its
prototype is defined in qapi/machine.json like the following code:
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index d0e7f1f615f3..0a923ac38803 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1650,6 +1650,14 @@
> > #
> > # @threads: number of threads per core
> > #
> > +# @l1d-cache: topology hierarchy of L1 data cache (since 9.0)
> > +#
> > +# @l1i-cache: topology hierarchy of L1 instruction cache (since 9.0)
> > +#
> > +# @l2-cache: topology hierarchy of L2 unified cache (since 9.0)
> > +#
> > +# @l3-cache: topology hierarchy of L3 unified cache (since 9.0)
> > +#
> > # Since: 6.1
> > ##
> > { 'struct': 'SMPConfiguration', 'data': {
> > @@ -1662,7 +1670,11 @@
> > '*modules': 'int',
> > '*cores': 'int',
> > '*threads': 'int',
> > - '*maxcpus': 'int' } }
> > + '*maxcpus': 'int',
> > + '*l1d-cache': 'str',
> > + '*l1i-cache': 'str',
> > + '*l2-cache': 'str',
> > + '*l3-cache': 'str' } }
> >
The gnerated complete structure is (will in build/qapi/qapi-types-machine.h):
struct SMPConfiguration {
bool has_cpus;
int64_t cpus;
bool has_drawers;
int64_t drawers;
bool has_books;
int64_t books;
bool has_sockets;
int64_t sockets;
bool has_dies;
int64_t dies;
bool has_clusters;
int64_t clusters;
bool has_modules;
int64_t modules;
bool has_cores;
int64_t cores;
bool has_threads;
int64_t threads;
bool has_maxcpus;
int64_t maxcpus;
char *l1d_cache;
char *l1i_cache;
char *l2_cache;
char *l3_cache;
};
The int member defined in qapi/machine.json will get their corresponding
status fields as has_* to indicate if user sets those int fields.
For str type, the status field is not needed since NULL is enough to
indicate no user sets that.
Thanks,
Zhao
On Tue, 20 Feb 2024 17:25:00 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> 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>
Hi Zhao Liu
I like the scheme. Strikes a good balance between complexity of description
and systems that actually exist. Sure there are systems with more cache
levels etc but they are rare and support can be easily added later
if people want to model them.
A few minor comments inline.
Jonathan
> ---
> hw/core/machine-smp.c | 128 ++++++++++++++++++++++++++++++++++++++++++
> hw/core/machine.c | 4 ++
> qapi/machine.json | 14 ++++-
> system/vl.c | 15 +++++
> 4 files changed, 160 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index 8a8296b0d05b..2cbd19f4aa57 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -61,6 +61,132 @@ 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 int smp_cache_string_to_topology(MachineState *ms,
Not a good name for a function that does rather more than that.
> + char *topo_str,
> + CPUTopoLevel *topo,
> + Error **errp)
> +{
> + *topo = string_to_cpu_topo(topo_str);
> +
> + 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 CPU topology level", topo_str);
> + return -1;
> + }
> +
> + if (!machine_check_topo_support(ms, *topo)) {
> + error_setg(errp, "Invalid cache topology level: %s. The topology "
> + "level is not supported by this machine", topo_str);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static void machine_parse_smp_cache_config(MachineState *ms,
> + const SMPConfiguration *config,
> + Error **errp)
> +{
> + MachineClass *mc = MACHINE_GET_CLASS(ms);
> +
> + if (config->l1d_cache) {
> + if (!mc->smp_props.l1_separated_cache_supported) {
> + error_setg(errp, "L1 D-cache topology not "
> + "supported by this machine");
> + return;
> + }
> +
> + if (smp_cache_string_to_topology(ms, config->l1d_cache,
> + &ms->smp_cache.l1d, errp)) {
Indent is to wrong opening bracket.
Same for other cases.
> + return;
> + }
> + }
> +}
> +
> /*
> * machine_parse_smp_config: Generic function used to parse the given
> * SMP configuration
> @@ -249,6 +375,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)
Hi Jonathan,
> Hi Zhao Liu
>
> I like the scheme. Strikes a good balance between complexity of description
> and systems that actually exist. Sure there are systems with more cache
> levels etc but they are rare and support can be easily added later
> if people want to model them.
Thanks for your support!
[snip]
> > +static int smp_cache_string_to_topology(MachineState *ms,
>
> Not a good name for a function that does rather more than that.
What about "smp_cache_get_valid_topology()"?
>
> > + char *topo_str,
> > + CPUTopoLevel *topo,
> > + Error **errp)
> > +{
> > + *topo = string_to_cpu_topo(topo_str);
> > +
> > + 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 CPU topology level", topo_str);
> > + return -1;
> > + }
> > +
> > + if (!machine_check_topo_support(ms, *topo)) {
> > + error_setg(errp, "Invalid cache topology level: %s. The topology "
> > + "level is not supported by this machine", topo_str);
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void machine_parse_smp_cache_config(MachineState *ms,
> > + const SMPConfiguration *config,
> > + Error **errp)
> > +{
> > + MachineClass *mc = MACHINE_GET_CLASS(ms);
> > +
> > + if (config->l1d_cache) {
> > + if (!mc->smp_props.l1_separated_cache_supported) {
> > + error_setg(errp, "L1 D-cache topology not "
> > + "supported by this machine");
> > + return;
> > + }
> > +
> > + if (smp_cache_string_to_topology(ms, config->l1d_cache,
> > + &ms->smp_cache.l1d, errp)) {
>
> Indent is to wrong opening bracket.
> Same for other cases.
Could you please educate me about the correct style here?
I'm unsure if it should be indented by 4 spaces.
Thanks,
Zhao
On Tue, 27 Feb 2024 17:20:25 +0800
Zhao Liu <zhao1.liu@linux.intel.com> wrote:
> Hi Jonathan,
>
> > Hi Zhao Liu
> >
> > I like the scheme. Strikes a good balance between complexity of description
> > and systems that actually exist. Sure there are systems with more cache
> > levels etc but they are rare and support can be easily added later
> > if people want to model them.
>
> Thanks for your support!
>
> [snip]
>
> > > +static int smp_cache_string_to_topology(MachineState *ms,
> >
> > Not a good name for a function that does rather more than that.
>
> What about "smp_cache_get_valid_topology()"?
Looking again, could we return the CPUTopoLevel? I think returning
CPU_TOPO_LEVEL_INVALID will replace -1/0 returns and this can just
be smp_cache_string_to_topology() as you have it in this version.
The check on the return value becomes a little more more complex
and I think you want to squash CPU_TOPO_LEVEL_MAX down so we only
have one invalid value to check at callers.. E.g.
static CPUTopoLevel smp_cache_string_to_topolgy(MachineState *ms,
char *top_str,
Error **errp)
{
CPUTopoLevel topo = string_to_cpu_topo(topo_str);
if (topo == CPU_TOPO_LEVEL_MAX || topo == CPU_TOP?O_LEVEL_INVALID) {
return CPU_TOPO_LEVEL_INVALID;
}
if (!machine_check_topo_support(ms, topo) {
error_setg(errp,
"Invalid cache topology level: %s. "
"The cache topology should match the CPU topology level",
//Break string like this to make it as grep-able as possible!
topo_str);
return CPU_TOPO_LEVEL_INVALID;
}
return topo;
}
The checks then become != CPU_TOPO_LEVEL_INVALID at each callsite.
Jonathan
On Tue, Feb 27, 2024 at 10:51:45AM +0000, Jonathan Cameron wrote:
> Date: Tue, 27 Feb 2024 10:51:45 +0000
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Subject: Re: [RFC 4/8] hw/core: Add cache topology options in -smp
> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32)
>
> On Tue, 27 Feb 2024 17:20:25 +0800
> Zhao Liu <zhao1.liu@linux.intel.com> wrote:
>
> > Hi Jonathan,
> >
> > > Hi Zhao Liu
> > >
> > > I like the scheme. Strikes a good balance between complexity of description
> > > and systems that actually exist. Sure there are systems with more cache
> > > levels etc but they are rare and support can be easily added later
> > > if people want to model them.
> >
> > Thanks for your support!
> >
> > [snip]
> >
> > > > +static int smp_cache_string_to_topology(MachineState *ms,
> > >
> > > Not a good name for a function that does rather more than that.
> >
> > What about "smp_cache_get_valid_topology()"?
>
> Looking again, could we return the CPUTopoLevel? I think returning
> CPU_TOPO_LEVEL_INVALID will replace -1/0 returns and this can just
> be smp_cache_string_to_topology() as you have it in this version.
>
> The check on the return value becomes a little more more complex
> and I think you want to squash CPU_TOPO_LEVEL_MAX down so we only
> have one invalid value to check at callers.. E.g.
>
> static CPUTopoLevel smp_cache_string_to_topolgy(MachineState *ms,
> char *top_str,
> Error **errp)
> {
> CPUTopoLevel topo = string_to_cpu_topo(topo_str);
>
> if (topo == CPU_TOPO_LEVEL_MAX || topo == CPU_TOP?O_LEVEL_INVALID) {
> return CPU_TOPO_LEVEL_INVALID;
> }
>
> if (!machine_check_topo_support(ms, topo) {
> error_setg(errp,
> "Invalid cache topology level: %s. "
> "The cache topology should match the CPU topology level",
> //Break string like this to make it as grep-able as possible!
> topo_str);
> return CPU_TOPO_LEVEL_INVALID;
> }
>
> return topo;
>
> }
>
>
> The checks then become != CPU_TOPO_LEVEL_INVALID at each callsite.
>
Good idea! This makes the code clearer. Let me try this way. Thanks!
On Tue, Feb 27, 2024 at 05:20:25PM +0800, Zhao Liu wrote:
> Hi Jonathan,
>
> > Hi Zhao Liu
> >
> > I like the scheme. Strikes a good balance between complexity of description
> > and systems that actually exist. Sure there are systems with more cache
> > levels etc but they are rare and support can be easily added later
> > if people want to model them.
>
> Thanks for your support!
>
> [snip]
>
> > > +static int smp_cache_string_to_topology(MachineState *ms,
> >
> > Not a good name for a function that does rather more than that.
>
> What about "smp_cache_get_valid_topology()"?
>
> >
> > > + char *topo_str,
> > > + CPUTopoLevel *topo,
> > > + Error **errp)
> > > +{
> > > + *topo = string_to_cpu_topo(topo_str);
> > > +
> > > + 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 CPU topology level", topo_str);
> > > + return -1;
> > > + }
> > > +
> > > + if (!machine_check_topo_support(ms, *topo)) {
> > > + error_setg(errp, "Invalid cache topology level: %s. The topology "
> > > + "level is not supported by this machine", topo_str);
> > > + return -1;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void machine_parse_smp_cache_config(MachineState *ms,
> > > + const SMPConfiguration *config,
> > > + Error **errp)
> > > +{
> > > + MachineClass *mc = MACHINE_GET_CLASS(ms);
> > > +
> > > + if (config->l1d_cache) {
> > > + if (!mc->smp_props.l1_separated_cache_supported) {
> > > + error_setg(errp, "L1 D-cache topology not "
> > > + "supported by this machine");
> > > + return;
> > > + }
> > > +
> > > + if (smp_cache_string_to_topology(ms, config->l1d_cache,
> > > + &ms->smp_cache.l1d, errp)) {
> >
> > Indent is to wrong opening bracket.
> > Same for other cases.
>
> Could you please educate me about the correct style here?
> I'm unsure if it should be indented by 4 spaces.
It needs to look like this:
if (smp_cache_string_to_topology(ms, config->l1d_cache,
&ms->smp_cache.l1d, errp)) {
so func parameters are aligned to the function calls' opening bracket,
not the 'if' statement's opening bracket.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
> > > > + if (smp_cache_string_to_topology(ms, config->l1d_cache,
> > > > + &ms->smp_cache.l1d, errp)) {
> > >
> > > Indent is to wrong opening bracket.
> > > Same for other cases.
> >
> > Could you please educate me about the correct style here?
> > I'm unsure if it should be indented by 4 spaces.
>
> It needs to look like this:
>
> if (smp_cache_string_to_topology(ms, config->l1d_cache,
> &ms->smp_cache.l1d, errp)) {
>
> so func parameters are aligned to the function calls' opening bracket,
> not the 'if' statement's opening bracket.
>
Thanks for your explaination!
Regards,
Zhao
Zhao Liu <zhao1.liu@linux.intel.com> writes:
> From: Zhao Liu <zhao1.liu@intel.com>
>
> 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 d0e7f1f615f3..0a923ac38803 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1650,6 +1650,14 @@
> #
> # @threads: number of threads per core
> #
> +# @l1d-cache: topology hierarchy of L1 data cache (since 9.0)
> +#
> +# @l1i-cache: topology hierarchy of L1 instruction cache (since 9.0)
> +#
> +# @l2-cache: topology hierarchy of L2 unified cache (since 9.0)
> +#
> +# @l3-cache: topology hierarchy of L3 unified cache (since 9.0)
> +#
Too terse, just like my review ;-P
> # Since: 6.1
> ##
> { 'struct': 'SMPConfiguration', 'data': {
> @@ -1662,7 +1670,11 @@
> '*modules': 'int',
> '*cores': 'int',
> '*threads': 'int',
> - '*maxcpus': 'int' } }
> + '*maxcpus': 'int',
> + '*l1d-cache': 'str',
> + '*l1i-cache': 'str',
> + '*l2-cache': 'str',
> + '*l3-cache': 'str' } }
>
> ##
> # @x-query-irq:
[...]
On Wed, Feb 21, 2024 at 01:46:21PM +0100, Markus Armbruster wrote:
> Date: Wed, 21 Feb 2024 13:46:21 +0100
> From: Markus Armbruster <armbru@redhat.com>
> Subject: Re: [RFC 4/8] hw/core: Add cache topology options in -smp
>
> Zhao Liu <zhao1.liu@linux.intel.com> writes:
>
> > From: Zhao Liu <zhao1.liu@intel.com>
> >
> > 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 d0e7f1f615f3..0a923ac38803 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -1650,6 +1650,14 @@
> > #
> > # @threads: number of threads per core
> > #
> > +# @l1d-cache: topology hierarchy of L1 data cache (since 9.0)
> > +#
> > +# @l1i-cache: topology hierarchy of L1 instruction cache (since 9.0)
> > +#
> > +# @l2-cache: topology hierarchy of L2 unified cache (since 9.0)
> > +#
> > +# @l3-cache: topology hierarchy of L3 unified cache (since 9.0)
> > +#
>
> Too terse, just like my review ;-P
;-) Yes, I'll add more information to improve the readability of the
code and comments.
Thanks,
Zhao
>
> > # Since: 6.1
> > ##
> > { 'struct': 'SMPConfiguration', 'data': {
> > @@ -1662,7 +1670,11 @@
> > '*modules': 'int',
> > '*cores': 'int',
> > '*threads': 'int',
> > - '*maxcpus': 'int' } }
> > + '*maxcpus': 'int',
> > + '*l1d-cache': 'str',
> > + '*l1i-cache': 'str',
> > + '*l2-cache': 'str',
> > + '*l3-cache': 'str' } }
> >
> > ##
> > # @x-query-irq:
>
> [...]
>
© 2016 - 2024 Red Hat, Inc.