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