Re: [PATCH v4 21/21] i386: Add new property to control L2 cache topo in CPUID.04H

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Sep 15, 2023 at 03:53:25PM +0800, Zhao Liu wrote:
> Hi Philippe,
> 
> On Thu, Sep 14, 2023 at 09:41:30AM +0200, Philippe Mathieu-Daudé wrote:
> > Date: Thu, 14 Sep 2023 09:41:30 +0200
> > From: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Subject: Re: [PATCH v4 21/21] i386: Add new property to control L2 cache
> >  topo in CPUID.04H
> > 
> > On 14/9/23 09:21, Zhao Liu wrote:
> > > From: Zhao Liu <zhao1.liu@intel.com>
> > > 
> > > The property x-l2-cache-topo will be used to change the L2 cache
> > > topology in CPUID.04H.
> > > 
> > > Now it allows user to set the L2 cache is shared in core level or
> > > cluster level.
> > > 
> > > If user passes "-cpu x-l2-cache-topo=[core|cluster]" then older L2 cache
> > > topology will be overrode by the new topology setting.
> > > 
> > > Here we expose to user "cluster" instead of "module", to be consistent
> > > with "cluster-id" naming.
> > > 
> > > Since CPUID.04H is used by intel CPUs, this property is available on
> > > intel CPUs as for now.
> > > 
> > > When necessary, it can be extended to CPUID.8000001DH for AMD CPUs.
> > > 
> > > (Tested the cache topology in CPUID[0x04] leaf with "x-l2-cache-topo=[
> > > core|cluster]", and tested the live migration between the QEMUs w/ &
> > > w/o this patch series.)
> > > 
> > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > > Tested-by: Yongwei Ma <yongwei.ma@intel.com>
> > > ---
> > > Changes since v3:
> > >   * Add description about test for live migration compatibility. (Babu)
> > > 
> > > Changes since v1:
> > >   * Rename MODULE branch to CPU_TOPO_LEVEL_MODULE to match the previous
> > >     renaming changes.
> > > ---
> > >   target/i386/cpu.c | 34 +++++++++++++++++++++++++++++++++-
> > >   target/i386/cpu.h |  2 ++
> > >   2 files changed, 35 insertions(+), 1 deletion(-)
> > 
> > 
> > > @@ -8079,6 +8110,7 @@ static Property x86_cpu_properties[] = {
> > >                        false),
> > >       DEFINE_PROP_BOOL("x-intel-pt-auto-level", X86CPU, intel_pt_auto_level,
> > >                        true),
> > > +    DEFINE_PROP_STRING("x-l2-cache-topo", X86CPU, l2_cache_topo_level),
> > 
> > We use the 'x-' prefix for unstable features, is it the case here?
> 
> I thought that if we can have a more general CLI way to define cache
> topology in the future, then this option can be removed.
> 
> I'm not sure if this option could be treated as unstable, what do you
> think?
> 
> 
> Thanks,
> Zhao

Then, please work on this new generic thing.
What we don't want is people relying on unstable options.

> > 
> > >       DEFINE_PROP_END_OF_LIST()
> > >   };
> >