[PATCH v4 21/21] i386: Add new property to control L2 cache topo in CPUID.04H

Zhao Liu posted 21 patches 2 years, 4 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Marcelo Tosatti <mtosatti@redhat.com>
There is a newer version of this series
[PATCH v4 21/21] i386: Add new property to control L2 cache topo in CPUID.04H
Posted by Zhao Liu 2 years, 4 months ago
From: Zhao Liu <zhao1.liu@intel.com>

The property x-l2-cache-topo will be used to change the L2 cache
topology in CPUID.04H.

Now it allows user to set the L2 cache is shared in core level or
cluster level.

If user passes "-cpu x-l2-cache-topo=[core|cluster]" then older L2 cache
topology will be overrode by the new topology setting.

Here we expose to user "cluster" instead of "module", to be consistent
with "cluster-id" naming.

Since CPUID.04H is used by intel CPUs, this property is available on
intel CPUs as for now.

When necessary, it can be extended to CPUID.8000001DH for AMD CPUs.

(Tested the cache topology in CPUID[0x04] leaf with "x-l2-cache-topo=[
core|cluster]", and tested the live migration between the QEMUs w/ &
w/o this patch series.)

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Yongwei Ma <yongwei.ma@intel.com>
---
Changes since v3:
 * Add description about test for live migration compatibility. (Babu)

Changes since v1:
 * Rename MODULE branch to CPU_TOPO_LEVEL_MODULE to match the previous
   renaming changes.
---
 target/i386/cpu.c | 34 +++++++++++++++++++++++++++++++++-
 target/i386/cpu.h |  2 ++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3bed823dc3b7..b1282c8bd3b7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -243,6 +243,9 @@ static uint32_t max_processor_ids_for_cache(X86CPUTopoInfo *topo_info,
     case CPU_TOPO_LEVEL_CORE:
         num_ids = 1 << apicid_core_offset(topo_info);
         break;
+    case CPU_TOPO_LEVEL_MODULE:
+        num_ids = 1 << apicid_module_offset(topo_info);
+        break;
     case CPU_TOPO_LEVEL_DIE:
         num_ids = 1 << apicid_die_offset(topo_info);
         break;
@@ -251,7 +254,7 @@ static uint32_t max_processor_ids_for_cache(X86CPUTopoInfo *topo_info,
         break;
     default:
         /*
-         * Currently there is no use case for SMT and MODULE, so use
+         * Currently there is no use case for SMT, so use
          * assert directly to facilitate debugging.
          */
         g_assert_not_reached();
@@ -7576,6 +7579,34 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         env->cache_info_amd.l3_cache = &legacy_l3_cache;
     }
 
+    if (cpu->l2_cache_topo_level) {
+        /*
+         * FIXME: Currently only supports changing CPUID[4] (for intel), and
+         * will support changing CPUID[0x8000001D] when necessary.
+         */
+        if (!IS_INTEL_CPU(env)) {
+            error_setg(errp, "only intel cpus supports x-l2-cache-topo");
+            return;
+        }
+
+        if (!strcmp(cpu->l2_cache_topo_level, "core")) {
+            env->cache_info_cpuid4.l2_cache->share_level = CPU_TOPO_LEVEL_CORE;
+        } else if (!strcmp(cpu->l2_cache_topo_level, "cluster")) {
+            /*
+             * We expose to users "cluster" instead of "module", to be
+             * consistent with "cluster-id" naming.
+             */
+            env->cache_info_cpuid4.l2_cache->share_level =
+                                                        CPU_TOPO_LEVEL_MODULE;
+        } else {
+            error_setg(errp,
+                       "x-l2-cache-topo doesn't support '%s', "
+                       "and it only supports 'core' or 'cluster'",
+                       cpu->l2_cache_topo_level);
+            return;
+        }
+    }
+
 #ifndef CONFIG_USER_ONLY
     MachineState *ms = MACHINE(qdev_get_machine());
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
@@ -8079,6 +8110,7 @@ static Property x86_cpu_properties[] = {
                      false),
     DEFINE_PROP_BOOL("x-intel-pt-auto-level", X86CPU, intel_pt_auto_level,
                      true),
+    DEFINE_PROP_STRING("x-l2-cache-topo", X86CPU, l2_cache_topo_level),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index a13132007415..05ffc4c1cc6e 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2073,6 +2073,8 @@ struct ArchCPU {
     int32_t hv_max_vps;
 
     bool xen_vapic;
+
+    char *l2_cache_topo_level;
 };
 
 
-- 
2.34.1
Re: [PATCH v4 21/21] i386: Add new property to control L2 cache topo in CPUID.04H
Posted by Philippe Mathieu-Daudé 2 years, 4 months ago
On 14/9/23 09:21, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> The property x-l2-cache-topo will be used to change the L2 cache
> topology in CPUID.04H.
> 
> Now it allows user to set the L2 cache is shared in core level or
> cluster level.
> 
> If user passes "-cpu x-l2-cache-topo=[core|cluster]" then older L2 cache
> topology will be overrode by the new topology setting.
> 
> Here we expose to user "cluster" instead of "module", to be consistent
> with "cluster-id" naming.
> 
> Since CPUID.04H is used by intel CPUs, this property is available on
> intel CPUs as for now.
> 
> When necessary, it can be extended to CPUID.8000001DH for AMD CPUs.
> 
> (Tested the cache topology in CPUID[0x04] leaf with "x-l2-cache-topo=[
> core|cluster]", and tested the live migration between the QEMUs w/ &
> w/o this patch series.)
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> ---
> Changes since v3:
>   * Add description about test for live migration compatibility. (Babu)
> 
> Changes since v1:
>   * Rename MODULE branch to CPU_TOPO_LEVEL_MODULE to match the previous
>     renaming changes.
> ---
>   target/i386/cpu.c | 34 +++++++++++++++++++++++++++++++++-
>   target/i386/cpu.h |  2 ++
>   2 files changed, 35 insertions(+), 1 deletion(-)


