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

[Zhao Liu <zhao1.liu@intel.com>]

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