diff mbox series

[v2,7/7] i386/pc: Support cache topology in -machine for PC machine

Message ID 20240908125920.1160236-8-zhao1.liu@intel.com (mailing list archive)
State New, archived
Headers show
Series Introduce SMP Cache Topology | expand

Commit Message

Zhao Liu Sept. 8, 2024, 12:59 p.m. UTC 
Allow user to configure l1d, l1i, l2 and l3 cache topologies for PC
machine.

Additionally, add the document of "-machine smp-cache" in
qemu-options.hx.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Tested-by: Yongwei Ma <yongwei.ma@intel.com>
---
Changes since Patch v1:
 * Merged document into this patch. (Markus)

Changes since RFC v2:
 * Used cache_supported array.
---
 hw/i386/pc.c    |  4 ++++
 qemu-options.hx | 28 +++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

Comments

Zhao Liu Oct. 7, 2024, 11:53 a.m. UTC | #1 
On Tue, Sep 17, 2024 at 10:16:31AM +0100, Jonathan Cameron wrote:
> Date: Tue, 17 Sep 2024 10:16:31 +0100
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Subject: Re: [PATCH v2 7/7] i386/pc: Support cache topology in -machine for
>  PC machine
> X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32)
> 
> On Sun,  8 Sep 2024 20:59:20 +0800
> Zhao Liu <zhao1.liu@intel.com> wrote:
> 
> > Allow user to configure l1d, l1i, l2 and l3 cache topologies for PC
> > machine.
> > 
> > Additionally, add the document of "-machine smp-cache" in
> > qemu-options.hx.
> > 
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
>
> Trivial language suggestions.
> In general looks good to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Hopefully QOM maintainers and others will get to this soon. 
> I'd like Ali's ARM series to land this cycle as well
> as the lack of this support has been a pain point for us
> for a while.
>
> Jonathan

Thanks! I'll refresh a new version.

[snip]

> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index d94e2cbbaeb1..3936ff3e77f9 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -39,7 +39,8 @@ DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> >      "                memory-encryption=@var{} memory encryption object to use (default=none)\n"
> >      "                hmat=on|off controls ACPI HMAT support (default=off)\n"
> >      "                memory-backend='backend-id' specifies explicitly provided backend for main RAM (default=none)\n"
> > -    "                cxl-fmw.0.targets.0=firsttarget,cxl-fmw.0.targets.1=secondtarget,cxl-fmw.0.size=size[,cxl-fmw.0.interleave-granularity=granularity]\n",
> > +    "                cxl-fmw.0.targets.0=firsttarget,cxl-fmw.0.targets.1=secondtarget,cxl-fmw.0.size=size[,cxl-fmw.0.interleave-granularity=granularity]\n"
> > +    "                smp-cache.0.cache=cachename,smp-cache.0.topology=topologylevel\n",
> 
> Now my cxl-fmw stuff has competition for most hideous element :)
> When we add a few more properties maybe we'll get an even longer line!

May JSON support can save us :). When I have time I will consider this.
Command line's keyval format is more convenient for configuring a single
element in an array.
 
> >      QEMU_ARCH_ALL)
> >  SRST
> >  ``-machine [type=]name[,prop=value[,...]]``
> > @@ -159,6 +160,31 @@ SRST
> >          ::
> >  
> >              -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
> > +
> > +    ``smp-cache.0.cache=cachename,smp-cache.0.topology=topologylevel``
> > +        Define cache properties (now only the cache topology level) for SMP
> > +        system.
> 
> I'd drop the 'now only' bit.  Just means we have add noise updating that
> later.   It's easy enough to look down and see what is available anyway give
> the parameter docs follow immediately after this.

Agree.