> @@ -8079,6 +8110,7 @@ static Property x86_cpu_properties[] = {
>                        false),
>       DEFINE_PROP_BOOL("x-intel-pt-auto-level", X86CPU, intel_pt_auto_level,
>                        true),
> +    DEFINE_PROP_STRING("x-l2-cache-topo", X86CPU, l2_cache_topo_level),

We use the 'x-' prefix for unstable features, is it the case here?

>       DEFINE_PROP_END_OF_LIST()
>   };
Re: [PATCH v4 21/21] i386: Add new property to control L2 cache topo in CPUID.04H
Posted by Zhao Liu 2 years, 4 months ago
Hi Philippe,

On Thu, Sep 14, 2023 at 09:41:30AM +0200, Philippe Mathieu-Daudé wrote:
> Date: Thu, 14 Sep 2023 09:41:30 +0200
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Subject: Re: [PATCH v4 21/21] i386: Add new property to control L2 cache
>  topo in CPUID.04H
> 
> On 14/9/23 09:21, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > The property x-l2-cache-topo will be used to change the L2 cache
> > topology in CPUID.04H.
> > 
> > Now it allows user to set the L2 cache is shared in core level or
> > cluster level.
> > 
> > If user passes "-cpu x-l2-cache-topo=[core|cluster]" then older L2 cache
> > topology will be overrode by the new topology setting.
> > 
> > Here we expose to user "cluster" instead of "module", to be consistent
> > with "cluster-id" naming.
> > 
> > Since CPUID.04H is used by intel CPUs, this property is available on
> > intel CPUs as for now.
> > 
> > When necessary, it can be extended to CPUID.8000001DH for AMD CPUs.
> > 
> > (Tested the cache topology in CPUID[0x04] leaf with "x-l2-cache-topo=[
> > core|cluster]", and tested the live migration between the QEMUs w/ &
> > w/o this patch series.)
> > 
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> > ---
> > Changes since v3:
> >   * Add description about test for live migration compatibility. (Babu)
> > 
> > Changes since v1:
> >   * Rename MODULE branch to CPU_TOPO_LEVEL_MODULE to match the previous
> >     renaming changes.
> > ---
> >   target/i386/cpu.c | 34 +++++++++++++++++++++++++++++++++-
> >   target/i386/cpu.h |  2 ++
> >   2 files changed, 35 insertions(+), 1 deletion(-)
> 
> 
> > @@ -8079,6 +8110,7 @@ static Property x86_cpu_properties[] = {
> >                        false),
> >       DEFINE_PROP_BOOL("x-intel-pt-auto-level", X86CPU, intel_pt_auto_level,
> >                        true),
> > +    DEFINE_PROP_STRING("x-l2-cache-topo", X86CPU, l2_cache_topo_level),
> 
> We use the 'x-' prefix for unstable features, is it the case here?

