Re: [Qemu-devel] [qom-cpu PATCH 2/2] i386: disable PMU passthrough mode by default

[Eduardo Habkost]

On Tue, Jul 23, 2013 at 08:01:29AM +0200, Igor Mammedov wrote:
> On Mon, 22 Jul 2013 16:25:35 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > Bug description: QEMU currently gets all bits from GET_SUPPORTED_CPUID
> > for CPUID leaf 0xA and passes them directly to the guest. This makes
> > the guest ABI depend on host kernel and host CPU capabilities, and
> > breaks live migration if we migrate between host with different
> > capabilities (e.g. different number of PMU counters).
> > 
> > This patch adds a "pmu-passthrough" property to X86CPU, and set it to
> > true only on "-cpu host", or on pc-*-1.5 and older machine-types.
> > 
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> >  include/hw/i386/pc.h  |  4 ++++
> >  target-i386/cpu-qom.h |  7 +++++++
> >  target-i386/cpu.c     | 11 ++++++++++-
> >  3 files changed, 21 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index 7fb97b0..3cea83f 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -235,6 +235,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >              .driver   = "virtio-net-pci",\
> >              .property = "any_layout",\
> >              .value    = "off",\
> > +        },{\
> > +            .driver = TYPE_X86_CPU,\
> > +            .property = "pmu-passthrough",\
> > +            .value = "on",\
> >          }
> >  
> >  #define PC_COMPAT_1_4 \
> > diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> > index 7e55e5f..b505a45 100644
> > --- a/target-i386/cpu-qom.h
> > +++ b/target-i386/cpu-qom.h
> > @@ -68,6 +68,13 @@ typedef struct X86CPU {
> >  
> >      /* Features that were filtered out because of missing host 
> > capabilities */
> >      uint32_t filtered_features[FEATURE_WORDS];
> > +
> > +    /* Pass all PMU CPUID bits to the guest directly from 
> > GET_SUPPORTED_CPUID.
> > +     * This can't be enabled by default because it breaks live-migration,
> > +     * as it makes the guest ABI change depending on host CPU/kernel
> > +     * capabilities.
> > +     */
> > +    bool pmu_passthrough;
> >  } X86CPU;
> >  
> >  static inline X86CPU *x86_env_get_cpu(CPUX86State *env)
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 41c81af..e192f63 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1475,17 +1475,25 @@ static void x86_cpu_get_feature_words(Object *obj, 
> > Visitor *v, void *opaque,
> >      error_propagate(errp, err);
> >  }
> >  
> > +static Property cpu_x86_properties[] = {
> > +    DEFINE_PROP_BOOL("pmu-passthrough", X86CPU, pmu_passthrough, false),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> >  static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> >                                  const char *name)
> >  {
> >      x86_def_t *def;
> >      int i;
> > +    Error *err = NULL;
> >  
> >      if (name == NULL) {
> >          return -1;
> >      }
> >      if (kvm_enabled() && strcmp(name, "host") == 0) {
> >          kvm_cpu_fill_host(x86_cpu_def);
> > +        object_property_set_bool(OBJECT(cpu), true, "pmu-passthrough", 
> > &err);
> > +        assert_no_error(err);
> Could this hunk be implemented using compat props?
> That would spare us dealing with it later.

Oh, I forgot we already have the qdev_prop_set_globals_for_type() hack
that would allow us to use model-specific compat_props.

But I never expected to have a "compat" property that will get set on
_all_ machine-types ("-cpu host" will have pmu-passthrough=true on all
machine-types). Would it make sense to do it?

Normally on cases like this we would just set a property default on the
host-x86-cpu class. But we still don't have the per-CPU-model X86CPU
subclasses, so today the defaults are coded in the init functions.

> 
> >          return 0;
> >      }
> >  
> > @@ -2017,7 +2025,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, 
> > uint32_t count,
> >          break;
> >      case 0xA:
> >          /* Architectural Performance Monitoring Leaf */
> > -        if (kvm_enabled()) {
> > +        if (kvm_enabled() && cpu->pmu_passthrough) {
> >              KVMState *s = cs->kvm_state;
> >  
> >              *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
> > @@ -2516,6 +2524,7 @@ static void x86_cpu_common_class_init(ObjectClass 
> > *oc, void *data)
> >      xcc->parent_realize = dc->realize;
> >      dc->realize = x86_cpu_realizefn;
> >      dc->bus_type = TYPE_ICC_BUS;
> > +    dc->props = cpu_x86_properties;
> >  
> >      xcc->parent_reset = cc->reset;
> >      cc->reset = x86_cpu_reset;
> 

-- 
Eduardo