Re: [PATCH v12 2/6] arm/virt.c: add cache hierarchy to device tree

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

Hi Ali,

I'm very sorry to bother you with some comments after so many versions.

> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> index 5cb2e9a7f5..7339782663 100644
> --- a/hw/cpu/core.c
> +++ b/hw/cpu/core.c

core.c is not the right place. It just contains the "cpu-core"
abstraction. So we need to move the following functions to other files.

> @@ -102,4 +102,96 @@ static void cpu_core_register_types(void)
>      type_register_static(&cpu_core_type_info);
>  }
>  
> +bool cache_described_at(const MachineState *ms, CpuTopologyLevel level)

It's better to add the comment about what this function did. (its name
doesn't reflect it wants to check the coresponding CPU topology level.)

I also feel it's not a good name, what about "machine_check_cache_at_topo_level"?

Furthermore, we can move this one to hw/core/machine-smp.c, as it is
about with machine's smp_cache.

> +{
> +    if (machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L3) == level ||
> +        machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L2) == level ||
> +        machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L1I) == level ||
> +        machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L1D) == level) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +int partial_cache_description(const MachineState *ms, CPUCorePPTTCaches *caches,
> +                              int num_caches)

Because I'll suggest to move CPUCorePPTTCaches to include/hw/acpi/cpu.h
later, and this function accepts CPUCorePPTTCaches as the argument, so
I think we could move this function to hw/acpi/cpu.c (if Michael and
Igor don't object).

> +{
> +    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,
> +                                                 CACHE_LEVEL_AND_TYPE_L1I) ==
> +                    CPU_TOPOLOGY_LEVEL_DEFAULT) {
> +                    return level;
> +                }
> +                break;
> +            case 2:
> +                if (machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L2) ==
> +                    CPU_TOPOLOGY_LEVEL_DEFAULT) {
> +                    return level;
> +                }
> +                break;
> +            case 3:
> +                if (machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L3) ==
> +                    CPU_TOPOLOGY_LEVEL_DEFAULT) {
> +                    return level;
> +                }
> +                break;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * This function assumes l3 and l2 have unified cache and l1 is split l1d
> + * and l1i, and further prepares the lowest cache level for a topology
> + * level.  The info will be fed to build_caches to create caches at the
> + * right level.
> + */
> +bool find_the_lowest_level_cache_defined_at_level(const MachineState *ms,
> +                                                  int *level_found,
> +                                                  CpuTopologyLevel topo_level) {

This is a very long name :-).

Maybe we can rename it like:

machine_find_lowest_level_cache_at_topo_level,

(sorry this name doesn't shorten the length but align the naming style
in machine-smp.c)

and explain the arguments in the comment.

Furthermore, we can move this one to hw/core/machine-smp.c, too.

> +    CpuTopologyLevel level;
> +
> +    level = machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L1I);
> +    if (level == topo_level) {
> +        *level_found = 1;
> +        return true;
> +    }
> +
> +    level = machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L1D);
> +    if (level == topo_level) {
> +        *level_found = 1;
> +        return true;
> +    }
> +
> +    level = machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L2);
> +    if (level == topo_level) {
> +        *level_found = 2;
> +        return true;
> +    }
> +
> +    level = machine_get_cache_topo_level(ms, CACHE_LEVEL_AND_TYPE_L3);
> +    if (level == topo_level) {
> +        *level_found = 3;
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  type_init(cpu_core_register_types)

...

> diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> index 98ab91647e..0f7bf8bc28 100644
> --- a/include/hw/cpu/core.h
> +++ b/include/hw/cpu/core.h
> @@ -11,6 +11,7 @@
>  
>  #include "hw/qdev-core.h"
>  #include "qom/object.h"
> +#include "qapi/qapi-types-machine-common.h"
>  
>  #define TYPE_CPU_CORE "cpu-core"
>  
> @@ -25,6 +26,32 @@ struct CPUCore {
>      int nr_threads;
>  };
>  
> +typedef enum CPUCacheType {
> +    CPU_CORE_DATA,
> +    CPU_CORE_INSTRUCTION,
> +    CPU_CORE_UNIFIED,
> +} CPUCoreCacheType;

This is a complete duplicate of the x86's CPUCaches (target/i386/cpu.h).

I think we can move x86's CPUCaches to include/hw/core/cpu.h.

> +typedef struct CPUPPTTCaches {
> +    CPUCoreCacheType type;
> +    uint32_t pptt_id;
> +    uint32_t sets;
> +    uint32_t size;
> +    uint32_t level;
> +    uint16_t linesize;
> +    uint8_t attributes; /* write policy: 0x0 write back, 0x1 write through */
> +    uint8_t associativity;
> +} CPUCorePPTTCaches;

x86 doesn't use PPTT to describe cache so it's not necessary to reuse
CPUCacheInfo (target/i386/cpu.h) for PPTT.

But I understand it's better to place this sturct in include/hw/acpi/cpu.h,
since it is part of the ACPI PPTT table.

> +int partial_cache_description(const MachineState *ms, CPUCorePPTTCaches *caches,
> +                              int num_caches);

Could move to include/hw/acpi/cpu.h, too.

> +bool cache_described_at(const MachineState *ms, CpuTopologyLevel level);
> +
> +bool find_the_lowest_level_cache_defined_at_level(const MachineState *ms,
> +                                                  int *level_found,
> +                                                  CpuTopologyLevel topo_level);
> +

Because these 2 functions' definitions would be moved to
hw/core/machine-smp.c, then we need to move their declarations to
include/hw/boards.h.


Except the above nits, the general part is fine for me.

Thanks,
Zhao