I thought that if we can have a more general CLI way to define cache
topology in the future, then this option can be removed.

I'm not sure if this option could be treated as unstable, what do you
think?


Thanks,
Zhao

> 
> >       DEFINE_PROP_END_OF_LIST()
> >   };
> 
Re: [PATCH v4 21/21] i386: Add new property to control L2 cache topo in CPUID.04H
Posted by Michael S. Tsirkin 2 years, 4 months ago
On Fri, Sep 15, 2023 at 03:53:25PM +0800, Zhao Liu wrote:
> Hi Philippe,
> 
> On Thu, Sep 14, 2023 at 09:41:30AM +0200, Philippe Mathieu-Daudé wrote:
> > Date: Thu, 14 Sep 2023 09:41:30 +0200
> > From: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Subject: Re: [PATCH v4 21/21] i386: Add new property to control L2 cache
> >  topo in CPUID.04H
> > 
> > On 14/9/23 09:21, Zhao Liu wrote:
> > > From: Zhao Liu <zhao1.liu@intel.com>
> > > 
> > > The property x-l2-cache-topo will be used to change the L2 cache
> > > topology in CPUID.04H.
> > > 
> > > Now it allows user to set the L2 cache is shared in core level or
> > > cluster level.
> > > 
> > > If user passes "-cpu x-l2-cache-topo=[core|cluster]" then older L2 cache
> > > topology will be overrode by the new topology setting.
> > > 
> > > Here we expose to user "cluster" instead of "module", to be consistent
> > > with "cluster-id" naming.
> > > 
> > > Since CPUID.04H is used by intel CPUs, this property is available on
> > > intel CPUs as for now.
> > > 
> > > When necessary, it can be extended to CPUID.8000001DH for AMD CPUs.
> > > 
> > > (Tested the cache topology in CPUID[0x04] leaf with "x-l2-cache-topo=[
> > > core|cluster]", and tested the live migration between the QEMUs w/ &
> > > w/o this patch series.)
> > > 
> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> > > ---
> > > Changes since v3:
> > >   * Add description about test for live migration compatibility. (Babu)
> > > 
> > > Changes since v1:
> > >   * Rename MODULE branch to CPU_TOPO_LEVEL_MODULE to match the previous
> > >     renaming changes.
> > > ---
> > >   target/i386/cpu.c | 34 +++++++++++++++++++++++++++++++++-
> > >   target/i386/cpu.h |  2 ++
> > >   2 files changed, 35 insertions(+), 1 deletion(-)
> > 
> > 
> > > @@ -8079,6 +8110,7 @@ static Property x86_cpu_properties[] = {
> > >                        false),
> > >       DEFINE_PROP_BOOL("x-intel-pt-auto-level", X86CPU, intel_pt_auto_level,
> > >                        true),
> > > +    DEFINE_PROP_STRING("x-l2-cache-topo", X86CPU, l2_cache_topo_level),
> > 
> > We use the 'x-' prefix for unstable features, is it the case here?
> 
> I thought that if we can have a more general CLI way to define cache
> topology in the future, then this option can be removed.
> 
> I'm not sure if this option could be treated as unstable, what do you
> think?
> 
> 
> Thanks,
> Zhao

Then, please work on this new generic thing.
What we don't want is people relying on unstable options.

> > 
> > >       DEFINE_PROP_END_OF_LIST()
> > >   };
> > 
Re: [PATCH v4 21/21] i386: Add new property to control L2 cache topo in CPUID.04H
Posted by Zhao Liu 2 years, 4 months ago
Hi Michael,

