(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 <ehabk...@redhat.com>
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Signed-off-by: Like Xu <like...@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 <ehabk...@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
>
--
Eduardo
