Re: [Qemu-devel] [PATCH for-2.12 v2 2/2] i386/hyperv: error out if features requested but unsupported

[Roman Kagan]

On Wed, Mar 28, 2018 at 03:53:31PM -0300, Eduardo Habkost wrote:
> On Wed, Mar 28, 2018 at 06:30:24PM +0300, Roman Kagan wrote:
> > In order to guarantee compatibility on migration, QEMU should have
> > complete control over the features it announces to the guest via CPUID.
> > 
> > However, for a number of Hyper-V-related cpu properties, if the
> > corresponding feature is not supported by the underlying KVM, the
> > propery is silently ignored and the feature is not announced to the
> > guest.
> > 
> > Refuse to start with an error instead.
> > 
> > Signed-off-by: Roman Kagan <address@hidden>
> 
> Something I didn't consider before:
> 
> Will this block migration before it even starts, or will crash
> the VM only after all migration data was sent to the destination?
> 
> I didn't test it, but kvm_arch_init_vcpu() seems to be too late
> to block an invalid/unsupport configuration.

I just did a simple test, force-failing one of the checks and starting
QEMU with -incoming defer.  It refused to start with the expected error
message.  IOW in the migration case the destination will abort before
the source have started to send any data.

> Maybe we can simply call hyperv_handle_properties() earlier,
> inside x86_cpu_realizefn()?

Now it's

  ...
  x86_cpu_realizefn
    qemu_init_vcpu
      qemu_kvm_start_vcpu
        qemu_thread_create(qemu_kvm_cpu_thread_fn)
          [in vcpu thread]
          qemu_kvm_cpu_thread_fn
            kvm_init_vcpu
              kvm_arch_init_vcpu
                hyperv_handle_properties
                  [error return]
                [error return]
              [error return]
            exit(1)

> (I know it's very late for this kind of intrusive change in
> v2.12, but I still think it's a good idea to fix this as soon as
> possible.)

I agree that the current hyperv flag handling begs for cleanup but I
think this can wait for post-2.12.

> > ---
> > v1 -> v2:
> >  - indicate what flag requested the feature that can't be enabled in the
> >    error message
> >  - fix a typo in the error message for VP_RUNTIME

I just noticed that I missed hv-time being silently cleared, too (just
in a slightly different pattern), so I'll have to respin.

Thanks,
Roman.