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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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

> ---
> 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(-)
> 
> 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",

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!

>      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.

> +
> +        ``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 ...

> +        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.
> +        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?

> +
> +        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,