On Wed, Sep 11, 2024 at 11:35 AM Jonathan Cameron <
jonathan.came...@huawei.com> wrote:
> On Tue, 10 Sep 2024 12:01:05 +0100
> Salil Mehta <salil.me...@huawei.com> wrote:
>
> > HI Zhao,
>
> A few trivial comments inline.
>
> >
> > > From: Zhao Liu <zhao1....@intel.com>
> > > Sent: Monday, September 9, 2024 4:28 PM
> > > To: Salil Mehta <salil.me...@huawei.com>
> > >
> > > On Wed, Sep 04, 2024 at 05:37:21PM +0000, Salil Mehta wrote:
> > > > Date: Wed, 4 Sep 2024 17:37:21 +0000
> > > > From: Salil Mehta <salil.me...@huawei.com>
> > > > Subject: RE: [PATCH RFC V3 01/29] arm/virt,target/arm: Add new
> ARMCPU
> > > > {socket,cluster,core,thread}-id property
> > > >
> > > > Hi Zhao,
> > > >
> > > > > From: zhao1....@intel.com <zhao1....@intel.com>
> > > > > Sent: Wednesday, September 4, 2024 3:43 PM
> > > > > To: Salil Mehta <salil.me...@huawei.com>
> > > > >
> > > > > Hi Salil,
> > > > >
> > > > > On Mon, Aug 19, 2024 at 11:53:52AM +0000, Salil Mehta wrote:
> > > > > > Date: Mon, 19 Aug 2024 11:53:52 +0000 > From: Salil Mehta
> > > > > <salil.me...@huawei.com> > Subject: RE: [PATCH RFC V3 01/29]
> > > > > arm/virt,target/arm: Add new ARMCPU >
> > > > > {socket,cluster,core,thread}-id property
> > > > >
> > > > > [snip]
> > > > >
> > > > > > > > NULL); @@ -2708,6 +2716,7 @@ static const CPUArchIdList
> > >
> > > > > *virt_possible_cpu_arch_ids(MachineState *ms)
> > > > > > > > {
> > > > > > > > int n;
> > > > > > > > unsigned int max_cpus = ms->smp.max_cpus;
> > > > > > > > + unsigned int smp_threads = ms->smp.threads;
> > > > > > > > VirtMachineState *vms = VIRT_MACHINE(ms);
> > > > > > > > MachineClass *mc = MACHINE_GET_CLASS(vms);
> > > > > > > >
> > > > > > > > @@ -2721,6 +2730,7 @@ static const CPUArchIdList > >
> > > > > *virt_possible_cpu_arch_ids(MachineState *ms)
> > > > > > > > ms->possible_cpus->len = max_cpus;
> > > > > > > > for (n = 0; n < ms->possible_cpus->len; n++) {
> > > > > > > > ms->possible_cpus->cpus[n].type = ms->cpu_type;
> > > > > > > > + ms->possible_cpus->cpus[n].vcpus_count =
> smp_threads;
> > > > > > > > ms->possible_cpus->cpus[n].arch_id =
> > > > > > > > virt_cpu_mp_affinity(vms, n);
> > > > > > > >
> > > > > > >
> > > > > > > Why @vcpus_count is initialized to @smp_threads? it needs
> to
> > > > > be > > documented in the commit log.
> > > > > >
> > > > > >
> > > > > > Because every thread internally amounts to a vCPU in QOM and
> > > > > which is > in 1:1 relationship with KVM vCPU. AFAIK, QOM does not
> > > > > strictly > follows any architecture. Once you start to get into
> > > > > details of > threads there are many aspects of shared resources
> one
> > > > > will have to > consider and these can vary across different
> > > > > implementations of architecture.
> > > > >
> > > > > For SPAPR CPU, the granularity of >possible_cpus->cpus[] is
> "core",
> > > > > and for x86, it's "thread" granularity.
> > > >
> > > >
> > > > We have threads per-core at microarchitecture level in ARM as well.
> > > > But each thread appears like a vCPU to OS and AFAICS there are no
> > > > special attributes attached to it. SMT can be enabled/disabled at
> > > > firmware and should get reflected in the configuration accordingly
> > > > i.e. value of *threads-per-core* changes between 1 and 'N'. This
> > > > means 'vcpus_count' has to reflect the correct configuration. But I
> > > > think threads lack proper representation in Qemu QOM.
> > >
> > > In topology related part, SMT (of x86) usually represents the logical
> > > processor level. And thread has the same meaning.
> >
> >
> > Agreed. It is same in ARM as well. The difference could be in how
> hardware
> > threads are implemented at microarchitecture level. Nevertheless, we do
> > have such virtual configurations, and the meaning of *threads* as-in QOM
> > topology (socket,cluster,core,thread) is virtualized similar to the
> hardware
> > threads in host. And One should be able to configure threads support in
> the
> > virtual environment, regardless whether or not underlying hardware
> > supports threads. That's my take.
> >
> > Other aspect is how we then expose these threads to the guest. The guest
> > kernel (just like host kernel) should gather topology information using
> > ACPI PPTT Table (This is ARM specific?).
>
> Not ARM specific, but not used by x86 in practice (I believe some risc-v
> boards
> use it).
>
> https://lore.kernel.org/linux-riscv/20240617131425.7526-3-cuiyun...@bytedance.com/
>
> > Later is populated by the Qemu
> > (just like by firmware for the host kernel) by making use of the virtual
> > topology. ARM guest kernel, in absence of PPTT support can detect
> > presence of hardware threads by reading MT Bit within the MPIDR_EL1
> > register.
>
> Sadly no it can't. Lots of CPUs cores that are single thread set that
> bit anyway (so it's garbage and PPTT / DT is the only source of truth)
>
> https://lore.kernel.org/all/caffo_h7vuehqv15epf+_zvrbdhc3jrejkkovshzhgcxnk+n...@mail.gmail.com/T/
Yes, agreed, this last explanation was not completely correct.
IICRC, Marc did point out in RFC V1 of June 2020 that value MT=0 is set by
KVM to tell the guest kernel that vCPUs at the same affinity-1 fields are
independent.
Problem was with the interpretation of non-zero MT Bit and it was not
consistent.
The key thing is we should not rely on the value of the MT Bit in MPIDR to
know
if multithreading exists. So yes, to know the exact status of the
multithreading
on ARM systems parsing PPTT Table is the only way.
I mentioned that because handling still exists in the kernel code but I
think it exists
for handling those other cases. Maybe a comment is required here (?):
https://elixir.bootlin.com/linux/v6.11-rc7/source/arch/arm64/kernel/topology.c#L34
Thanks for pointing this out.
Thanks
Salil.
>
>
> Jonathan
>
>