On Tue, Oct 03, 2023 at 08:57:27AM -0400, Michael S. Tsirkin wrote:
> Date: Tue, 3 Oct 2023 08:57:27 -0400
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Subject: Re: [PATCH v4 21/21] i386: Add new property to control L2 cache
>  topo in CPUID.04H
> 
> On Fri, Sep 15, 2023 at 03:53:25PM +0800, Zhao Liu wrote:
> > Hi Philippe,
> > 
> > On Thu, Sep 14, 2023 at 09:41:30AM +0200, Philippe Mathieu-Daud? wrote:
> > > Date: Thu, 14 Sep 2023 09:41:30 +0200
> > > From: Philippe Mathieu-Daud? <philmd@linaro.org>
> > > Subject: Re: [PATCH v4 21/21] i386: Add new property to control L2 cache
> > >  topo in CPUID.04H
> > > 
> > > On 14/9/23 09:21, Zhao Liu wrote:
> > > > From: Zhao Liu <zhao1.liu@intel.com>
> > > > 
> > > > The property x-l2-cache-topo will be used to change the L2 cache
> > > > topology in CPUID.04H.
> > > > 
> > > > Now it allows user to set the L2 cache is shared in core level or
> > > > cluster level.
> > > > 
> > > > If user passes "-cpu x-l2-cache-topo=[core|cluster]" then older L2 cache
> > > > topology will be overrode by the new topology setting.
> > > > 
> > > > Here we expose to user "cluster" instead of "module", to be consistent
> > > > with "cluster-id" naming.
> > > > 
> > > > Since CPUID.04H is used by intel CPUs, this property is available on
> > > > intel CPUs as for now.
> > > > 
> > > > When necessary, it can be extended to CPUID.8000001DH for AMD CPUs.
> > > > 
> > > > (Tested the cache topology in CPUID[0x04] leaf with "x-l2-cache-topo=[
> > > > core|cluster]", and tested the live migration between the QEMUs w/ &
> > > > w/o this patch series.)
> > > > 
> > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> > > > ---
> > > > Changes since v3:
> > > >   * Add description about test for live migration compatibility. (Babu)
> > > > 
> > > > Changes since v1:
> > > >   * Rename MODULE branch to CPU_TOPO_LEVEL_MODULE to match the previous
> > > >     renaming changes.
> > > > ---
> > > >   target/i386/cpu.c | 34 +++++++++++++++++++++++++++++++++-
> > > >   target/i386/cpu.h |  2 ++
> > > >   2 files changed, 35 insertions(+), 1 deletion(-)
> > > 
> > > 
> > > > @@ -8079,6 +8110,7 @@ static Property x86_cpu_properties[] = {
> > > >                        false),
> > > >       DEFINE_PROP_BOOL("x-intel-pt-auto-level", X86CPU, intel_pt_auto_level,
> > > >                        true),
> > > > +    DEFINE_PROP_STRING("x-l2-cache-topo", X86CPU, l2_cache_topo_level),
> > > 
> > > We use the 'x-' prefix for unstable features, is it the case here?
> > 
> > I thought that if we can have a more general CLI way to define cache
> > topology in the future, then this option can be removed.
> > 
> > I'm not sure if this option could be treated as unstable, what do you
> > think?
> > 
> > 
> > Thanks,
> > Zhao
> 
> Then, please work on this new generic thing.
> What we don't want is people relying on unstable options.
> 

Okay, I'll remove this option in the next refresh.

BTW, about the generic cache topology, what about porting this option to
smp? Just like:

-smp cpus=4,sockets=2,cores=2,threads=1, \
     l3-cache=socket,l2-cache=core,l1-i-cache=core,l1-d-cache=core

From the previous discussion [1] with Jonathan, it seems this format
could also meet the requirement for ARM.

If you like this, I'll move forward in this direction. ;-)

