Re: [PATCH v2 2/2] KVM: arm64: Skip unreset vCPUs in MPIDR lookup table

[fuqiang wang <fuqiang.wng@gmail.com>]

On 6/15/26 6:08 PM, Marc Zyngier wrote:

Hi Oliver, Marc

Thank you very much for your replies and guidance. I'm sorry for the
late reply.(I took some time to brush up on ARM CPU hotplug.)

> On Mon, 15 Jun 2026 05:20:35 +0100,
> Oliver Upton <oupton@kernel.org> wrote:
>>
>> On Sun, Jun 14, 2026 at 10:26:32AM +0100, Marc Zyngier wrote:
>>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>>> index 3732ee9eb0d4..fccfa97370df 100644
>>>> --- a/arch/arm64/kvm/arm.c
>>>> +++ b/arch/arm64/kvm/arm.c
>>>> @@ -887,8 +887,18 @@ static void kvm_init_mpidr_data(struct kvm *kvm)
>>>>   	data->mpidr_mask = mask;
>>>>   
>>>>   	kvm_for_each_vcpu(c, vcpu, kvm) {
>>>> -		u64 aff = kvm_vcpu_get_mpidr_aff(vcpu);
>>>> -		u16 index = kvm_mpidr_index(data, aff);
>>>> +		u64 aff;
>>>> +		u16 index;
>>>> +
>>>> +		/*
>>>> +		 * Skip vCPUs that haven't been reset yet; their MPIDR_EL1 is
>>>> +		 * zero.
>>>> +		 */
>>>> +		if (!kvm_vcpu_mpidr_is_reset(vcpu))
>>>> +			continue;
>>>
>>> But what about the initial loop that computes the significant bits
>>> amongst the vcpus?

Yes, this is indeed missed here, possibly making the following nr_entries
calculation too large.

>>>
>>>> +
>>>> +		aff = kvm_vcpu_get_mpidr_aff(vcpu);
>>>> +		index = kvm_mpidr_index(data, aff);
>>>
>>> In all honesty, I think this is a userspace bug more than anything
>>> else, and checking for random bits in MPDR_EL1 to verify whether the
>>> value is plausible is gross.
>>
>> +1. Checking the MPIDR value is also broken because userspace can write
>> whatever it wants to the register, which could even clear the RES1 bit
>> that's getting tested here.

Yes, the change here is quite ugly — it looks like temporary debug code.
It would be better to change it to !kvm_vcpu_initialized().

>>
>>> Yhis isn't different from setting MPIDR_EL1 to the same value on all
>>> vcpus, which we don't try to mitigate. Late setting of MPIDR_EL1 also
>>> defeats the whole point of having a cache for the affinity to index
>>> conversion, making SGIs pretty slow for late CPUs.
>>>
>>> I really think that by not finalising your vcpus and start running the
>>> guest, you have cornered yourself pretty badly, and we shouldn't try
>>> to paper over it.
>>
>> I generally agree, although I wouldn't be against a change that nuked
>> any of the cached routings in case of userspace doing stupid things like
>> collisions and whatnot.

I once thought about sending patch [1] again, because I found that Oliver
had done something similar[2]... but then I gave up.

For restless userspace programs, KVM cannot foresee their behavior. For
example, when a VCPU is running, there may be cases like:

+ some vcpus not create, later create
+ some vcpus created but not reset, later reset
+ some vcpu not create, later create BUT never run. This may prevent other
   vCPU threads from using the mpidr_data, because they rely on the vCPU that
   destroys the mpidr_data to trigger a rebuild. (Although we can rebuild
   mpidr_data when resetting the vCPU(just for later-created vcpu), it is
   preferable to defer the rebuild until after all destruction actions have
   been completed).

even, as mentioned above, userspace write MPIDR_EL1 to cause collisions. It
seems somewhat not worth the effort.

[1]: https://github.com/cai-fuqiang/armkvm/commit/eeda9309e0a6c700449e90cf27127a7e4e0238ed
[2]: https://lore.kernel.org/all/20240508071952.2035422-1-oliver.upton@linux.dev/#r

> 
> Detecting collisions is difficult, as we have no idea of the overall
> guest topology. All we can do is work out whether the computed mask
> has enough bits to represent the number of online vcpus, but that's
> not necessarily good enough.
> 
> One possibility would be to invalidate the cache on each update to any
> MPIDR_EL1, including reset. People doing silly things by initialising
> vcpus post first start will still suffer, but should we care?

However, could we detect collisions within the init function and, upon
detection, notify userspace without taking any corrective or fallback
action? This would serve as a stronger reminder to userspace of its
non-compliant behavior.

e.g.

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 3732ee9eb0d4..7563feab1a11 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -893,6 +893,17 @@ static void kvm_init_mpidr_data(struct kvm *kvm)
                 data->cmpidr_to_idx[index] = c;
         }

+       kvm_for_each_vcpu(c, vcpu, kvm) {
+               u64 aff = kvm_vcpu_get_mpidr_aff(vcpu);
+               u16 index = kvm_mpidr_index(data, aff);
+
+               if (data->cmpidr_to_idx[index] != c) {
+                       pr_warn("Multiple vCPUs share the same MPIDR value, "
+                               "it may cause the guest to hang or run slower\n");
+                       break;
+               }
+       }
+
         rcu_assign_pointer(kvm->arch.mpidr_data, data);
  out:
         mutex_unlock(&kvm->arch.config_lock);


Thanks
fuqiang

> 
> Thanks,
> 
> 	M.
>