| Message ID | 1366915386-14728-8-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 25 Apr 2013 15:43:06 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > The current code handling the CPUID 0xA leaf simply forwards all data > from GET_SUPPORTED_CPUID directly to the guest, breaking migration > between hosts with different number of PMU counters. > > This patch disables this behavior, except on older machine-types (for > compatibility) and on the "host" CPU model. Please, make it static property and use compat properties. Result will be simpler and much less will have to be redone/discarded after converting to the rest to properties and sub-classes. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > Cc: Gleb Natapov <gleb@redhat.com> > --- > hw/i386/pc_piix.c | 1 + > hw/i386/pc_q35.c | 1 + > target-i386/cpu.c | 25 ++++++++++++++++++++++++- > target-i386/cpu.h | 7 +++++++ > 4 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 9372f77..bbc7064 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -239,6 +239,7 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args) > { > x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE); > x86_cpu_compat_set_model("486", 0); > + x86_cpu_enable_pmu_passthrough(); > pc_init_pci(args); > } > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index fc566fd..9718e94 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -213,6 +213,7 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args) > { > x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE); > x86_cpu_compat_set_model("486", 0); > + x86_cpu_enable_pmu_passthrough(); > pc_q35_init(args); > } > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 4fc7527..602d00f 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -245,6 +245,16 @@ static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | > (1 << KVM_FEATURE_PV_EOI) | > (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > > +/* Enables passthrough of CPUID leaf 0xA by default, for compatibility with old > + * machine-types. > + */ > +static bool default_pmu_passthrough; > + > +void x86_cpu_enable_pmu_passthrough(void) > +{ > + default_pmu_passthrough = true; > +} > + > void disable_kvm_pv_eoi(void) > { > kvm_default_features &= ~(1UL << KVM_FEATURE_PV_EOI); > @@ -375,6 +385,12 @@ typedef struct x86_def_t { > int stepping; > FeatureWordArray features; > char model_id[48]; > + > + /* Enable direct passthrough of PMU leaf from the GET_SUPPORTED_CPUID > + * data returned by the kernel. This is not migration-safe and should > + * never be enabled by default. > + */ > + bool cpuid_pmu_passthrough; > } x86_def_t; > > #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) > @@ -1120,6 +1136,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) > x86_cpu_def->features[FEAT_KVM] = > kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX); > > + x86_cpu_def->cpuid_pmu_passthrough = true; > + > #endif /* CONFIG_KVM */ > } > > @@ -1735,9 +1753,13 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp) > return; > } > > + /* Defaults & compat bits that are not in the builtin_x86_defs table: */ > if (kvm_enabled()) { > def->features[FEAT_KVM] |= kvm_default_features; > } > + if (default_pmu_passthrough) { > + def->cpuid_pmu_passthrough = true; > + } > def->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR; > > object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp); > @@ -1755,6 +1777,7 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp) > env->features[FEAT_C000_0001_EDX] = def->features[FEAT_C000_0001_EDX]; > env->features[FEAT_7_0_EBX] = def->features[FEAT_7_0_EBX]; > env->cpuid_xlevel2 = def->xlevel2; > + env->cpuid_pmu_passthrough = def->cpuid_pmu_passthrough; > > object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp); > } > @@ -1986,7 +2009,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() && env->cpuid_pmu_passthrough) { > KVMState *s = cs->kvm_state; > > *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX); > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 1cd5d19..12c8bf1 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -843,6 +843,12 @@ typedef struct CPUX86State { > uint32_t cpuid_vendor3; > uint32_t cpuid_version; > FeatureWordArray features; > + > + /* Enable direct passthrough of PMU leaf from the GET_SUPPORTED_CPUID > + * data returned by the kernel. This is not migration-safe and should > + * never be enabled by default. > + */ > + bool cpuid_pmu_passthrough; > uint32_t cpuid_model[12]; > uint32_t cpuid_apic_id; > > @@ -1258,6 +1264,7 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w, > uint32_t feat_add, uint32_t feat_remove); > void x86_cpu_compat_set_level(const char *cpu_model, uint32_t level); > void x86_cpu_compat_set_model(const char *cpu_model, uint32_t model); > +void x86_cpu_enable_pmu_passthrough(void); > > > /* Return name of 32-bit register, from a R_* constant */ > -- > 1.8.1.4 >
On Fri, Apr 26, 2013 at 05:10:29PM +0200, Igor Mammedov wrote: > On Thu, 25 Apr 2013 15:43:06 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > The current code handling the CPUID 0xA leaf simply forwards all data > > from GET_SUPPORTED_CPUID directly to the guest, breaking migration > > between hosts with different number of PMU counters. > > > > This patch disables this behavior, except on older machine-types (for > > compatibility) and on the "host" CPU model. > Please, make it static property and use compat properties. > Result will be simpler and much less will have to be redone/discarded after > converting to the rest to properties and sub-classes. I was going to say that static properties were too much work to be done in time for 1.5, but you are right: in this specific case adding a static property for the cpuid_pmu_passthrough field looks very easy. I will give it a try. I will probably try to make the "model" field a static property as well. Then only x86_cpu_compat_set_features() would be kept, as converting feature flags to static properties will probably require more work. Thanks,
Am 26.04.2013 17:31, schrieb Eduardo Habkost: > On Fri, Apr 26, 2013 at 05:10:29PM +0200, Igor Mammedov wrote: >> On Thu, 25 Apr 2013 15:43:06 -0300 >> Eduardo Habkost <ehabkost@redhat.com> wrote: >> >>> The current code handling the CPUID 0xA leaf simply forwards all data >>> from GET_SUPPORTED_CPUID directly to the guest, breaking migration >>> between hosts with different number of PMU counters. >>> >>> This patch disables this behavior, except on older machine-types (for >>> compatibility) and on the "host" CPU model. >> Please, make it static property and use compat properties. >> Result will be simpler and much less will have to be redone/discarded after >> converting to the rest to properties and sub-classes. > > I was going to say that static properties were too much work to be done > in time for 1.5, but you are right: in this specific case adding a > static property for the cpuid_pmu_passthrough field looks very easy. I > will give it a try. I am hoping to get as initial set (though not all) of the static properties still into 1.5. Using them to fix CPUID bugs can then be done during Hard Freeze. :) Andreas
On Fri, 26 Apr 2013 17:33:18 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 26.04.2013 17:31, schrieb Eduardo Habkost: > > On Fri, Apr 26, 2013 at 05:10:29PM +0200, Igor Mammedov wrote: > >> On Thu, 25 Apr 2013 15:43:06 -0300 > >> Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > >>> The current code handling the CPUID 0xA leaf simply forwards all data > >>> from GET_SUPPORTED_CPUID directly to the guest, breaking migration > >>> between hosts with different number of PMU counters. > >>> > >>> This patch disables this behavior, except on older machine-types (for > >>> compatibility) and on the "host" CPU model. > >> Please, make it static property and use compat properties. > >> Result will be simpler and much less will have to be redone/discarded after > >> converting to the rest to properties and sub-classes. > > > > I was going to say that static properties were too much work to be done > > in time for 1.5, but you are right: in this specific case adding a > > static property for the cpuid_pmu_passthrough field looks very easy. I > > will give it a try. > > I am hoping to get as initial set (though not all) of the static > properties still into 1.5. Using them to fix CPUID bugs can then be done > during Hard Freeze. :) patch "[PATCH 02/10] target-i386: cpu: convert existing dynamic properties into static properties" should be enough for using model,level compat properties. > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg >
On Fri, Apr 26, 2013 at 05:39:15PM +0200, Igor Mammedov wrote: > On Fri, 26 Apr 2013 17:33:18 +0200 > Andreas Färber <afaerber@suse.de> wrote: > > > Am 26.04.2013 17:31, schrieb Eduardo Habkost: > > > On Fri, Apr 26, 2013 at 05:10:29PM +0200, Igor Mammedov wrote: > > >> On Thu, 25 Apr 2013 15:43:06 -0300 > > >> Eduardo Habkost <ehabkost@redhat.com> wrote: > > >> > > >>> The current code handling the CPUID 0xA leaf simply forwards all data > > >>> from GET_SUPPORTED_CPUID directly to the guest, breaking migration > > >>> between hosts with different number of PMU counters. > > >>> > > >>> This patch disables this behavior, except on older machine-types (for > > >>> compatibility) and on the "host" CPU model. > > >> Please, make it static property and use compat properties. > > >> Result will be simpler and much less will have to be redone/discarded after > > >> converting to the rest to properties and sub-classes. > > > > > > I was going to say that static properties were too much work to be done > > > in time for 1.5, but you are right: in this specific case adding a > > > static property for the cpuid_pmu_passthrough field looks very easy. I > > > will give it a try. > > > > I am hoping to get as initial set (though not all) of the static > > properties still into 1.5. Using them to fix CPUID bugs can then be done > > during Hard Freeze. :) > patch "[PATCH 02/10] target-i386: cpu: convert existing dynamic properties > into static properties" should be enough for using model,level compat > properties. ...except that we don't have X86CPU model subclasses yet, so we can't set model-specific compat properties using compat_props. Maybe we can make compat_props work for pmu-passthrough, but it may be not completely straightforward because I would like to keep it enabled by default on -cpu host.
On Fri, 26 Apr 2013 14:30:40 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Apr 26, 2013 at 05:39:15PM +0200, Igor Mammedov wrote: > > On Fri, 26 Apr 2013 17:33:18 +0200 > > Andreas Färber <afaerber@suse.de> wrote: > > > > > Am 26.04.2013 17:31, schrieb Eduardo Habkost: > > > > On Fri, Apr 26, 2013 at 05:10:29PM +0200, Igor Mammedov wrote: > > > >> On Thu, 25 Apr 2013 15:43:06 -0300 > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote: > > > >> > > > >>> The current code handling the CPUID 0xA leaf simply forwards all data > > > >>> from GET_SUPPORTED_CPUID directly to the guest, breaking migration > > > >>> between hosts with different number of PMU counters. > > > >>> > > > >>> This patch disables this behavior, except on older machine-types (for > > > >>> compatibility) and on the "host" CPU model. > > > >> Please, make it static property and use compat properties. > > > >> Result will be simpler and much less will have to be redone/discarded after > > > >> converting to the rest to properties and sub-classes. > > > > > > > > I was going to say that static properties were too much work to be done > > > > in time for 1.5, but you are right: in this specific case adding a > > > > static property for the cpuid_pmu_passthrough field looks very easy. I > > > > will give it a try. > > > > > > I am hoping to get as initial set (though not all) of the static > > > properties still into 1.5. Using them to fix CPUID bugs can then be done > > > during Hard Freeze. :) > > patch "[PATCH 02/10] target-i386: cpu: convert existing dynamic properties > > into static properties" should be enough for using model,level compat > > properties. > > ...except that we don't have X86CPU model subclasses yet, so we can't > set model-specific compat properties using compat_props. X86CPU could serve here > > Maybe we can make compat_props work for pmu-passthrough, but it may be > not completely straightforward because I would like to keep it enabled > by default on -cpu host. Maybe for 'host' hack realize() to override default, will do and later we can move it to host_subclass. it's easier to re-factor than compact functions > > -- > Eduardo >
On Fri, Apr 26, 2013 at 07:41:10PM +0200, Igor Mammedov wrote: > On Fri, 26 Apr 2013 14:30:40 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Fri, Apr 26, 2013 at 05:39:15PM +0200, Igor Mammedov wrote: > > > On Fri, 26 Apr 2013 17:33:18 +0200 > > > Andreas Färber <afaerber@suse.de> wrote: > > > > > > > Am 26.04.2013 17:31, schrieb Eduardo Habkost: > > > > > On Fri, Apr 26, 2013 at 05:10:29PM +0200, Igor Mammedov wrote: > > > > >> On Thu, 25 Apr 2013 15:43:06 -0300 > > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > >> > > > > >>> The current code handling the CPUID 0xA leaf simply forwards all data > > > > >>> from GET_SUPPORTED_CPUID directly to the guest, breaking migration > > > > >>> between hosts with different number of PMU counters. > > > > >>> > > > > >>> This patch disables this behavior, except on older machine-types (for > > > > >>> compatibility) and on the "host" CPU model. > > > > >> Please, make it static property and use compat properties. > > > > >> Result will be simpler and much less will have to be redone/discarded after > > > > >> converting to the rest to properties and sub-classes. > > > > > > > > > > I was going to say that static properties were too much work to be done > > > > > in time for 1.5, but you are right: in this specific case adding a > > > > > static property for the cpuid_pmu_passthrough field looks very easy. I > > > > > will give it a try. > > > > > > > > I am hoping to get as initial set (though not all) of the static > > > > properties still into 1.5. Using them to fix CPUID bugs can then be done > > > > during Hard Freeze. :) > > > patch "[PATCH 02/10] target-i386: cpu: convert existing dynamic properties > > > into static properties" should be enough for using model,level compat > > > properties. > > > > ...except that we don't have X86CPU model subclasses yet, so we can't > > set model-specific compat properties using compat_props. > X86CPU could serve here How? If we need to set model=8 only on the "486" CPU model, how would we do that in the compat_props table without subclasses? > > > > > Maybe we can make compat_props work for pmu-passthrough, but it may be > > not completely straightforward because I would like to keep it enabled > > by default on -cpu host. > Maybe for 'host' hack realize() to override default, will do and later we > can move it to host_subclass. Yes, it's doable. And I don't think we even need to hack methods, we just need a x86_def_t field that indicates that this is a model that overrides the default (and that field would be true only for "host"). But I still don't see how it would be possible to use compat_props for the 486 model=8 fix or for the n270 movbe fix.
On Fri, 26 Apr 2013 16:01:15 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Apr 26, 2013 at 07:41:10PM +0200, Igor Mammedov wrote: > > On Fri, 26 Apr 2013 14:30:40 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Fri, Apr 26, 2013 at 05:39:15PM +0200, Igor Mammedov wrote: > > > > On Fri, 26 Apr 2013 17:33:18 +0200 > > > > Andreas Färber <afaerber@suse.de> wrote: > > > > > > > > > Am 26.04.2013 17:31, schrieb Eduardo Habkost: > > > > > > On Fri, Apr 26, 2013 at 05:10:29PM +0200, Igor Mammedov wrote: > > > > > >> On Thu, 25 Apr 2013 15:43:06 -0300 > > > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > >> > > > > > >>> The current code handling the CPUID 0xA leaf simply forwards all data > > > > > >>> from GET_SUPPORTED_CPUID directly to the guest, breaking migration > > > > > >>> between hosts with different number of PMU counters. > > > > > >>> > > > > > >>> This patch disables this behavior, except on older machine-types (for > > > > > >>> compatibility) and on the "host" CPU model. > > > > > >> Please, make it static property and use compat properties. > > > > > >> Result will be simpler and much less will have to be redone/discarded after > > > > > >> converting to the rest to properties and sub-classes. > > > > > > > > > > > > I was going to say that static properties were too much work to be done > > > > > > in time for 1.5, but you are right: in this specific case adding a > > > > > > static property for the cpuid_pmu_passthrough field looks very easy. I > > > > > > will give it a try. > > > > > > > > > > I am hoping to get as initial set (though not all) of the static > > > > > properties still into 1.5. Using them to fix CPUID bugs can then be done > > > > > during Hard Freeze. :) > > > > patch "[PATCH 02/10] target-i386: cpu: convert existing dynamic properties > > > > into static properties" should be enough for using model,level compat > > > > properties. > > > > > > ...except that we don't have X86CPU model subclasses yet, so we can't > > > set model-specific compat properties using compat_props. > > X86CPU could serve here > > How? > > If we need to set model=8 only on the "486" CPU model, how would we do > that in the compat_props table without subclasses? for model, it would be its possible with custom setter, might be preferred way since it's local to cpu.c, and could be easily replaced with with one-liner once sub-classes are in tree. > > > > > > > > > Maybe we can make compat_props work for pmu-passthrough, but it may be > > > not completely straightforward because I would like to keep it enabled > > > by default on -cpu host. > > Maybe for 'host' hack realize() to override default, will do and later we > > can move it to host_subclass. > > Yes, it's doable. And I don't think we even need to hack methods, we > just need a x86_def_t field that indicates that this is a model that > overrides the default (and that field would be true only for "host"). Extended x86_def_t might be harder to handle when switching to sub-classes, although it's hard to say without comparing patches for both ways. > > But I still don't see how it would be possible to use compat_props for > the 486 model=8 fix or for the n270 movbe fix. > It's impossible to implement "n270 movbe" fixup with compat props currently, so 5/7 + 1/7 looks ok. We can drop them once features converted to static properties + sub-classes.
On Tue, Apr 30, 2013 at 07:04:23PM +0200, Igor Mammedov wrote: > On Fri, 26 Apr 2013 16:01:15 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Fri, Apr 26, 2013 at 07:41:10PM +0200, Igor Mammedov wrote: > > > On Fri, 26 Apr 2013 14:30:40 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Fri, Apr 26, 2013 at 05:39:15PM +0200, Igor Mammedov wrote: > > > > > On Fri, 26 Apr 2013 17:33:18 +0200 > > > > > Andreas Färber <afaerber@suse.de> wrote: > > > > > > > > > > > Am 26.04.2013 17:31, schrieb Eduardo Habkost: > > > > > > > On Fri, Apr 26, 2013 at 05:10:29PM +0200, Igor Mammedov wrote: > > > > > > >> On Thu, 25 Apr 2013 15:43:06 -0300 > > > > > > >> Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > >> > > > > > > >>> The current code handling the CPUID 0xA leaf simply forwards all data > > > > > > >>> from GET_SUPPORTED_CPUID directly to the guest, breaking migration > > > > > > >>> between hosts with different number of PMU counters. > > > > > > >>> > > > > > > >>> This patch disables this behavior, except on older machine-types (for > > > > > > >>> compatibility) and on the "host" CPU model. > > > > > > >> Please, make it static property and use compat properties. > > > > > > >> Result will be simpler and much less will have to be redone/discarded after > > > > > > >> converting to the rest to properties and sub-classes. > > > > > > > > > > > > > > I was going to say that static properties were too much work to be done > > > > > > > in time for 1.5, but you are right: in this specific case adding a > > > > > > > static property for the cpuid_pmu_passthrough field looks very easy. I > > > > > > > will give it a try. > > > > > > > > > > > > I am hoping to get as initial set (though not all) of the static > > > > > > properties still into 1.5. Using them to fix CPUID bugs can then be done > > > > > > during Hard Freeze. :) > > > > > patch "[PATCH 02/10] target-i386: cpu: convert existing dynamic properties > > > > > into static properties" should be enough for using model,level compat > > > > > properties. > > > > > > > > ...except that we don't have X86CPU model subclasses yet, so we can't > > > > set model-specific compat properties using compat_props. > > > X86CPU could serve here > > > > How? > > > > If we need to set model=8 only on the "486" CPU model, how would we do > > that in the compat_props table without subclasses? > for model, it would be its possible with custom setter, might be preferred way > since it's local to cpu.c, and could be easily replaced with with one-liner once > sub-classes are in tree. A custom setter has to be set inside class_init, because instance_init is run _after_ global properties are set, but we still don't have subclasses so we can't have a different setter for "486" or a compat_props item that would affect only "486". How exactly would this solution look like? I can't see it. (My main question isn't about the implementation inside cpu.c, but how the machine-type code inside pc_piix.c would look like). > > > > > > > > > > > > > > Maybe we can make compat_props work for pmu-passthrough, but it may be > > > > not completely straightforward because I would like to keep it enabled > > > > by default on -cpu host. > > > Maybe for 'host' hack realize() to override default, will do and later we > > > can move it to host_subclass. > > > > Yes, it's doable. And I don't think we even need to hack methods, we > > just need a x86_def_t field that indicates that this is a model that > > overrides the default (and that field would be true only for "host"). > Extended x86_def_t might be harder to handle when switching to sub-classes, > although it's hard to say without comparing patches for both ways. I believe it's impossible to implement any behavior that will change depending on the CPU model (enabling PMU passthrough on "host", disabling on all other models) without extending x86_def_t. x86_def_t data is the only thing we get back from cpu_x86_find_by_name(). Would you prefer to change cpu_x86_find_by_name() to get a X86CPU* as argument, too? I was trying to avoid that, but it is doable. > > > > > But I still don't see how it would be possible to use compat_props for > > the 486 model=8 fix or for the n270 movbe fix. > > > It's impossible to implement "n270 movbe" fixup with compat props currently, > so 5/7 + 1/7 looks ok. We can drop them once features converted to static properties > + sub-classes. True. But why this doesn't apply to 6/7 (486 model=8) as well?
Am 30.04.2013 21:57, schrieb Eduardo Habkost: > On Tue, Apr 30, 2013 at 07:04:23PM +0200, Igor Mammedov wrote: >> It's impossible to implement "n270 movbe" fixup with compat props currently, >> so 5/7 + 1/7 looks ok. We can drop them once features converted to static properties >> + sub-classes. > > True. But why this doesn't apply to 6/7 (486 model=8) as well? Eduardo, this series is "RFC". Are you going to grant permission to pick individual agreed-on patches from it or are you planning to resend? Andreas
On Wed, May 01, 2013 at 01:14:48PM +0200, Andreas Färber wrote: > Am 30.04.2013 21:57, schrieb Eduardo Habkost: > > On Tue, Apr 30, 2013 at 07:04:23PM +0200, Igor Mammedov wrote: > >> It's impossible to implement "n270 movbe" fixup with compat props currently, > >> so 5/7 + 1/7 looks ok. We can drop them once features converted to static properties > >> + sub-classes. > > > > True. But why this doesn't apply to 6/7 (486 model=8) as well? > > Eduardo, this series is "RFC". Are you going to grant permission to pick > individual agreed-on patches from it or are you planning to resend? I was planning to rebase and resend because of the other changes entering qom-cpu and to address the concerns about the other patches. But if you want to pick patches directly from this RFC series, I don't mind at all. It's up to you.
Am 02.05.2013 16:43, schrieb Eduardo Habkost: > On Wed, May 01, 2013 at 01:14:48PM +0200, Andreas Färber wrote: >> Am 30.04.2013 21:57, schrieb Eduardo Habkost: >>> On Tue, Apr 30, 2013 at 07:04:23PM +0200, Igor Mammedov wrote: >>>> It's impossible to implement "n270 movbe" fixup with compat props currently, >>>> so 5/7 + 1/7 looks ok. We can drop them once features converted to static properties >>>> + sub-classes. >>> >>> True. But why this doesn't apply to 6/7 (486 model=8) as well? >> >> Eduardo, this series is "RFC". Are you going to grant permission to pick >> individual agreed-on patches from it or are you planning to resend? > > I was planning to rebase and resend because of the other changes > entering qom-cpu and to address the concerns about the other patches. > But if you want to pick patches directly from this RFC series, I don't > mind at all. It's up to you. OK. Could you look at the series I posted yesterday? If you're okay with it and Anthony doesn't get a heart attack due to my qdev creativity ;) I'd like to rebase the remainder of this series on it. Andreas
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 9372f77..bbc7064 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -239,6 +239,7 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args) { x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE); x86_cpu_compat_set_model("486", 0); + x86_cpu_enable_pmu_passthrough(); pc_init_pci(args); } diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index fc566fd..9718e94 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -213,6 +213,7 @@ static void pc_q35_init_1_4(QEMUMachineInitArgs *args) { x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE); x86_cpu_compat_set_model("486", 0); + x86_cpu_enable_pmu_passthrough(); pc_q35_init(args); } diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 4fc7527..602d00f 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -245,6 +245,16 @@ static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | (1 << KVM_FEATURE_PV_EOI) | (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); +/* Enables passthrough of CPUID leaf 0xA by default, for compatibility with old + * machine-types. + */ +static bool default_pmu_passthrough; + +void x86_cpu_enable_pmu_passthrough(void) +{ + default_pmu_passthrough = true; +} + void disable_kvm_pv_eoi(void) { kvm_default_features &= ~(1UL << KVM_FEATURE_PV_EOI); @@ -375,6 +385,12 @@ typedef struct x86_def_t { int stepping; FeatureWordArray features; char model_id[48]; + + /* Enable direct passthrough of PMU leaf from the GET_SUPPORTED_CPUID + * data returned by the kernel. This is not migration-safe and should + * never be enabled by default. + */ + bool cpuid_pmu_passthrough; } x86_def_t; #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) @@ -1120,6 +1136,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) x86_cpu_def->features[FEAT_KVM] = kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX); + x86_cpu_def->cpuid_pmu_passthrough = true; + #endif /* CONFIG_KVM */ } @@ -1735,9 +1753,13 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp) return; } + /* Defaults & compat bits that are not in the builtin_x86_defs table: */ if (kvm_enabled()) { def->features[FEAT_KVM] |= kvm_default_features; } + if (default_pmu_passthrough) { + def->cpuid_pmu_passthrough = true; + } def->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR; object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp); @@ -1755,6 +1777,7 @@ static void cpu_x86_register(X86CPU *cpu, const char *name, Error **errp) env->features[FEAT_C000_0001_EDX] = def->features[FEAT_C000_0001_EDX]; env->features[FEAT_7_0_EBX] = def->features[FEAT_7_0_EBX]; env->cpuid_xlevel2 = def->xlevel2; + env->cpuid_pmu_passthrough = def->cpuid_pmu_passthrough; object_property_set_str(OBJECT(cpu), def->model_id, "model-id", errp); } @@ -1986,7 +2009,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() && env->cpuid_pmu_passthrough) { KVMState *s = cs->kvm_state; *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX); diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 1cd5d19..12c8bf1 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -843,6 +843,12 @@ typedef struct CPUX86State { uint32_t cpuid_vendor3; uint32_t cpuid_version; FeatureWordArray features; + + /* Enable direct passthrough of PMU leaf from the GET_SUPPORTED_CPUID + * data returned by the kernel. This is not migration-safe and should + * never be enabled by default. + */ + bool cpuid_pmu_passthrough; uint32_t cpuid_model[12]; uint32_t cpuid_apic_id; @@ -1258,6 +1264,7 @@ void x86_cpu_compat_set_features(const char *cpu_model, FeatureWord w, uint32_t feat_add, uint32_t feat_remove); void x86_cpu_compat_set_level(const char *cpu_model, uint32_t level); void x86_cpu_compat_set_model(const char *cpu_model, uint32_t model); +void x86_cpu_enable_pmu_passthrough(void); /* Return name of 32-bit register, from a R_* constant */
The current code handling the CPUID 0xA leaf simply forwards all data from GET_SUPPORTED_CPUID directly to the guest, breaking migration between hosts with different number of PMU counters. This patch disables this behavior, except on older machine-types (for compatibility) and on the "host" CPU model. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Cc: Gleb Natapov <gleb@redhat.com> --- hw/i386/pc_piix.c | 1 + hw/i386/pc_q35.c | 1 + target-i386/cpu.c | 25 ++++++++++++++++++++++++- target-i386/cpu.h | 7 +++++++ 4 files changed, 33 insertions(+), 1 deletion(-)