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

[Igor Mammedov]

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.

>          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;