Re: [Qemu-devel PATCH v2] target/i386: add "-cpu,lbr-fmt=*" support to enable guest LBR

[Eduardo Habkost]

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

-- 
Eduardo