Re: [Qemu-devel] [PATCH 05/23] hyperv: ensure VP index equal to QEMU cpu_index

[Roman Kagan]

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.