On Fri, 27 Aug 2021 16:06:16 +0200 Claudio Imbrenda imbrenda@linux.ibm.com wrote:
On Fri, 27 Aug 2021 14:54:29 +0200 Halil Pasic pasic@linux.ibm.com wrote:
While in practice vcpu->vcpu_idx == vcpu->vcp_id is often true,
s/vcp_id/vcpu_id/
it may not always be, and we must not rely on this.
why?
maybe add a simple explanation of why vcpu_idx and vcpu_id can be different, namely: KVM decides the vcpu_idx, userspace decides the vcpu_id, thus the two might not match
Not sure that is a good explanation. A quote from Documentation/virt/kvm/api.rst:
""" 4.7 KVM_CREATE_VCPU -------------------
:Capability: basic :Architectures: all :Type: vm ioctl :Parameters: vcpu id (apic id on x86) :Returns: vcpu fd on success, -1 on error
This API adds a vcpu to a virtual machine. No more than max_vcpus may be added. The vcpu id is an integer in the range [0, max_vcpu_id).
The recommended max_vcpus value can be retrieved using the KVM_CAP_NR_VCPUS of the KVM_CHECK_EXTENSION ioctl() at run-time. The maximum possible value for max_vcpus can be retrieved using the KVM_CAP_MAX_VCPUS of the KVM_CHECK_EXTENSION ioctl() at run-time. """
Based on that and a quick look at the code, it looks to me like the set of valid vcpu_id values are a subset of the range of vcpu_idx-es, i.e. that kvm could decide to choose vcpu_id for the value of vcpu_idx. I don't think it should, but it could. Were the set of valid vcpu_id values not a subset of the set of supported vcpu_idx values, then one could argue that this is why.
I didn't want to get into explaining the why, I just wanted to state the fact.
Currently kvm->arch.idle_mask is indexed by vcpu_id, which implies that code like for_each_set_bit(vcpu_id, kvm->arch.idle_mask, online_vcpus) { vcpu = kvm_get_vcpu(kvm, vcpu_id);
you can also add a sentence to clarify that kvm_get_vcpu expects an vcpu_idx, not an vcpu_id.
maybe ...
do_stuff(vcpu);} is not legit. The trouble is, we do actually use kvm->arch.idle_mask
... s/legit./legit, because kvm_get_vcpu() expects a vcpu_idx and not a vcpu_id.
But I agree kvm_get_vcpu(kvm, vcpu_id); does not scream BUG at me while kvm_get_vcpu_by_idx(kvm, vcpu_id) would look much more suspicious.
[..]
otherwise looks good to me.
Reviewed-by: Claudio Imbrenda imbrenda@linux.ibm.com
Thanks for your reveiew!
Halil
[..]