Re: [PATCH v2 4/7] hw/core: Check smp cache topology support for machine

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

On Sun,  8 Sep 2024 20:59:17 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> Add cache_supported flags in SMPCompatProps to allow machines to
> configure various caches support.
> 
> And check the compatibility of the cache properties with the
> machine support in machine_parse_smp_cache().
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> Tested-by: Yongwei Ma <yongwei.ma@intel.com>

Just a few trivial comments inline.

FWIW with or without those changes.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> Changes since Patch v1:
>  * Dropped machine_check_smp_cache_support() and did the check when
>    -machine parses smp-cache in machine_parse_smp_cache().
> 
> Changes since RFC v2:
>  * Split as a separate commit to just include compatibility checking and
>    topology checking.
>  * Allow setting "default" topology level even though the cache
>    isn't supported by machine. (Daniel)
> ---
>  hw/core/machine-smp.c | 78 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/boards.h   |  3 ++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index b517c3471d1a..9a281946762f 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -261,10 +261,47 @@ void machine_parse_smp_config(MachineState *ms,


>  bool machine_parse_smp_cache(MachineState *ms,
>                               const SmpCachePropertiesList *caches,
>                               Error **errp)
>  {
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
>      const SmpCachePropertiesList *node;
>      DECLARE_BITMAP(caches_bitmap, CACHE_LEVEL_AND_TYPE__MAX);
>  
> @@ -293,6 +330,47 @@ bool machine_parse_smp_cache(MachineState *ms,
>          }
>      }
>  
> +    for (int i = 0; i < CACHE_LEVEL_AND_TYPE__MAX; i++) {
> +        const SmpCacheProperties *props = &ms->smp_cache.props[i];
> +
> +        /*
> +         * Allow setting "default" topology level even though the cache
> +         * isn't supported by machine.
I'd flip the comment as the condition is doing the opposite.

            * Reject non "default" topology level if the cache isn't
            * supported by the machine.
> +         */
> +        if (props->topology != CPU_TOPOLOGY_LEVEL_DEFAULT &&
> +            !mc->smp_props.cache_supported[props->cache]) {
> +            error_setg(errp,
> +                       "%s cache topology not supported by this machine",
> +                       CacheLevelAndType_str(node->value->cache));
> +            return false;
> +        }
> +
> +        if (!machine_check_topo_support(ms, props->topology, errp)) {
> +            return false;
> +        }
> +    }
> +
> +    if (smp_cache_topo_cmp(&ms->smp_cache,
> +                           CACHE_LEVEL_AND_TYPE_L1D,

Short line wrap.  Maybe combine the two lines above and similar
cases.  Up to you though, I don't feel that strongly.

> +                           CACHE_LEVEL_AND_TYPE_L2) ||
> +        smp_cache_topo_cmp(&ms->smp_cache,
> +                           CACHE_LEVEL_AND_TYPE_L1I,
> +                           CACHE_LEVEL_AND_TYPE_L2)) {
> +        error_setg(errp,
> +                   "Invalid smp cache topology. "
> +                   "L2 cache topology level shouldn't be lower than L1 cache");
> +        return false;
> +    }
> +
> +    if (smp_cache_topo_cmp(&ms->smp_cache,
> +                           CACHE_LEVEL_AND_TYPE_L2,
> +                           CACHE_LEVEL_AND_TYPE_L3)) {
> +        error_setg(errp,
> +                   "Invalid smp cache topology. "
> +                   "L3 cache topology level shouldn't be lower than L2 cache");
> +        return false;
> +    }
> +
>      return true;
>  }