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
