Re: [PATCH 2/2] kvm/arm: Add mp_affinity for arm vcpu

Andrew Jones
2020-09-17T10:54:44Z

On Thu, Sep 17, 2020 at 11:01:39AM +0100, Marc Zyngier wrote:
> On 2020-09-17 10:47, Alexandru Elisei wrote:
> > Hi,
> >
> > On 9/17/20 9:42 AM, Marc Zyngier wrote:
> > > On 2020-09-17 09:04, Andrew Jones wrote:
> > > > On Thu, Sep 17, 2020 at 08:47:42AM +0100, Marc Zyngier wrote:
> > > > > On 2020-09-17 03:30, Ying Fang wrote:
> > > > > > Allow userspace to set MPIDR using vcpu ioctl KVM_ARM_SET_MP_AFFINITY,
> > > > > > so that we can support cpu topology for arm.
> > > > >
> > > > > MPIDR has *nothing* to do with CPU topology in the ARM architecture.
> > > > > I encourage you to have a look at the ARM ARM and find out how often
> > > > > the word "topology" is used in conjunction with the
> > > > > MPIDR_EL1 register.
> > > > >
> > > >
> > > > Hi Marc,
> > > >
> > > > I mostly agree. However, the CPU topology descriptions use MPIDR to
> > > > identify PEs. If userspace wants to build topology descriptions then
> > > > it either needs to
> > > >
> > > > 1) build them after instantiating all KVM VCPUs in order to query KVM
> > > >    for each MPIDR, or
> > > > 2) have a way to ask KVM for an MPIDR of given VCPU ID in advance
> > > >    (maybe just a scratch VCPU), or
> > > > 3) have control over the MPIDRs so it can choose them when it likes,
> > > >    use them for topology descriptions, and then instantiate KVM VCPUs
> > > >    with them.
> > > >
> > > > I think (3) is the most robust approach, and it has the least
> > > > overhead.
> > >
> > > I don't disagree with the goal, and not even with the choice of
> > > implementation (though I have huge reservations about its quality).
> > >
> > > But the key word here is *userspace*. Only userspace has a notion of
> > > how MPIDR values map to the assumed topology. That's not something
> > > that KVM does nor should interpret (aside from the GIC-induced Aff0
> > > brain-damage). So talking of "topology" in a KVM kernel patch sends
> > > the wrong message, and that's all this remark was about.
> >
> > There's also a patch queued for next which removes using MPIDR as a
> > source of
> > information about CPU topology [1]: "arm64: topology: Stop using MPIDR
> > for
> > topology information".
> >
> > I'm not really sure how useful KVM fiddling with the guest MPIDR will be
> > going
> > forward, at least for a Linux guest.
>
> I think these are two orthogonal things. There is value in setting MPIDR
> to something different as a way to replicate an existing system, for
> example. But deriving *any* sort of topology information from MPIDR isn't
> reliable at all, and is better expressed by firmware tables (and even
> that isn't great).
>

Yes, this is my opinion as well and I'm glad to see the patch that
Alexandru pointed out, since it should stop the MPIDR abuse. Ying Fang
has also posted a QEMU series that populates DT and ACPI[*] to describe
CPU topology to the guest. The user controlled MPIDR is being proposed
in order to support that series.

[*] https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg06027.html

Thanks,
drew

> As far as I am concerned, this patch fits in the "cosmetic" department.
> It's a "nice to have", but doesn't really solve much. Firmware tables
> and userspace placement of the vcpus are key.
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
>