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

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

Hi Igor,

> > 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)

Yes, I was previously puzzled as to why the core_id of spapr is global,
which is completely different from the meaning of core_id in x86. Now,
your analogy has made it very clear to me. Thanks!

> 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. 

Thank you! Now I see.

> > 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.

Is it also because of architectural reasons that the smallest granularity
supported by spapr can only be the core?

> 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).

Thank you. So, I would like to ask, should the elements in possible_cpus
be understood as the smallest granularity supported by hotplug?

I want to understand that this reason is unrelated to the loongarch patch,
instead I mainly want to continue thinking about my previous qom-topo[*]
proposal.

I remember your hotplug slides also mentioned larger granularity hotplug,
which I understand, for example, allows x86 to support core/socket, etc.
(this of course requires core/socket object abstraction).

If possible_cpus[] only needs to correspond to the smallest granularity
topo object, then it matches my qom-topo proposal quite well, essentially
mapping one layer of a complete topology tree (which is built from socket
to thread, layer by layer) to possible_cpus[] (actually, this is my design:
mapping the thread layer of the x86 topology tree to possible_cpus[]. :) )

Although many years have passed, I still believe that larger granularity
hotplug is valuable, especially as hardware includes more and more CPUs.

[*]: https://lore.kernel.org/qemu-devel/20240919015533.766754-1-zhao1.liu@intel.com/

[snip]

> > 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)

Thank you for your guidance. Similar to Bibo’s question, I also wonder
if there is the need for a property that won't be exposed to users.

Regards,
Zhao