[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg03997.html

Thanks,
Zhao
Re: [PATCH v4 21/21] i386: Add new property to control L2 cache topo in CPUID.04H
Posted by Zhao Liu 2 years, 3 months ago
Hi Michael,

On Sat, Oct 07, 2023 at 12:36:41AM +0800, Zhao Liu wrote:
> Date: Sat, 7 Oct 2023 00:36:41 +0800
> From: Zhao Liu <zhao1.liu@linux.intel.com>
> Subject: Re: [PATCH v4 21/21] i386: Add new property to control L2 cache
>  topo in CPUID.04H
> 
> Hi Michael,
> 
> On Tue, Oct 03, 2023 at 08:57:27AM -0400, Michael S. Tsirkin wrote:
> > Date: Tue, 3 Oct 2023 08:57:27 -0400
> > From: "Michael S. Tsirkin" <mst@redhat.com>
> > Subject: Re: [PATCH v4 21/21] i386: Add new property to control L2 cache
> >  topo in CPUID.04H
> > 
> > On Fri, Sep 15, 2023 at 03:53:25PM +0800, Zhao Liu wrote:
> > > Hi Philippe,
> > > 
> > > On Thu, Sep 14, 2023 at 09:41:30AM +0200, Philippe Mathieu-Daud? wrote:
> > > > Date: Thu, 14 Sep 2023 09:41:30 +0200
> > > > From: Philippe Mathieu-Daud? <philmd@linaro.org>
> > > > Subject: Re: [PATCH v4 21/21] i386: Add new property to control L2 cache
> > > >  topo in CPUID.04H
> > > > 
> > > > On 14/9/23 09:21, Zhao Liu wrote:
> > > > > From: Zhao Liu <zhao1.liu@intel.com>
> > > > > 
> > > > > The property x-l2-cache-topo will be used to change the L2 cache
> > > > > topology in CPUID.04H.
> > > > > 
> > > > > Now it allows user to set the L2 cache is shared in core level or
> > > > > cluster level.
> > > > > 
> > > > > If user passes "-cpu x-l2-cache-topo=[core|cluster]" then older L2 cache
> > > > > topology will be overrode by the new topology setting.
> > > > > 
> > > > > Here we expose to user "cluster" instead of "module", to be consistent
> > > > > with "cluster-id" naming.
> > > > > 
> > > > > Since CPUID.04H is used by intel CPUs, this property is available on
> > > > > intel CPUs as for now.
> > > > > 
> > > > > When necessary, it can be extended to CPUID.8000001DH for AMD CPUs.
> > > > > 
> > > > > (Tested the cache topology in CPUID[0x04] leaf with "x-l2-cache-topo=[
> > > > > core|cluster]", and tested the live migration between the QEMUs w/ &
> > > > > w/o this patch series.)
> > > > > 
> > > > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > > > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> > > > > ---
> > > > > Changes since v3:
> > > > >   * Add description about test for live migration compatibility. (Babu)
> > > > > 
> > > > > Changes since v1:
> > > > >   * Rename MODULE branch to CPU_TOPO_LEVEL_MODULE to match the previous
> > > > >     renaming changes.
> > > > > ---
> > > > >   target/i386/cpu.c | 34 +++++++++++++++++++++++++++++++++-
> > > > >   target/i386/cpu.h |  2 ++
> > > > >   2 files changed, 35 insertions(+), 1 deletion(-)
> > > > 
> > > > 
> > > > > @@ -8079,6 +8110,7 @@ static Property x86_cpu_properties[] = {
> > > > >                        false),
> > > > >       DEFINE_PROP_BOOL("x-intel-pt-auto-level", X86CPU, intel_pt_auto_level,
> > > > >                        true),
> > > > > +    DEFINE_PROP_STRING("x-l2-cache-topo", X86CPU, l2_cache_topo_level),
> > > > 
> > > > We use the 'x-' prefix for unstable features, is it the case here?
> > > 
> > > I thought that if we can have a more general CLI way to define cache
> > > topology in the future, then this option can be removed.
> > > 
> > > I'm not sure if this option could be treated as unstable, what do you
> > > think?
> > > 
> > > 
> > > Thanks,
> > > Zhao
> > 
> > Then, please work on this new generic thing.
> > What we don't want is people relying on unstable options.
> > 
> 
> Okay, I'll remove this option in the next refresh.
> 
> BTW, about the generic cache topology, what about porting this option to
> smp? Just like:
> 
> -smp cpus=4,sockets=2,cores=2,threads=1, \
>      l3-cache=socket,l2-cache=core,l1-i-cache=core,l1-d-cache=core
> 
> From the previous discussion [1] with Jonathan, it seems this format
> could also meet the requirement for ARM.
> 
> If you like this, I'll move forward in this direction. ;-)
> 
> [1]: https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg03997.html

Let me get this thread warmed up again.

Could I get your initial thoughts on the general cache topology idea
above?

Thanks,
Zhao