On Tue, Jun 13, 2017 at 03:57:52PM -0300, Eduardo Habkost wrote: > On Tue, Jun 06, 2017 at 09:19:30PM +0300, Roman Kagan wrote: > > Hyper-V identifies vcpus by the virtual processor (VP) index which is > > normally queried by the guest via HV_X64_MSR_VP_INDEX msr. > > > > It has to be owned by QEMU in order to preserve it across migration. > > > > However, the current implementation in KVM doesn't allow to set this > > msr, and KVM uses its own notion of VP index. Fortunately, the way > > vcpus are created in QEMU/KVM basically guarantees that the KVM value is > > equal to QEMU cpu_index. > > This might not be true in the future. cpu_index is not a good > identifier for CPUs, and we would like to remove it in the > future. > > But it looks like we have no choice, see extra comments below:
> > +static void hyperv_set_vp_index(CPUState *cs) > > +{ > > + struct { > > + struct kvm_msrs info; > > + struct kvm_msr_entry entries[1]; > > + } msr_data; > > + int ret; > > + > > + msr_data.info.nmsrs = 1; > > + msr_data.entries[0].index = HV_X64_MSR_VP_INDEX; > > + > > + /* > > + * Some kernels don't support setting HV_X64_MSR_VP_INDEX. However, > > + * they use VP_INDEX equal to cpu_index due to the way vcpus are > > created. > > + * So ignore the errors from SET_MSRS, but verify that GET_MSRS > > returns the > > + * expected value. > > + */ > > Oh. This sounds like a problem. As reference for others, this > is the KVM code in kvm_hv_get_msr(): > > case HV_X64_MSR_VP_INDEX: { > int r; > struct kvm_vcpu *v; > > kvm_for_each_vcpu(r, v, vcpu->kvm) { > if (v == vcpu) { > data = r; > break; > } > } > break; > } > > The problem with that is that it will break as soon as we create > VCPUs in a different order. Unsolvable on hosts that don't allow > HV_X64_MSR_VP_INDEX to be set, however. Right, thanks for putting together a detailed explanation. This was a thinko back then, not to have HV_X64_MSR_VP_INDEX maintained by QEMU. I'm going to post a patch to KVM fixing that. Meanwhile QEMU needs a way to maintain its notion of vp_index that is 1) in sync with kernel's notion 2) also with kernels that don't support setting the msr 3) persistent across migrations cpu_index looked like a perfect candidate. > > + msr_data.entries[0].data = cs->cpu_index; > > + kvm_vcpu_ioctl(cs, KVM_SET_MSRS, &msr_data); > > + ret = kvm_vcpu_ioctl(cs, KVM_GET_MSRS, &msr_data); > > + assert(ret == 1); > > + assert(msr_data.entries[0].data == cs->cpu_index); > > If KVM already initializes the MSR to cpu_index and we will abort > if it was not set to anything except cpu_index here, why exactly > do we need the KVM_SET_MSRS call? The kernel made no obligations to keep initializing its vp_index identically to QEMU's cpu_index. So we'd better check and abort if that got out of sync. Once KVM gains the ability to learn vp_index from QEMU we'll no longer depend on that as we'll do explicit synchronization. > > +} > > + > > static int hyperv_handle_properties(CPUState *cs) > > { > > X86CPU *cpu = X86_CPU(cs); > > CPUX86State *env = &cpu->env; > > > > + hyperv_set_vp_index(cs); > > + > > The purpose of hyperv_handle_properties() is just to initialize > cpu->features[] CPUID data based on the hyperv options. I suggest > creating a new hyperv_vcpu_init() or x86_cpu_hyperv_init() > function for these initialization steps. Sounds like a good idea, thanks. Roman.