> > +
> > +        ``cache=cachename`` specifies the cache that the properties will be
> > +        applied on. This field is the combination of cache level and cache
> > +        type. Currently it supports ``l1d`` (L1 data cache), ``l1i`` (L1
> 
> Drop the word Currently as I don't think it adds anything to he meaning.
> We are never going to add docs that say 'previously it supported' or 'in the
> future it will support'.
> 
> 	   "Supports ...
> 

Thanks! I will change to "It supports ..."

> > +        instruction cache), ``l2`` (L2 unified cache) and ``l3`` (L3 unified
> > +        cache).
> > +
> > +        ``topology=topologylevel`` sets the cache topology level. It accepts
> > +        CPU topology levels including ``thread``, ``core``, ``module``,
> > +        ``cluster``, ``die``, ``socket``, ``book``, ``drawer`` and a special
> > +        value ``default``. If ``default`` is set, then the cache topology will
> > +        follow the architecture's default cache topology model. If other CPU
> If another topology level is set
> 
> would be clearer.   I briefly read this as saying the topology for another CPU
> rather than a different value here.

Ah, yes, I agree.

> > +        topology level is set, the cache will be shared at corresponding CPU
> > +        topology level. For example, ``topology=core`` makes the cache shared
> > +        in a core.
> "by all threads within a core." perhaps?

Nice, it's more accurate.

Thanks,
Zhao
diff mbox series

Patch

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ba0ff511836c..d562fd25aad2 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1788,6 +1788,10 @@  static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->nvdimm_supported = true;
     mc->smp_props.dies_supported = true;
     mc->smp_props.modules_supported = true;
+    mc->smp_props.cache_supported[CACHE_LEVEL_AND_TYPE_L1D] = true;
+    mc->smp_props.cache_supported[CACHE_LEVEL_AND_TYPE_L1I] = true;
+    mc->smp_props.cache_supported[CACHE_LEVEL_AND_TYPE_L2] = true;
+    mc->smp_props.cache_supported[CACHE_LEVEL_AND_TYPE_L3] = true;
     mc->default_ram_id = "pc.ram";
     pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_AUTO;
 
diff --git a/qemu-options.hx b/qemu-options.hx
index d94e2cbbaeb1..3936ff3e77f9 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -39,7 +39,8 @@  DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
     "                memory-encryption=@var{} memory encryption object to use (default=none)\n"
     "                hmat=on|off controls ACPI HMAT support (default=off)\n"
     "                memory-backend='backend-id' specifies explicitly provided backend for main RAM (default=none)\n"
-    "                cxl-fmw.0.targets.0=firsttarget,cxl-fmw.0.targets.1=secondtarget,cxl-fmw.0.size=size[,cxl-fmw.0.interleave-granularity=granularity]\n",
+    "                cxl-fmw.0.targets.0=firsttarget,cxl-fmw.0.targets.1=secondtarget,cxl-fmw.0.size=size[,cxl-fmw.0.interleave-granularity=granularity]\n"
+    "                smp-cache.0.cache=cachename,smp-cache.0.topology=topologylevel\n",
     QEMU_ARCH_ALL)
 SRST
 ``-machine [type=]name[,prop=value[,...]]``
@@ -159,6 +160,31 @@  SRST
         ::
 
             -machine cxl-fmw.0.targets.0=cxl.0,cxl-fmw.0.targets.1=cxl.1,cxl-fmw.0.size=128G,cxl-fmw.0.interleave-granularity=512
+
+    ``smp-cache.0.cache=cachename,smp-cache.0.topology=topologylevel``
+        Define cache properties (now only the cache topology level) for SMP
+        system.
+
+        ``cache=cachename`` specifies the cache that the properties will be
+        applied on. This field is the combination of cache level and cache
+        type. Currently it supports ``l1d`` (L1 data cache), ``l1i`` (L1
+        instruction cache), ``l2`` (L2 unified cache) and ``l3`` (L3 unified
+        cache).
+
+        ``topology=topologylevel`` sets the cache topology level. It accepts
+        CPU topology levels including ``thread``, ``core``, ``module``,
+        ``cluster``, ``die``, ``socket``, ``book``, ``drawer`` and a special
+        value ``default``. If ``default`` is set, then the cache topology will
+        follow the architecture's default cache topology model. If other CPU
+        topology level is set, the cache will be shared at corresponding CPU
+        topology level. For example, ``topology=core`` makes the cache shared
+        in a core.
+
+        Example:
+
+        ::
+
+            -machine smp-cache.0.cache=l1d,smp-cache.0.topology=core,smp-cache.1.cache=l1i,smp-cache.1.topology=core
 ERST
 
 DEF("M", HAS_ARG, QEMU_OPTION_M,