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

[Eduardo Habkost]

On Tue, Jul 23, 2013 at 05:09:02PM +0200, Paolo Bonzini wrote:
> Il 23/07/2013 16:13, Eduardo Habkost ha scritto:
> > On Tue, Jul 23, 2013 at 11:18:03AM +0200, Paolo Bonzini wrote:
> >> Il 22/07/2013 21:25, Eduardo Habkost ha scritto:
> >>> 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.
> >>
> >> Can we just call the property "pmu"?  It doesn't have to be passthough.
> > 
> > Yes, but the only options we have today are "no PMU" and "passthrough
> > PMU". I wouldn't like to make "pmu=on" enable the passthrough behavior
> > implicitly (I don't want things that break live-migration to be enabled
> > without making it explicit that it is a host-dependent/passthrough
> > mode).
> 
> I think "passthrough PMU" should be considered a bug except of course
> with "-cpu host".
> 
> If "-cpu Nehalem,pmu=on" goes from passthrough to Nehalem-compatible in
> a future QEMU release, that'll be a bugfix.

Exactly. But then I don't understand your suggestion. We still need a
property to enable pasthrough behavior on old machine-types (not
perfect, but a best-effort way to try to keep compatibility), and I
named that option "pmu-passthrough". How do you think we should name it?

> 
> Paolo
> 
> > I considered creating a property named "pmu" and use "pmu=host" to
> > enable the current passthrough behavior, but:
> > 
> >>
> >> Later we can support setting the right values for leaf 0xA.  This way
> >> migration will work even for -cpu other than "-cpu host", and the same
> >> option will work.
> > 
> > Yes. I just don't know what will be the best way to specify the PMU
> > counters in the command-line/properties when we do it, so I thought it
> > would be better to create a "pmu" property only after we decide how
> > exactly it will look like.
> > 
> >>
> >> Paolo
> >>
> >>> 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);
> >>>          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