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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 30 Oct 2024 09:42:10 +0800
maobibo <maobibo@loongson.cn> wrote:

> Hi Zhao,
> 
> Thanks for reviewing the patch.
> 
> On 2024/10/29 下午9:19, Zhao Liu 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)?
> >      
> >>   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().  
> sure, will do.
> 
> >   
> >>       }
> >>       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?  
> The value of arch_id and cpu_index is the same now, however meaning is 
> different. cpu_index is cpuslot index of possible_cpus array, arch_id is 
> physical id.
> 
> Now there is no special HW calculation for physical id, value of them is 
> the same. In future if physical id width exceeds 8-bit because extioi 
> only support max 256 cpu routing, its calculation method will change.
> >   
> >> +    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.  
> yes, it is should be 1, thank for pointing it out
> > 
> >   
> >>           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?  

> The property is wrongly used here. we want to differentiate cold-added 
> cpus and hot-added cpus.  phy_id of cold-added cpus can be set during 
> power on, however that of hot-added cpus is default -1.
why would you need to differentiate them?

> 
> Internal variable phy_id can be set with default value -1 in function 
> loongarch_cpu_init(), property can be removed.
> 
> Regards
> Bibo Mao
> > 
> > Thanks,
> > Zhao
> >   
> 
>