[PATCH RESEND 1/2] target/i386: add "-cpu, lbr-fmt=*" support to enable guest LBR

[PATCH RESEND 1/2] target/i386: add "-cpu, lbr-fmt=*" support to enable guest LBR

[Like Xu]

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
  or use the QEMU option "-cpu host,migratable=no".

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 ++++++++++
 target/i386/kvm/kvm.c |  5 +++--
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ad99cad0e7..eee6da3ad8 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6627,6 +6627,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 (kvm_enabled() && 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);
     }
 
@@ -6734,6 +6741,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;
@@ -7300,6 +7315,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 570f916878..b12c879fc4 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -354,6 +354,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
@@ -1726,6 +1727,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.
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 7fe9f52710..4d842d32a6 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2732,8 +2732,9 @@ static void kvm_msr_entry_add_perf(X86CPU *cpu, FeatureWordArray f)
                                            MSR_IA32_PERF_CAPABILITIES);
 
     if (kvm_perf_cap) {
-        kvm_msr_entry_add(cpu, MSR_IA32_PERF_CAPABILITIES,
-                        kvm_perf_cap & f[FEAT_PERF_CAPABILITIES]);
+        kvm_perf_cap = cpu->migratable ?
+            (kvm_perf_cap & f[FEAT_PERF_CAPABILITIES]) : kvm_perf_cap;
+        kvm_msr_entry_add(cpu, MSR_IA32_PERF_CAPABILITIES, kvm_perf_cap);
     }
 }
 
-- 
2.30.2

[PATCH RESEND 2/2] target/i386: add kvm_exact_match_flags to FeatureWordInfo

[Like Xu]

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 bits too.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 target/i386/cpu.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index eee6da3ad8..56a486b498 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -708,6 +708,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] = {
@@ -1147,6 +1149,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] = {
@@ -6623,16 +6630,18 @@ 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 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() && 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() && 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);
     }
-- 
2.30.2

Re: [PATCH RESEND 1/2] target/i386: add "-cpu, lbr-fmt=*" support to enable guest LBR

[Eduardo Habkost]

Hi,

Sorry for missing the previous submission of this series, and
thanks for resubmitting.

On Fri, Apr 23, 2021 at 10:20:36AM +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
>   or use the QEMU option "-cpu host,migratable=no".
> 
> 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 ++++++++++
>  target/i386/kvm/kvm.c |  5 +++--
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ad99cad0e7..eee6da3ad8 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6627,6 +6627,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 (kvm_enabled() && w == FEAT_PERF_CAPABILITIES &&

If this block of code should run only once, why is this inside
the loop in the first place?

I suggest following the same pattern used for intel-pt flags and
moving it outside the loop.

> +            (requested_features & PERF_CAP_LBR_FMT)) {

What exactly is supposed to happen if the VCPU is configured with
LBR_FMT=0 and the host has LBR_FMT != 0 ?

If it shouldn't be an error, then the new kvm_exact_match_flags
field added in patch 2/2 becomes hard to reuse, and easy to
misuse (there's no code documentation indicating that a mismatch
is allowed if the requested bits are all zero).  In that case,
maybe patch 2/2 could be dropped by now.

If it should be an error, this patch and 2/2 don't seem correct.
If correcting that, I also suggest reversing the patch order in
the series, so this whole block of code doesn't even need to be
added in the first place.


> +            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);
>      }
>  
> @@ -6734,6 +6741,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;
> @@ -7300,6 +7315,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 570f916878..b12c879fc4 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -354,6 +354,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
> @@ -1726,6 +1727,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.
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 7fe9f52710..4d842d32a6 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2732,8 +2732,9 @@ static void kvm_msr_entry_add_perf(X86CPU *cpu, FeatureWordArray f)
>                                             MSR_IA32_PERF_CAPABILITIES);
>  
>      if (kvm_perf_cap) {
> -        kvm_msr_entry_add(cpu, MSR_IA32_PERF_CAPABILITIES,
> -                        kvm_perf_cap & f[FEAT_PERF_CAPABILITIES]);
> +        kvm_perf_cap = cpu->migratable ?
> +            (kvm_perf_cap & f[FEAT_PERF_CAPABILITIES]) : kvm_perf_cap;
> +        kvm_msr_entry_add(cpu, MSR_IA32_PERF_CAPABILITIES, kvm_perf_cap);
>      }
>  }
>  
> -- 
> 2.30.2
> 
> 

