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

[Paolo Bonzini]

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.

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