Message ID | 20240908125920.1160236-8-zhao1.liu@intel.com |
---|---|
State | New |
Headers | show |
Series | Introduce SMP Cache Topology | expand |
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 --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,