| Message ID | 20200929061217.118440-1-like.xu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] target/i386: add "-cpu, lbr-fmt=*" support to enable guest LBR | expand |
(CCing the people from the thread, as kvm_exact_match_flags would be useful for INTEL_PT_IP_LIP) On Tue, Sep 29, 2020 at 02:12:17PM +0800, Like Xu wrote: > The last branch recording (LBR) is a performance monitor unit (PMU) > feature on Intel processors that records a running trace of the most > recent branches taken by the processor in the LBR stack. The QEMU > could configure whether it's enabled or not for each guest via CLI. > > The LBR feature would be enabled on the guest if: > - the KVM is enabled and the PMU is enabled and, > - the msr-based-feature IA32_PERF_CAPABILITIES is supporterd on KVM and, > - the supported returned value for lbr_fmt from this msr is not zero and, > - the requested guest vcpu model does support FEAT_1_ECX.CPUID_EXT_PDCM, > - the configured lbr-fmt value is the same as the host lbr_fmt value. > > Cc: Eduardo Habkost <ehabkost@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Like Xu <like.xu@linux.intel.com> The approach below looks better, thanks! Only one problem below, with a few suggestions and questions: > --- > target/i386/cpu.c | 16 ++++++++++++++++ > target/i386/cpu.h | 10 ++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 3ffd877dd5..b10344be01 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -6461,6 +6461,13 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose) > x86_cpu_get_supported_feature_word(w, false); > uint64_t requested_features = env->features[w]; > uint64_t unavailable_features = requested_features & ~host_feat; > + if (w == FEAT_PERF_CAPABILITIES && > + (requested_features & PERF_CAP_LBR_FMT)) { > + if ((host_feat & PERF_CAP_LBR_FMT) != > + (requested_features & PERF_CAP_LBR_FMT)) { > + unavailable_features |= PERF_CAP_LBR_FMT; > + } > + } This looks correct, but needs to be conditional on kvm_enabled(). I also have a suggestion: instead of hardcoding the PERF_CAPABILITIES rules in this loop, this could become a FeatureWordInfo field. It would be very useful for other features like intel-pt, where we need some bits to match the host too. Could you please check if the following patch works? Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- diff --git a/target/i386/cpu.c b/target/i386/cpu.c index b10344be010..d4107dcc026 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -704,6 +704,8 @@ typedef struct FeatureWordInfo { uint64_t migratable_flags; /* Feature flags known to be migratable */ /* Features that shouldn't be auto-enabled by "-cpu host" */ uint64_t no_autoenable_flags; + /* Bits that must match host exactly when using KVM */ + uint64_t kvm_exact_match_flags; } FeatureWordInfo; static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { @@ -1143,6 +1145,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .msr = { .index = MSR_IA32_PERF_CAPABILITIES, }, + /* + * KVM is not able to emulate a VCPU with LBR_FMT different + * from the host, so LBR_FMT must match the host exactly. + */ + .kvm_exact_match_flags = PERF_CAP_LBR_FMT, }, [FEAT_VMX_PROCBASED_CTLS] = { @@ -6457,16 +6464,15 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose) } for (w = 0; w < FEATURE_WORDS; w++) { + FeatureWordInfo *fi = &feature_word_info[w]; uint64_t host_feat = x86_cpu_get_supported_feature_word(w, false); uint64_t requested_features = env->features[w]; uint64_t unavailable_features = requested_features & ~host_feat; - if (w == FEAT_PERF_CAPABILITIES && - (requested_features & PERF_CAP_LBR_FMT)) { - if ((host_feat & PERF_CAP_LBR_FMT) != - (requested_features & PERF_CAP_LBR_FMT)) { - unavailable_features |= PERF_CAP_LBR_FMT; - } + if (kvm_enabled()) { + uint64_t mismatches = (requested_features ^ host_feat) & + fi->kvm_exact_match_flags; + mark_unavailable_features(cpu, w, mismatches, "feature doesn't match host"); } mark_unavailable_features(cpu, w, unavailable_features, prefix); } --------------------------- > mark_unavailable_features(cpu, w, unavailable_features, prefix); > } > > @@ -6533,6 +6540,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > } > } > > + if (cpu->lbr_fmt) { > + if (!cpu->enable_pmu) { > + error_setg(errp, "LBR is unsupported since guest PMU is disabled."); > + return; > + } This is not wrong, but looks like an obstacle for making the feature migratable and needs to be addressed as a follow up if you want live migration support. CPUID[0xA] is exposing host CPUID directly if enable_pmu is enabled. Also, I don't understand why PDCM is being silently disabled at cpu_x86_cpuid() if enable_pmu is not set. Isn't it valid to set PDCM=1 without CPUID[0xA]? > + env->features[FEAT_PERF_CAPABILITIES] |= cpu->lbr_fmt; > + } > + > /* mwait extended info: needed for Core compatibility */ > /* We always wake on interrupt even if host does not have the capability */ > cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE; > @@ -7157,6 +7172,7 @@ static Property x86_cpu_properties[] = { > #endif > DEFINE_PROP_INT32("node-id", X86CPU, node_id, CPU_UNSET_NUMA_NODE_ID), > DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false), > + DEFINE_PROP_UINT8("lbr-fmt", X86CPU, lbr_fmt, 0), > > DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts, > HYPERV_SPINLOCK_NEVER_NOTIFY), > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index f519d2bfd4..c1cf8b7160 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -357,6 +357,7 @@ typedef enum X86Seg { > #define ARCH_CAP_TSX_CTRL_MSR (1<<7) > > #define MSR_IA32_PERF_CAPABILITIES 0x345 > +#define PERF_CAP_LBR_FMT 0x3f > > #define MSR_IA32_TSX_CTRL 0x122 > #define MSR_IA32_TSCDEADLINE 0x6e0 > @@ -1701,6 +1702,15 @@ struct X86CPU { > */ > bool enable_pmu; > > + /* > + * Configure LBR_FMT bits on IA32_PERF_CAPABILITIES MSR. > + * This can't be enabled by default yet because it doesn't have > + * ABI stability guarantees, as it is only allowed to pass all > + * LBR_FMT bits returned by kvm_arch_get_supported_msr_feature() > + * (that depends on host CPU and kernel capabilities) to the guest. > + */ > + uint8_t lbr_fmt; > + > /* LMCE support can be enabled/disabled via cpu option 'lmce=on/off'. It is > * disabled by default to avoid breaking migration between QEMU with > * different LMCE configurations. > -- > 2.21.3 >
Hi Eduardo, On 2020/9/30 1:38, Eduardo Habkost wrote: > (CCing the people from the thread, as kvm_exact_match_flags would > be useful for INTEL_PT_IP_LIP) > > On Tue, Sep 29, 2020 at 02:12:17PM +0800, Like Xu wrote: >> The last branch recording (LBR) is a performance monitor unit (PMU) >> feature on Intel processors that records a running trace of the most >> recent branches taken by the processor in the LBR stack. The QEMU >> could configure whether it's enabled or not for each guest via CLI. >> >> The LBR feature would be enabled on the guest if: >> - the KVM is enabled and the PMU is enabled and, >> - the msr-based-feature IA32_PERF_CAPABILITIES is supporterd on KVM and, >> - the supported returned value for lbr_fmt from this msr is not zero and, >> - the requested guest vcpu model does support FEAT_1_ECX.CPUID_EXT_PDCM, >> - the configured lbr-fmt value is the same as the host lbr_fmt value. >> >> Cc: Eduardo Habkost <ehabkost@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Like Xu <like.xu@linux.intel.com> > > The approach below looks better, thanks! Only one problem below, > with a few suggestions and questions: > >> --- >> target/i386/cpu.c | 16 ++++++++++++++++ >> target/i386/cpu.h | 10 ++++++++++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index 3ffd877dd5..b10344be01 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -6461,6 +6461,13 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose) >> x86_cpu_get_supported_feature_word(w, false); >> uint64_t requested_features = env->features[w]; >> uint64_t unavailable_features = requested_features & ~host_feat; >> + if (w == FEAT_PERF_CAPABILITIES && >> + (requested_features & PERF_CAP_LBR_FMT)) { >> + if ((host_feat & PERF_CAP_LBR_FMT) != >> + (requested_features & PERF_CAP_LBR_FMT)) { >> + unavailable_features |= PERF_CAP_LBR_FMT; >> + } >> + } > > This looks correct, but needs to be conditional on kvm_enabled(). > > I also have a suggestion: instead of hardcoding the > PERF_CAPABILITIES rules in this loop, this could become a > FeatureWordInfo field. It would be very useful for other > features like intel-pt, where we need some bits to match the host > too. The idea looks good to me. > > Could you please check if the following patch works? > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index b10344be010..d4107dcc026 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -704,6 +704,8 @@ typedef struct FeatureWordInfo { > uint64_t migratable_flags; /* Feature flags known to be migratable */ > /* Features that shouldn't be auto-enabled by "-cpu host" */ > uint64_t no_autoenable_flags; > + /* Bits that must match host exactly when using KVM */ > + uint64_t kvm_exact_match_flags; > } FeatureWordInfo; > > static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > @@ -1143,6 +1145,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > .msr = { > .index = MSR_IA32_PERF_CAPABILITIES, > }, > + /* > + * KVM is not able to emulate a VCPU with LBR_FMT different > + * from the host, so LBR_FMT must match the host exactly. > + */ > + .kvm_exact_match_flags = PERF_CAP_LBR_FMT, > }, > > [FEAT_VMX_PROCBASED_CTLS] = { > @@ -6457,16 +6464,15 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose) > } > > for (w = 0; w < FEATURE_WORDS; w++) { > + FeatureWordInfo *fi = &feature_word_info[w]; > uint64_t host_feat = > x86_cpu_get_supported_feature_word(w, false); > uint64_t requested_features = env->features[w]; > uint64_t unavailable_features = requested_features & ~host_feat; > - if (w == FEAT_PERF_CAPABILITIES && > - (requested_features & PERF_CAP_LBR_FMT)) { > - if ((host_feat & PERF_CAP_LBR_FMT) != > - (requested_features & PERF_CAP_LBR_FMT)) { > - unavailable_features |= PERF_CAP_LBR_FMT; > - } > + if (kvm_enabled()) { > + uint64_t mismatches = (requested_features ^ host_feat) & > + fi->kvm_exact_match_flags; > + mark_unavailable_features(cpu, w, mismatches, "feature doesn't match host"); > } > mark_unavailable_features(cpu, w, unavailable_features, prefix); > } > > --------------------------- I may refine this part in this way: for (w = 0; w < FEATURE_WORDS; w++) { FeatureWordInfo *fi = &feature_word_info[w]; uint64_t match_flags = fi->kvm_exact_match_flags; uint64_t host_feat = x86_cpu_get_supported_feature_word(w, false); uint64_t requested_features = env->features[w]; uint64_t unavailable_features = requested_features & ~host_feat; if (kvm_enabled() && match_flags) { uint64_t mismatches = (requested_features & match_flags) && (requested_features ^ host_feat) & match_flags; mark_unavailable_features(cpu, w, mismatches, "feature doesn't match host"); unavailable_features &= ~match_flags; } mark_unavailable_features(cpu, w, unavailable_features, prefix); } which helps to: - make "-cpu host,pmu=true,lbr-fmt=0" just work as disable-lbr; - save a little overhead to get mismatches for non-related FEATURE_WORDS; - avoid to do mark_unavailable_features twice; > >> mark_unavailable_features(cpu, w, unavailable_features, prefix); >> } >> >> @@ -6533,6 +6540,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) >> } >> } >> >> + if (cpu->lbr_fmt) { >> + if (!cpu->enable_pmu) { >> + error_setg(errp, "LBR is unsupported since guest PMU is disabled."); >> + return; >> + } > > This is not wrong, but looks like an obstacle for making the > feature migratable and needs to be addressed as a follow up if > you want live migration support. CPUID[0xA] is exposing host > CPUID directly if enable_pmu is enabled. It's true. I have noticed the issue as well and will follow up to make pmu-enabled guest migratable. > > Also, I don't understand why PDCM is being silently disabled at > cpu_x86_cpuid() if enable_pmu is not set. Isn't it valid to set > PDCM=1 without CPUID[0xA]? It's invalid to set PDCM=1 w/o CPUID[0xA] since all features on the PERF_CAPABILITIES are dependent on the enabled guest pmu (which provides basic event counters to make LBR work). Thanks, Like Xu > > >> + env->features[FEAT_PERF_CAPABILITIES] |= cpu->lbr_fmt; >> + } >> + >> /* mwait extended info: needed for Core compatibility */ >> /* We always wake on interrupt even if host does not have the capability */ >> cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE; >> @@ -7157,6 +7172,7 @@ static Property x86_cpu_properties[] = { >> #endif >> DEFINE_PROP_INT32("node-id", X86CPU, node_id, CPU_UNSET_NUMA_NODE_ID), >> DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false), >> + DEFINE_PROP_UINT8("lbr-fmt", X86CPU, lbr_fmt, 0), >> >> DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts, >> HYPERV_SPINLOCK_NEVER_NOTIFY), >> diff --git a/target/i386/cpu.h b/target/i386/cpu.h >> index f519d2bfd4..c1cf8b7160 100644 >> --- a/target/i386/cpu.h >> +++ b/target/i386/cpu.h >> @@ -357,6 +357,7 @@ typedef enum X86Seg { >> #define ARCH_CAP_TSX_CTRL_MSR (1<<7) >> >> #define MSR_IA32_PERF_CAPABILITIES 0x345 >> +#define PERF_CAP_LBR_FMT 0x3f >> >> #define MSR_IA32_TSX_CTRL 0x122 >> #define MSR_IA32_TSCDEADLINE 0x6e0 >> @@ -1701,6 +1702,15 @@ struct X86CPU { >> */ >> bool enable_pmu; >> >> + /* >> + * Configure LBR_FMT bits on IA32_PERF_CAPABILITIES MSR. >> + * This can't be enabled by default yet because it doesn't have >> + * ABI stability guarantees, as it is only allowed to pass all >> + * LBR_FMT bits returned by kvm_arch_get_supported_msr_feature() >> + * (that depends on host CPU and kernel capabilities) to the guest. >> + */ >> + uint8_t lbr_fmt; >> + >> /* LMCE support can be enabled/disabled via cpu option 'lmce=on/off'. It is >> * disabled by default to avoid breaking migration between QEMU with >> * different LMCE configurations. >> -- >> 2.21.3 >> >
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 3ffd877dd5..b10344be01 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6461,6 +6461,13 @@ static void x86_cpu_filter_features(X86CPU *cpu, bool verbose) x86_cpu_get_supported_feature_word(w, false); uint64_t requested_features = env->features[w]; uint64_t unavailable_features = requested_features & ~host_feat; + if (w == FEAT_PERF_CAPABILITIES && + (requested_features & PERF_CAP_LBR_FMT)) { + if ((host_feat & PERF_CAP_LBR_FMT) != + (requested_features & PERF_CAP_LBR_FMT)) { + unavailable_features |= PERF_CAP_LBR_FMT; + } + } mark_unavailable_features(cpu, w, unavailable_features, prefix); } @@ -6533,6 +6540,14 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) } } + if (cpu->lbr_fmt) { + if (!cpu->enable_pmu) { + error_setg(errp, "LBR is unsupported since guest PMU is disabled."); + return; + } + env->features[FEAT_PERF_CAPABILITIES] |= cpu->lbr_fmt; + } + /* mwait extended info: needed for Core compatibility */ /* We always wake on interrupt even if host does not have the capability */ cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE; @@ -7157,6 +7172,7 @@ static Property x86_cpu_properties[] = { #endif DEFINE_PROP_INT32("node-id", X86CPU, node_id, CPU_UNSET_NUMA_NODE_ID), DEFINE_PROP_BOOL("pmu", X86CPU, enable_pmu, false), + DEFINE_PROP_UINT8("lbr-fmt", X86CPU, lbr_fmt, 0), DEFINE_PROP_UINT32("hv-spinlocks", X86CPU, hyperv_spinlock_attempts, HYPERV_SPINLOCK_NEVER_NOTIFY), diff --git a/target/i386/cpu.h b/target/i386/cpu.h index f519d2bfd4..c1cf8b7160 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -357,6 +357,7 @@ typedef enum X86Seg { #define ARCH_CAP_TSX_CTRL_MSR (1<<7) #define MSR_IA32_PERF_CAPABILITIES 0x345 +#define PERF_CAP_LBR_FMT 0x3f #define MSR_IA32_TSX_CTRL 0x122 #define MSR_IA32_TSCDEADLINE 0x6e0 @@ -1701,6 +1702,15 @@ struct X86CPU { */ bool enable_pmu; + /* + * Configure LBR_FMT bits on IA32_PERF_CAPABILITIES MSR. + * This can't be enabled by default yet because it doesn't have + * ABI stability guarantees, as it is only allowed to pass all + * LBR_FMT bits returned by kvm_arch_get_supported_msr_feature() + * (that depends on host CPU and kernel capabilities) to the guest. + */ + uint8_t lbr_fmt; + /* LMCE support can be enabled/disabled via cpu option 'lmce=on/off'. It is * disabled by default to avoid breaking migration between QEMU with * different LMCE configurations.
The last branch recording (LBR) is a performance monitor unit (PMU) feature on Intel processors that records a running trace of the most recent branches taken by the processor in the LBR stack. The QEMU could configure whether it's enabled or not for each guest via CLI. The LBR feature would be enabled on the guest if: - the KVM is enabled and the PMU is enabled and, - the msr-based-feature IA32_PERF_CAPABILITIES is supporterd on KVM and, - the supported returned value for lbr_fmt from this msr is not zero and, - the requested guest vcpu model does support FEAT_1_ECX.CPUID_EXT_PDCM, - the configured lbr-fmt value is the same as the host lbr_fmt value. Cc: Eduardo Habkost <ehabkost@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Like Xu <like.xu@linux.intel.com> --- target/i386/cpu.c | 16 ++++++++++++++++ target/i386/cpu.h | 10 ++++++++++ 2 files changed, 26 insertions(+)