On Thu, 5 Jun 2025 17:44:52 +0800
Zhao Liu <zhao1....@intel.com> wrote:
> 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
>
>
>
Hi Zhao,
Thanks for the feedback, I was actually unsure about the structure, and
was looking to get some feedback here, to see where to put what! I
will go over the suggestions, and take note. In the meantime,
let's see if we can get some more review here.
Alireza
