Re: [PATCH 2/2] hw/acpi: add cache hierarchy node to pptt table

[Zhao Liu]

Hi Alireza,

On Fri, Aug 23, 2024 at 01:54:46PM +0100, Alireza Sanaee wrote:

[snip]

> +static int partial_cache_description(MachineState *ms, ACPIPPTTCache* caches,
> +                                 int num_caches)
> +{
> +    int level, c;
> +
> +    for (level = 1; level < num_caches; level++) {
> +        for (c = 0; c < num_caches; c++) {
> +            if (caches[c].level != level) {
> +                continue;
> +            }
> +
> +            switch (level) {
> +            case 1:
> +                /*
> +                 * L1 cache is assumed to have both L1I and L1D available.
> +                 * Technically both need to be checked.
> +                 */
> +                if (machine_get_cache_topo_level(ms, SMP_CACHE_L1I) ==
> +                        CPU_TOPO_LEVEL_DEFAULT) {

This check just concerns L1i, but it looks not covering L1d, is L1d being
missed?  

> +                    assert(machine_get_cache_topo_level(ms, SMP_CACHE_L1D) !=
> +                           CPU_TOPO_LEVEL_DEFAULT);

I understand you don't want user to configure other different levels for
L1d in this case...If so, it's better to return error (error_steg or
error_report or some other error print ways) to tell user his cache
configuration is invalid.

> +                    return level;
> +                }
> +                break;
> +            case 2:
> +                if (machine_get_cache_topo_level(ms, SMP_CACHE_L2) ==
> +                        CPU_TOPO_LEVEL_DEFAULT) {
> +                    return level;
> +                }
> +                break;
> +            case 3:
> +                if (machine_get_cache_topo_level(ms, SMP_CACHE_L3) ==
> +                        CPU_TOPO_LEVEL_DEFAULT) {
> +                    return level;
> +                }
> +                break;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +

[snip]

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index b0c68d66a3..b723248ecf 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -3093,6 +3093,11 @@ static void virt_machine_class_init(ObjectClass *oc, 
> void *data)
>      hc->unplug = virt_machine_device_unplug_cb;
>      mc->nvdimm_supported = true;
>      mc->smp_props.clusters_supported = true;
> +    /* Supported cached */
> +    mc->smp_props.cache_supported[SMP_CACHE_L1D] = true;
> +    mc->smp_props.cache_supported[SMP_CACHE_L1I] = true;
> +    mc->smp_props.cache_supported[SMP_CACHE_L2] = true;
> +    mc->smp_props.cache_supported[SMP_CACHE_L3] = true;
>      mc->auto_enable_numa_with_memhp = true;
>      mc->auto_enable_numa_with_memdev = true;
>      /* platform instead of architectural choice */
> diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
> index bf6f2f9107..de95ec9c0f 100644
> --- a/hw/core/machine-smp.c
> +++ b/hw/core/machine-smp.c
> @@ -274,7 +274,11 @@ unsigned int machine_topo_get_threads_per_socket(const 
> MachineState *ms)
>  CpuTopologyLevel machine_get_cache_topo_level(const MachineState *ms,
>                                                SMPCacheName cache)
>  {
> -    return ms->smp_cache->props[cache].topo;
> +    if (ms->smp_cache) {
> +        return ms->smp_cache->props[cache].topo;
> +    }
> +
> +    return CPU_TOPO_LEVEL_DEFAULT;
>  }
>  
>  static bool machine_check_topo_support(MachineState *ms,

Maybe it's better to split smp-cache support/check on Arm in a seperate
patch.

Regards,
Zhao