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.came...@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....@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....@intel.com>
> > Tested-by: Yongwei Ma <yongwei...@intel.com>
>
> Trivial language suggestions.
> In general looks good to me.
>
> Reviewed-by: Jonathan Cameron <jonathan.came...@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