-- 
Eduardo

Re: [PATCH RESEND 1/2] target/i386: add "-cpu, lbr-fmt=*" support to enable guest LBR

[Like Xu]

Hi Eduardo,

On 2021/4/24 5:20, Eduardo Habkost wrote:
> Hi,
> 
> Sorry for missing the previous submission of this series, and
> thanks for resubmitting.

Long time no see and thanks for your comments.

> 
> On Fri, Apr 23, 2021 at 10:20:36AM +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
>>    or use the QEMU option "-cpu host,migratable=no".
>>
>> 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 ++++++++++
>>   target/i386/kvm/kvm.c |  5 +++--
>>   3 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index ad99cad0e7..eee6da3ad8 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -6627,6 +6627,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 (kvm_enabled() && w == FEAT_PERF_CAPABILITIES &&
> 
> If this block of code should run only once, why is this inside
> the loop in the first place?
> 
> I suggest following the same pattern used for intel-pt flags and
> moving it outside the loop.

Sure, the mark_unavailable_features() will skip the check for
feature_word(FEAT_PERF_CAPABILITIES) and avoid avoid double checking.

> 
>> +            (requested_features & PERF_CAP_LBR_FMT)) {
> 
> What exactly is supposed to happen if the VCPU is configured with
> LBR_FMT=0 and the host has LBR_FMT != 0 ?

If the VCPU is configured with LBR_FMT=0 and the host has LBR_FMT != 0,
the guest LBR will be enabled if "migratable=no" and
will be disabled if "migratable=yes" by default.

Some test cases and expected results can be listed as:

"-cpu host,lbr-fmt=0" --> "Disable guest LBR"
"-cpu host,lbr-fmt=5" --> "Enable guest LBR"
"-cpu host,lbr-fmt=6" --> "Disable guest LBR and show warning"

"-cpu host,migratable=no" --> "Enable guest LBR and show warning"
"-cpu host,migratable=no,lbr-fmt=0" --> "Enable guest LBR and show warning"
"-cpu host,migratable=no,lbr-fmt=5" --> "Enable guest LBR"
"-cpu host,migratable=no,lbr-fmt=6" --> "Disable guest LBR and show warning"

> 
> If it shouldn't be an error, then the new kvm_exact_match_flags
> field added in patch 2/2 becomes hard to reuse, and easy to
> misuse (there's no code documentation indicating that a mismatch
> is allowed if the requested bits are all zero).  In that case,
> maybe patch 2/2 could be dropped by now.
> 

Let us drop the patch 2/2 and please help review the new version:

https://lore.kernel.org/qemu-devel/20210427080948.439432-1-like.xu@linux.intel.com/

> If it should be an error, this patch and 2/2 don't seem correct.
> If correcting that, I also suggest reversing the patch order in
> the series, so this whole block of code doesn't even need to be
> added in the first place.
> 
> 
>> +            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);
>>       }
>>   
>> @@ -6734,6 +6741,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;
>> @@ -7300,6 +7315,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 570f916878..b12c879fc4 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -354,6 +354,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
>> @@ -1726,6 +1727,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.
>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>> index 7fe9f52710..4d842d32a6 100644
>> --- a/target/i386/kvm/kvm.c
>> +++ b/target/i386/kvm/kvm.c
>> @@ -2732,8 +2732,9 @@ static void kvm_msr_entry_add_perf(X86CPU *cpu, FeatureWordArray f)
>>                                              MSR_IA32_PERF_CAPABILITIES);
>>   
>>       if (kvm_perf_cap) {
>> -        kvm_msr_entry_add(cpu, MSR_IA32_PERF_CAPABILITIES,
>> -                        kvm_perf_cap & f[FEAT_PERF_CAPABILITIES]);
>> +        kvm_perf_cap = cpu->migratable ?
>> +            (kvm_perf_cap & f[FEAT_PERF_CAPABILITIES]) : kvm_perf_cap;
>> +        kvm_msr_entry_add(cpu, MSR_IA32_PERF_CAPABILITIES, kvm_perf_cap);
>>       }
>>   }
>>   
>> -- 
>> 2.30.2
>>
>>
>