Re: [Qemu-devel] [PATCH 1/1] cpu: report hyperv feature words through qom

[Eduardo Habkost]

On Tue, Jun 14, 2016 at 11:59:18PM +0300, Denis V. Lunev wrote:
> On 06/14/2016 11:54 PM, Eduardo Habkost wrote:
> > On Tue, Jun 14, 2016 at 11:45:08PM +0300, Denis V. Lunev wrote:
> > > On 06/14/2016 10:59 PM, Eduardo Habkost wrote:
> > > > On Tue, Jun 14, 2016 at 01:28:40PM +0300, Denis V. Lunev wrote:
> > > > > From: Evgeny Yakovlev <address@hidden>
> > > > > 
> > > > > This change adds hyperv feature words report through qom rpc.
> > > > > 
> > > > > When VM is configured with hyperv features enabled libvirt will check 
> > > > > that
> > > > > required featured words are set in cpuid leaf 40000003 through qom
> > > > > request.
> > > > > 
> > > > > Currently qemu does not report hyperv feature words which prevents 
> > > > > windows
> > > > > guests from starting with libvirt.
> > > > > 
> > > > > Signed-off-by: Evgeny Yakovlev <address@hidden>
> > > > > Signed-off-by: Denis V. Lunev <address@hidden>
> > > > > CC: Paolo Bonzini <address@hidden>
> > > > > CC: Richard Henderson <address@hidden>
> > > > > CC: Eduardo Habkost <address@hidden>
> > > > > CC: Marcelo Tosatti <address@hidden>
> > > > Which QEMU version did you use to test this? Some of those properties 
> > > > already
> > > > exist. See:
> > > > 
> > > >     static Property x86_cpu_properties[] = {
> > > >         [...]
> > > >         { .name  = "hv-spinlocks", .info  = &qdev_prop_spinlocks },
> > > >         DEFINE_PROP_BOOL("hv-relaxed", X86CPU, hyperv_relaxed_timing, 
> > > > false),
> > > >         DEFINE_PROP_BOOL("hv-vapic", X86CPU, hyperv_vapic, false),
> > > >         DEFINE_PROP_BOOL("hv-time", X86CPU, hyperv_time, false),
> > > >         DEFINE_PROP_BOOL("hv-crash", X86CPU, hyperv_crash, false),
> > > >         DEFINE_PROP_BOOL("hv-reset", X86CPU, hyperv_reset, false),
> > > >         DEFINE_PROP_BOOL("hv-vpindex", X86CPU, hyperv_vpindex, false),
> > > >         DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false),
> > > >         DEFINE_PROP_BOOL("hv-synic", X86CPU, hyperv_synic, false),
> > > >         DEFINE_PROP_BOOL("hv-stimer", X86CPU, hyperv_stimer, false),
> > > >         [...]
> > > >         DEFINE_PROP_STRING("hv-vendor-id", X86CPU, hyperv_vendor_id),
> > > >         DEFINE_PROP_END_OF_LIST()
> > > >     };
> > > > 
> > > > QEMU will crash if you try to register the properties twice:
> > > > 
> > > >     $ ./x86_64-softmmu/qemu-system-x86_64
> > > >     qemu-system-x86_64: 
> > > > /home/ehabkost/rh/proj/virt/qemu/target-i386/cpu.c:3094: 
> > > > x86_cpu_register_bit_prop: Assertion `fp->ptr == field' failed.
> > > >     Aborted (core dumped)
> > > > 
> > > > I like the idea of moving hyperv feature information inside the 
> > > > features array,
> > > > though.
> > > no, idea is a bit different.
> > > 
> > > The user selects properties in the command line to enable
> > > different HyperV enlightenments. This is how we do that
> > > and this is how the QEMU is expected to work.
> > > 
> > > After that libvirt starts to check that these properties do
> > > work. In order to do that it executes qom-get and expects
> > > to find enabled HyperV enlightenments in the guest CPUID.
> > > 
> > > This is the idea of this patch.
> > And why exactly moving hyperv feature information inside the
> > CPUX86State::features array wouldn't do exactly what you say
> > above?
> > 
> property remains ;)
> 
> OK, may be I have missed you point. I fear word "move"
> in your letter. I think we "add" it to features.

I mean we move data to a different struct field (from the
hyperv_* booleans to the features array), and let the existing
feature property system handle it.

Your patch does that partially, but it conflicts with the
existing properties registered at x86_cpu_properties.

But we also need to make kvm_arch_get_supported_cpuid() handle
the hyperv CPUID leaves (to replace the existing
kvm_check_extensions() and has_msr_* checks in
kvm_arch_init_vcpu()).

After we do that, some things should work automatically:
* New hv-* properties available in QOM and "-cpu";
* hyperv support for the "check" and "enforce" options;
* hyperv support in "-cpu host'; and
* hyperv information returned in the "feature-words" and
  "filtered-features" QOM properties.

See how FEAT_KVM is handled in target-i386/cpu.c and
kvm_arch_init_vcpu(), for reference.

-- 
Eduardo