Re: [PATCH v2 1/4] hw/loongarch/virt: Add CPU topology support

[Igor Mammedov]

On Tue, 29 Oct 2024 21:19:15 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> Hi Bibo,
> 
> [snip]
> 
> > +In the CPU topology relationship, When we know the ``socket_id`` 
> > ``core_id``
> > +and ``thread_id`` of the CPU, we can calculate its ``arch_id``:
> > +
> > +``arch_id = (socket_id * S) + (core_id * C) + (thread_id * T)``  
> 
> What's the difference between arch_id and CPU index (CPUState.cpu_index)?

They might be the same but not necessarily.
arch_id is unique cpu identifier from architecture point of view
(which easily could be sparse and without specific order).
(ex: for x86 it's apic_id, for spapr it's core_id)

while cpu_index is internal QEMU, that existed before possible_cpus[]
and non-sparse range and depends on order of cpus are created.
For machines that support possible_cpus[], we override default
cpu_index assignment to fit possible_cpus[].

It might be nice to get rid of cpu_index in favor of possible_cpus[],
but that would be a lot work probably with no huge benefit
when it comes majority of machines that don't need variable
cpu count. 

>   
> >  static void virt_init(MachineState *machine)
> >  {
> > -    LoongArchCPU *lacpu;
> >      const char *cpu_model = machine->cpu_type;
> >      MemoryRegion *address_space_mem = get_system_memory();
> >      LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(machine);
> > @@ -1145,7 +1144,7 @@ static void virt_init(MachineState *machine)
> >      hwaddr base, size, ram_size = machine->ram_size;
> >      const CPUArchIdList *possible_cpus;
> >      MachineClass *mc = MACHINE_GET_CLASS(machine);
> > -    CPUState *cpu;
> > +    Object *cpuobj;
> >  
> >      if (!cpu_model) {
> >          cpu_model = LOONGARCH_CPU_TYPE_NAME("la464");
> > @@ -1164,12 +1163,25 @@ static void virt_init(MachineState *machine)
> >  
> >      /* Init CPUs */
> >      possible_cpus = mc->possible_cpu_arch_ids(machine);
> > -    for (i = 0; i < possible_cpus->len; i++) {
> > -        cpu = cpu_create(machine->cpu_type);
> > -        cpu->cpu_index = i;
> > -        machine->possible_cpus->cpus[i].cpu = cpu;
> > -        lacpu = LOONGARCH_CPU(cpu);
> > -        lacpu->phy_id = machine->possible_cpus->cpus[i].arch_id;
> > +    for (i = 0; i < machine->smp.cpus; i++) {
> > +        cpuobj = object_new(machine->cpu_type);
> > +        object_property_set_uint(cpuobj, "phy-id",
> > +                                possible_cpus->cpus[i].arch_id, NULL);
> > +        /*
> > +         * The CPU in place at the time of machine startup will also enter
> > +         * the CPU hot-plug process when it is created, but at this time,
> > +         * the GED device has not been created, resulting in exit in the 
> > CPU
> > +         * hot-plug process, which can avoid the incumbent CPU repeatedly
> > +         * applying for resources.
> > +         *
> > +         * The interrupt resource of the in-place CPU will be requested at
> > +         * the current function call virt_irq_init().
> > +         *
> > +         * The interrupt resource of the subsequently inserted CPU will be
> > +         * requested in the CPU hot-plug process.
> > +         */
> > +        qdev_realize(DEVICE(cpuobj), NULL, &error_fatal);
> > +        object_unref(cpuobj);  
> 
> You can use qdev_realize_and_unref().
> 
> >      }
> >      fdt_add_cpu_nodes(lvms);
> >      fdt_add_memory_nodes(machine);
> > @@ -1286,6 +1298,35 @@ static void virt_initfn(Object *obj)
> >      virt_flash_create(lvms);
> >  }
> >  
> > +static int virt_get_arch_id_from_topo(MachineState *ms, LoongArchCPUTopo 
> > *topo)
> > +{
> > +    int arch_id, sock_vcpu_num, core_vcpu_num;
> > +
> > +    /*
> > +     * calculate total logical cpus across socket/core/thread.
> > +     * For more information on how to calculate the arch_id,
> > +     * you can refer to the CPU Topology chapter of the
> > +     * docs/system/loongarch/virt.rst document.
> > +     */
> > +    sock_vcpu_num = topo->socket_id * (ms->smp.threads * ms->smp.cores);
> > +    core_vcpu_num = topo->core_id * ms->smp.threads;
> > +
> > +    /* get vcpu-id(logical cpu index) for this vcpu from this topology */
> > +    arch_id = (sock_vcpu_num + core_vcpu_num) + topo->thread_id;  
> 
> Looking at the calculations, arch_id looks the same as cpu_index, right?
> 
> > +    assert(arch_id >= 0 && arch_id < ms->possible_cpus->len);
> > +
> > +    return arch_id;
> > +}
> > +
> > +static void virt_get_topo_from_index(MachineState *ms,
> > +                                     LoongArchCPUTopo *topo, int index)
> > +{
> > +    topo->socket_id = index / (ms->smp.cores * ms->smp.threads);
> > +    topo->core_id = index / ms->smp.threads % ms->smp.cores;
> > +    topo->thread_id = index % ms->smp.threads;
> > +}
> > +
> >  static bool memhp_type_supported(DeviceState *dev)
> >  {
> >      /* we only support pc dimm now */
> > @@ -1385,8 +1426,9 @@ static HotplugHandler 
> > *virt_get_hotplug_handler(MachineState *machine,
> >  
> >  static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> >  {
> > -    int n;
> > +    int n, arch_id;
> >      unsigned int max_cpus = ms->smp.max_cpus;
> > +    LoongArchCPUTopo topo;
> >  
> >      if (ms->possible_cpus) {
> >          assert(ms->possible_cpus->len == max_cpus);
> > @@ -1397,17 +1439,17 @@ static const CPUArchIdList 
> > *virt_possible_cpu_arch_ids(MachineState *ms)
> >                                    sizeof(CPUArchId) * max_cpus);
> >      ms->possible_cpus->len = max_cpus;
> >      for (n = 0; n < ms->possible_cpus->len; n++) {
> > +        virt_get_topo_from_index(ms, &topo, n);
> > +        arch_id = virt_get_arch_id_from_topo(ms, &topo);
> > +        ms->possible_cpus->cpus[n].vcpus_count = ms->smp.threads;  
> 
> In include/hw/boards.h, the doc of CPUArchId said:
> 
> vcpus_count - number of threads provided by @cpu object
> 
> And I undersatnd each element in possible_cpus->cpus[] is mapped to a
> CPU object, so that here vcpus_count should be 1.

it's arch specific, CPU object in possible_cpus was meant to point to
thread/core/..higher layers in future../

For example spapr put's there CPUCore, where vcpus_count can be > 1

That is a bit broken though, since CPUCore is not CPUState by any means,
spapr_core_plug() gets away with it only because
  core_slot->cpu = CPU(dev)
CPU() is dumb pointer cast.

Ideally CPUArchId::cpu should be (Object*) to accommodate various
levels of granularity correctly (with dumb cast above it's just
cosmetics though as long as we treat it as Object in non arch
specific code).

> >          ms->possible_cpus->cpus[n].type = ms->cpu_type;
> > -        ms->possible_cpus->cpus[n].arch_id = n;
> > -
> > +        ms->possible_cpus->cpus[n].arch_id = arch_id;
> >          ms->possible_cpus->cpus[n].props.has_socket_id = true;
> > -        ms->possible_cpus->cpus[n].props.socket_id  =
> > -                                   n / (ms->smp.cores * ms->smp.threads);
> > +        ms->possible_cpus->cpus[n].props.socket_id = topo.socket_id;
> >          ms->possible_cpus->cpus[n].props.has_core_id = true;
> > -        ms->possible_cpus->cpus[n].props.core_id =
> > -                                   n / ms->smp.threads % ms->smp.cores;
> > +        ms->possible_cpus->cpus[n].props.core_id = topo.core_id;
> >          ms->possible_cpus->cpus[n].props.has_thread_id = true;
> > -        ms->possible_cpus->cpus[n].props.thread_id = n % ms->smp.threads;
> > +        ms->possible_cpus->cpus[n].props.thread_id = topo.thread_id;
> >      }
> >      return ms->possible_cpus;
> >  }
> > diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c
> > index 7212fb5f8f..5dfc0d5c43 100644
> > --- a/target/loongarch/cpu.c
> > +++ b/target/loongarch/cpu.c
> > @@ -16,6 +16,7 @@
> >  #include "kvm/kvm_loongarch.h"
> >  #include "exec/exec-all.h"
> >  #include "cpu.h"
> > +#include "hw/qdev-properties.h"
> >  #include "internals.h"
> >  #include "fpu/softfloat-helpers.h"
> >  #include "cpu-csr.h"
> > @@ -780,6 +781,15 @@ static int64_t loongarch_cpu_get_arch_id(CPUState *cs)
> >  }
> >  #endif
> >  
> > +static Property loongarch_cpu_properties[] = {
> > +    DEFINE_PROP_UINT32("phy-id", LoongArchCPU, phy_id, UNSET_PHY_ID),  
> 
> IIUC, the phy_id is kind of like the x86 apic_id, but the field is completely
> derived from topology, so why do you need to define it as a property and then
> expose it to the user?

for x86 we do expose apic_id as a property as well, partly from historical pov
but also it's better to access cpu fields via properties from outside of
CPU object than directly poke inside of CPU structure from outside of CPU
(especially if it comes to generic code)

> 
> Thanks,
> Zhao
> 
>