Guest LBR Enabling

arch/x86/kvm/pmu_intel.c +2 -0

+	case INTEL_FAM6_CORE2_MEROM:
+	case INTEL_FAM6_CORE2_MEROM_L:

arch/x86/kvm/pmu_intel.c static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr) · line 187

Quoted 11 lines · Wei Wang

arch/x86/kvm/pmu_intel.c +6 -0

+
+    /*
+     * It could be possible that people have vcpus of old model run on
+     * physcal cpus of newer model, for example a BDW guest on a SKX
+     * machine (but not possible to be the other way around).
+     * The BDW guest may not get accurate results on a SKX machine 

as it
+ * only reads 16 entries of the lbr stack while there are 32
entries

arch/x86/kvm/pmu_intel.c +2 -0

+     * of recordings. So we currently forbid the lbr enabling when the
+     * vcpu and physical cpu see different lbr stack entries.

I think it's not enough to only check number of entries. The LBR from/to MSRs may be different even the number of entries is the same, e.g SLM and KNL.

arch/x86/kvm/pmu_intel.c static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr) · line 189

Quoted 3 lines

arch/x86/kvm/pmu_intel.c +2 -0

+     */
+    switch (vcpu_model) {

That's a duplicate of intel_pmu_init(). I think it's better to factor out the common part if you want to check LBR MSRs and entries. Then we don't need to add the same codes in two different places when enabling new platforms.

Actually, I think we may just support LBR for guest if it has the identical CPU model as host. It should be good enough for now.

Quoted 15 lines · Kan

Quoted 11 lines · Wei Wang

+
+    /*
+     * It could be possible that people have vcpus of old model run on +     * physcal cpus of newer model, for example a BDW guest on a SKX +     * machine (but not possible to be the other way around). +     * The BDW guest may not get accurate results on a SKX machine as it
+     * only reads 16 entries of the lbr stack while there are 32 entries
+     * of recordings. So we currently forbid the lbr enabling when the +     * vcpu and physical cpu see different lbr stack entries.

Quoted 1 line

I think it's not enough to only check number of entries. The LBR from/to MSRs may be different even the number of entries is the same, e.g SLM and KNL.

Yes, we could add the comparison of the FROM msrs.

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

+     */
+    switch (vcpu_model) {

That's a duplicate of intel_pmu_init(). I think it's better to factor out the common part if you want to check LBR MSRs and entries. Then we don't need to add the same codes in two different places when enabling new platforms.

Yes, I thought about this, but intel_pmu_init() does a lot more things in each "Case xx". Any thought about how to factor them out?

Quoted 3 lines

Actually, I think we may just support LBR for guest if it has the identical CPU model as host. It should be good enough for now.

I actually tried this in the first place but it failed to work with the existing QEMU. For example, when we specify "Broadwell" cpu from qemu, then qemu uses Broadwell core model, but the physical machine I have is Broadwell X. This patch will support this case.

Quoted 30 lines · Wei Wang

Quoted 17 lines · Kan

Quoted 7 lines · Wei Wang

arch/x86/kvm/pmu_intel.c +3 -0

+
+    /*
+     * It could be possible that people have vcpus of old model 

run on

arch/x86/kvm/pmu_intel.c +3 -0

+     * physcal cpus of newer model, for example a BDW guest on a SKX
+     * machine (but not possible to be the other way around).
+     * The BDW guest may not get accurate results on a SKX machine 

Quoted 6 lines

as it
+ * only reads 16 entries of the lbr stack while there are 32
entries
+ * of recordings. So we currently forbid the lbr enabling when
the
+ * vcpu and physical cpu see different lbr stack entries.

Quoted 1 line

I think it's not enough to only check number of entries. The LBR from/to MSRs may be different even the number of entries is the same, e.g SLM and KNL.

Yes, we could add the comparison of the FROM msrs.

Quoted 6 lines

arch/x86/kvm/pmu_intel.c static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr) · line 189

Quoted 3 lines

arch/x86/kvm/pmu_intel.c +2 -0

+     */
+    switch (vcpu_model) {

That's a duplicate of intel_pmu_init(). I think it's better to factor out the common part if you want to check LBR MSRs and entries. Then we don't need to add the same codes in two different places when enabling new platforms.

Quoted 2 lines

Yes, I thought about this, but intel_pmu_init() does a lot more things in each "Case xx". Any thought about how to factor them out?

I think we may only move the "switch (boot_cpu_data.x86_model) { ... }" to a new function, e.g. __intel_pmu_init(int model, struct x86_pmu *x86_pmu)

In __intel_pmu_init, if the model != boot_cpu_data.x86_model, you only need to update x86_pmu.*. Just ignore global settings, e.g hw_cache_event_ids, mem_attr, extra_attr etc.

Quoted 8 lines

Quoted 4 lines

Actually, I think we may just support LBR for guest if it has the identical CPU model as host. It should be good enough for now.

I actually tried this in the first place but it failed to work with the existing QEMU. For example, when we specify "Broadwell" cpu from qemu, then qemu uses Broadwell core model, but the physical machine I have is Broadwell X. This patch will support this case.

I mean is it good enough if we only support "-cpu host"?

Quoted 33 lines · Kan

Quoted 30 lines · Wei Wang

Quoted 17 lines · Kan

Quoted 5 lines · Wei Wang

+
+    /*
+     * It could be possible that people have vcpus of old model run on
+     * physcal cpus of newer model, for example a BDW guest on a SKX

Quoted 8 lines

+     * machine (but not possible to be the other way around). +     * The BDW guest may not get accurate results on a SKX machine as it
+     * only reads 16 entries of the lbr stack while there are 32 entries
+     * of recordings. So we currently forbid the lbr enabling when the
+     * vcpu and physical cpu see different lbr stack entries.

Quoted 1 line

I think it's not enough to only check number of entries. The LBR from/to MSRs may be different even the number of entries is the same, e.g SLM and KNL.

Yes, we could add the comparison of the FROM msrs.

Quoted 6 lines

Quoted 3 lines

+     */
+    switch (vcpu_model) {

That's a duplicate of intel_pmu_init(). I think it's better to factor out the common part if you want to check LBR MSRs and entries. Then we don't need to add the same codes in two different places when enabling new platforms.

Quoted 6 lines

Quoted 2 lines

Yes, I thought about this, but intel_pmu_init() does a lot more things in each "Case xx". Any thought about how to factor them out?

I think we may only move the "switch (boot_cpu_data.x86_model) { ... }" to a new function, e.g. __intel_pmu_init(int model, struct x86_pmu *x86_pmu)

In __intel_pmu_init, if the model != boot_cpu_data.x86_model, you only need to update x86_pmu.*. Just ignore global settings, e.g hw_cache_event_ids, mem_attr, extra_attr etc.

Thanks for sharing. I understand the point of maintaining those models at one place, but this factor-out doesn't seem very elegant to me, like below

__intel_pmu_init (int model, struct x86_pmu *x86_pmu) {
...
switch (model)
case INTEL_FAM6_NEHALEM:
case INTEL_FAM6_NEHALEM_EP:
case INTEL_FAM6_NEHALEM_EX:
    intel_pmu_lbr_init(x86_pmu);
    if (model != boot_cpu_data.x86_model) return;

    /* Other a lot of things init like below..*/ memcpy(hw_cache_event_ids, nehalem_hw_cache_event_ids, sizeof(hw_cache_event_ids)); memcpy(hw_cache_extra_regs, nehalem_hw_cache_extra_regs, sizeof(hw_cache_extra_regs)); x86_pmu.event_constraints = intel_nehalem_event_constraints; x86_pmu.pebs_constraints =
intel_nehalem_pebs_event_constraints;
                x86_pmu.enable_all = intel_pmu_nhm_enable_all; x86_pmu.extra_regs = intel_nehalem_extra_regs;
 ...

Case...
}
We need insert "if (model != boot_cpu_data.x86_model)" in every "Case xx".

What would be the rationale that we only do lbr_init for "x86_pmu" when model != boot_cpu_data.x86_model?
(It looks more like a workaround to factor-out the function and get what we want)

I would prefer having them separated as this patch for now - it is logically more clear to me.

Quoted 27 lines

Thanks for sharing. I understand the point of maintaining those models at one place, but this factor-out doesn't seem very elegant to me, like below

__intel_pmu_init (int model, struct x86_pmu *x86_pmu) {
...
switch (model)
case INTEL_FAM6_NEHALEM:
case INTEL_FAM6_NEHALEM_EP:
case INTEL_FAM6_NEHALEM_EX:

     intel_pmu_lbr_init(x86_pmu);
     if (model != boot_cpu_data.x86_model)
         return;

     /* Other a lot of things init like below..*/
     memcpy(hw_cache_event_ids, nehalem_hw_cache_event_ids,
                    sizeof(hw_cache_event_ids));
     memcpy(hw_cache_extra_regs, nehalem_hw_cache_extra_regs,
                    sizeof(hw_cache_extra_regs));
     x86_pmu.event_constraints = intel_nehalem_event_constraints;
                 x86_pmu.pebs_constraints = 
intel_nehalem_pebs_event_constraints;
                 x86_pmu.enable_all = intel_pmu_nhm_enable_all;
                 x86_pmu.extra_regs = intel_nehalem_extra_regs;

...

Quoted 7 lines

Case...
}
We need insert "if (model != boot_cpu_data.x86_model)" in every "Case xx".

What would be the rationale that we only do lbr_init for "x86_pmu" when model != boot_cpu_data.x86_model?
(It looks more like a workaround to factor-out the function and get what we want)

I thought the new function may be extended to support fake pmu as below.
It's not only for lbr. PMU has many CPU specific features. It can be used for other features, if you want to check the compatibility in future. But I don't have an example now.

__intel_pmu_init (int model, struct x86_pmu *x86_pmu)
{
bool fake_pmu = (model != boot_cpu_data.x86_model) ? true : false;
...
switch (model)
case INTEL_FAM6_NEHALEM:
case INTEL_FAM6_NEHALEM_EP:
case INTEL_FAM6_NEHALEM_EX:

     intel_pmu_lbr_init(x86_pmu);
     x86_pmu->event_constraints = intel_nehalem_event_constraints;
     x86_pmu->pebs_constraints = intel_nehalem_pebs_event_constraints;
     x86_pmu->enable_all = intel_pmu_nhm_enable_all;
     x86_pmu->extra_regs = intel_nehalem_extra_regs;

     if (fake_pmu)
         return;

     /* Global variables should not be updated for fake PMU */
     memcpy(hw_cache_event_ids, nehalem_hw_cache_event_ids,
                    sizeof(hw_cache_event_ids));
     memcpy(hw_cache_extra_regs, nehalem_hw_cache_extra_regs,
                    sizeof(hw_cache_extra_regs));

Quoted 1 line

Quoted 2 lines

I would prefer having them separated as this patch for now - it is logically more clear to me.

But it will be a problem for maintenance. Perf developer probably forget to update the list in KVM. I think you have to regularly check the perf code.

Quoted 51 lines · Kan

Quoted 26 lines

Thanks for sharing. I understand the point of maintaining those models at one place, but this factor-out doesn't seem very elegant to me, like below

__intel_pmu_init (int model, struct x86_pmu *x86_pmu) {
...
switch (model)
case INTEL_FAM6_NEHALEM:
case INTEL_FAM6_NEHALEM_EP:
case INTEL_FAM6_NEHALEM_EX:
     intel_pmu_lbr_init(x86_pmu);
     if (model != boot_cpu_data.x86_model) return;

     /* Other a lot of things init like below..*/ memcpy(hw_cache_event_ids, nehalem_hw_cache_event_ids, sizeof(hw_cache_event_ids)); memcpy(hw_cache_extra_regs, nehalem_hw_cache_extra_regs, sizeof(hw_cache_extra_regs)); x86_pmu.event_constraints = intel_nehalem_event_constraints; x86_pmu.pebs_constraints = intel_nehalem_pebs_event_constraints; x86_pmu.enable_all = intel_pmu_nhm_enable_all; x86_pmu.extra_regs = intel_nehalem_extra_regs;
  ...

Quoted 7 lines

Case...
}
We need insert "if (model != boot_cpu_data.x86_model)" in every "Case xx".

What would be the rationale that we only do lbr_init for "x86_pmu" when model != boot_cpu_data.x86_model?
(It looks more like a workaround to factor-out the function and get what we want)

I thought the new function may be extended to support fake pmu as below.
It's not only for lbr. PMU has many CPU specific features. It can be used for other features, if you want to check the compatibility in future. But I don't have an example now.

__intel_pmu_init (int model, struct x86_pmu *x86_pmu)
{
bool fake_pmu = (model != boot_cpu_data.x86_model) ? true : false;
...
switch (model)
case INTEL_FAM6_NEHALEM:
case INTEL_FAM6_NEHALEM_EP:
case INTEL_FAM6_NEHALEM_EX:
     intel_pmu_lbr_init(x86_pmu); x86_pmu->event_constraints = intel_nehalem_event_constraints; x86_pmu->pebs_constraints = intel_nehalem_pebs_event_constraints; x86_pmu->enable_all = intel_pmu_nhm_enable_all; x86_pmu->extra_regs = intel_nehalem_extra_regs;

Quoted 2 lines

     if (fake_pmu)
         return;

It looks similar as the one I shared above, the difference is that more things (e.g. constraints) are assigned to x86_fake_pmu.
I'm not sure about the logic behind it (still look like a workaround).

Quoted 12 lines

     /* Global variables should not be updated for fake PMU */ memcpy(hw_cache_event_ids, nehalem_hw_cache_event_ids, sizeof(hw_cache_event_ids)); memcpy(hw_cache_extra_regs, nehalem_hw_cache_extra_regs, sizeof(hw_cache_extra_regs));

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

I would prefer having them separated as this patch for now - it is logically more clear to me.

But it will be a problem for maintenance. Perf developer probably forget to update the list in KVM. I think you have to regularly check the perf code.

It's been very common in hypervisor development. That's why we have hypervisor developers here. When a new platform is added, we will definitely get some work like this to do.

Quoted 53 lines · Wei Wang

Quoted 52 lines · Kan

Quoted 27 lines

Thanks for sharing. I understand the point of maintaining those models at one place, but this factor-out doesn't seem very elegant to me, like below

__intel_pmu_init (int model, struct x86_pmu *x86_pmu) {
...
switch (model)
case INTEL_FAM6_NEHALEM:
case INTEL_FAM6_NEHALEM_EP:
case INTEL_FAM6_NEHALEM_EX:

     intel_pmu_lbr_init(x86_pmu);
     if (model != boot_cpu_data.x86_model)
         return;

     /* Other a lot of things init like below..*/
     memcpy(hw_cache_event_ids, nehalem_hw_cache_event_ids,
                    sizeof(hw_cache_event_ids));
     memcpy(hw_cache_extra_regs, nehalem_hw_cache_extra_regs,
                    sizeof(hw_cache_extra_regs));
     x86_pmu.event_constraints = intel_nehalem_event_constraints;
                 x86_pmu.pebs_constraints = 
intel_nehalem_pebs_event_constraints;
                 x86_pmu.enable_all = intel_pmu_nhm_enable_all;
                 x86_pmu.extra_regs = intel_nehalem_extra_regs;

...

Quoted 7 lines

Case...
}
We need insert "if (model != boot_cpu_data.x86_model)" in every "Case xx".

What would be the rationale that we only do lbr_init for "x86_pmu" when model != boot_cpu_data.x86_model?
(It looks more like a workaround to factor-out the function and get what we want)

I thought the new function may be extended to support fake pmu as below.
It's not only for lbr. PMU has many CPU specific features. It can be used for other features, if you want to check the compatibility in future. But I don't have an example now.

__intel_pmu_init (int model, struct x86_pmu *x86_pmu)
{
bool fake_pmu = (model != boot_cpu_data.x86_model) ? true : false;
...
switch (model)
case INTEL_FAM6_NEHALEM:
case INTEL_FAM6_NEHALEM_EP:
case INTEL_FAM6_NEHALEM_EX:

     intel_pmu_lbr_init(x86_pmu);
     x86_pmu->event_constraints = intel_nehalem_event_constraints;
     x86_pmu->pebs_constraints = intel_nehalem_pebs_event_constraints;
     x86_pmu->enable_all = intel_pmu_nhm_enable_all;
     x86_pmu->extra_regs = intel_nehalem_extra_regs;

Quoted 6 lines

Quoted 2 lines

     if (fake_pmu)
         return;

It looks similar as the one I shared above, the difference is that more things (e.g. constraints) are assigned to x86_fake_pmu.
I'm not sure about the logic behind it (still look like a workaround).

The fake x86_pmu will include all the supported features in host. If you want to check other features in future, it would be useful.

The lbr stack is architecturally specific, for example, SKX has 32 lbr stack entries while HSW has 16 entries, so a HSW guest running on a SKX machine may not get accurate perf results. Currently, we forbid the guest lbr enabling when the guest and host see different lbr stack entries.

Quoted 5 lines · Wei Wang

The lbr stack is architecturally specific, for example, SKX has 32 lbr stack entries while HSW has 16 entries, so a HSW guest running on a SKX machine may not get accurate perf results. Currently, we forbid the guest lbr enabling when the guest and host see different lbr stack entries.

How do you handle live migration?

Quoted 6 lines · Jim Mattson

Quoted 5 lines · Wei Wang

The lbr stack is architecturally specific, for example, SKX has 32 lbr stack entries while HSW has 16 entries, so a HSW guest running on a SKX machine may not get accurate perf results. Currently, we forbid the guest lbr enabling when the guest and host see different lbr stack entries.

Quoted 1 line

How do you handle live migration?

This feature is gated by the QEMU "lbr=true" option. So if the lbr fails to work on the destination machine, the destination side QEMU wouldn't be able to boot, and migration will not happen.

Yes, but then what happens?

Fast forward to, say, 2021. You're decommissioning all Broadwell servers in your data center. You have to migrate the running VMs off of those Broadwell systems onto newer hardware. But, with the current implementation, the migration cannot happen. So, what do you do? I suppose you just never enable the feature in the first place. Right?

Quoted 7 lines · Wei Wang

Quoted 6 lines · Jim Mattson

Quoted 5 lines · Wei Wang

The lbr stack is architecturally specific, for example, SKX has 32 lbr stack entries while HSW has 16 entries, so a HSW guest running on a SKX machine may not get accurate perf results. Currently, we forbid the guest lbr enabling when the guest and host see different lbr stack entries.

Quoted 5 lines

Quoted 1 line

How do you handle live migration?

This feature is gated by the QEMU "lbr=true" option. So if the lbr fails to work on the destination machine, the destination side QEMU wouldn't be able to boot, and migration will not happen.

Yes, but then what happens?

Fast forward to, say, 2021. You're decommissioning all Broadwell servers in your data center. You have to migrate the running VMs off of those Broadwell systems onto newer hardware. But, with the current implementation, the migration cannot happen. So, what do you do? I suppose you just never enable the feature in the first place. Right?

Quoted 5 lines · Jim Mattson

Fast forward to, say, 2021. You're decommissioning all Broadwell servers in your data center. You have to migrate the running VMs off of those Broadwell systems onto newer hardware. But, with the current implementation, the migration cannot happen. So, what do you do? I suppose you just never enable the feature in the first place. Right?

I'm not sure if that's the way people would do with their data centers. What would be the point of decommissioning all the BDW machines when there are important BDW VMs running?

Quoted 11 lines · Wei Wang

Quoted 5 lines · Jim Mattson

Fast forward to, say, 2021. You're decommissioning all Broadwell servers in your data center. You have to migrate the running VMs off of those Broadwell systems onto newer hardware. But, with the current implementation, the migration cannot happen. So, what do you do? I suppose you just never enable the feature in the first place. Right?

I'm not sure if that's the way people would do with their data centers. What would be the point of decommissioning all the BDW machines when there are important BDW VMs running?

TCO increases as hardware ages, while the TCO for each successive generation of hardware tends to be lower than its predecessor. Thus, the point of decommissioning all BDW hosts that are beyond their useful service life is to save money. Our assumption for Google Cloud is that we will always be able to emulate older Intel processors on newer Intel processors, so the running BDW VMs should not be affected by the decommissioning of BDW hardware. Obviously, that means that we won't offer features that don't have a forward migration story, such as this one.

Yes, someday Intel will drop support for some feature that we currently offer (like MPX, perhaps), and that will cause us some grief.

Bits [0, 5] of MSR_IA32_PERF_CAPABILITIES tell about the format of the addresses stored in the LBR stack. Expose those bits to the guest when the guest lbr feature is enabled.

Signed-off-by Wei Wang <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Andi Kleen <[email protected]>

---
 arch/x86/include/asm/perf_event.h | 2 ++
 arch/x86/kvm/cpuid.c              | 2 +-
 arch/x86/kvm/vmx.c                | 9 +++++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

arch/x86/include/asm/perf_event.h +2 -0

index 2f82795..eee09b7 100644
@@ -87,6 +87,8 @@
 #define ARCH_PERFMON_BRANCH_MISSES_RETIRED             6
 #define ARCH_PERFMON_EVENTS_COUNT                      7
 
+#define X86_PERF_CAP_MASK_LBR_FMT                      0x3f
+
 /*
  * Intel "Architectural Performance Monitoring" CPUID
  * detection/enumeration details:

arch/x86/kvm/cpuid.c +1 -1

index 7bcfa61..3b8a57b 100644
@@ -365,7 +365,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
                F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
                0 /* DS-CPL, VMX, SMX, EST */ |
                0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
-               F(FMA) | F(CX16) | 0 /* xTPR Update, PDCM */ |
+               F(FMA) | F(CX16) | 0 /* xTPR Update*/ | F(PDCM) |
                F(PCID) | 0 /* Reserved, DCA */ | F(XMM4_1) |
                F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) |
                0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE */ | F(AVX) |

arch/x86/kvm/vmx.c +0 -0

index 8d5d984..ee02967 100644

arch/x86/kvm/vmx.c +5 -1

+               if (vcpu->kvm->arch.lbr_in_guest)
+                       msr_info->data &= X86_PERF_CAP_MASK_LBR_FMT;
+               break;
        case MSR_TSC_AUX:
                if (!msr_info->host_initiated &&
                    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))

@@ -4343,6 +4350,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                else
                        clear_atomic_switch_msr(vmx, MSR_IA32_XSS);
                break;
+       case MSR_IA32_PERF_CAPABILITIES:
+               return 1; /* RO MSR */
        case MSR_TSC_AUX:
                if (!msr_info->host_initiated &&
                    !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
--

2.7.4

Quoted 45 lines · Wei Wang

Bits [0, 5] of MSR_IA32_PERF_CAPABILITIES tell about the format of the addresses stored in the LBR stack. Expose those bits to the guest when the guest lbr feature is enabled.

Signed-off-by Wei Wang <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Andi Kleen <[email protected]>

---
  arch/x86/include/asm/perf_event.h | 2 ++
  arch/x86/kvm/cpuid.c              | 2 +-
  arch/x86/kvm/vmx.c                | 9 +++++++++
  3 files changed, 12 insertions(+), 1 deletion(-)

arch/x86/include/asm/perf_event.h +2 -0

index 2f82795..eee09b7 100644
@@ -87,6 +87,8 @@
  #define ARCH_PERFMON_BRANCH_MISSES_RETIRED             6
  #define ARCH_PERFMON_EVENTS_COUNT                      7
 
+#define X86_PERF_CAP_MASK_LBR_FMT                      0x3f
+
  /*
   * Intel "Architectural Performance Monitoring" CPUID
   * detection/enumeration details:

arch/x86/kvm/cpuid.c +1 -1

index 7bcfa61..3b8a57b 100644
@@ -365,7 +365,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
                 F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
                 0 /* DS-CPL, VMX, SMX, EST */ |
                 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ |
-               F(FMA) | F(CX16) | 0 /* xTPR Update, PDCM */ |
+               F(FMA) | F(CX16) | 0 /* xTPR Update*/ | F(PDCM) |
                 F(PCID) | 0 /* Reserved, DCA */ | F(XMM4_1) |
                 F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) |
                 0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE */ | F(AVX) |

arch/x86/kvm/vmx.c +0 -0

index 8d5d984..ee02967 100644
@@ -4161,6 +4161,13 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                         return 1;

arch/x86/kvm/vmx.c static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) · line 4167

Quoted 6 lines

arch/x86/kvm/vmx.c +4 -0

                 msr_info->data = vcpu->arch.ia32_xss;
                 break;
+       case MSR_IA32_PERF_CAPABILITIES:
+               if (!boot_cpu_has(X86_FEATURE_PDCM))
+                       return 1;
+               msr_info->data = native_read_msr(MSR_IA32_PERF_CAPABILITIES);

Since this isn't guarded by vcpu->kvm->arch.lbr_in_guest, it breaks backwards compatibility, doesn't it?

Right, thanks. Probably better to change it to below:

msr_info->data = 0;
data = native_read_msr(MSR_IA32_PERF_CAPABILITIES); if (vcpu->kvm->arch.lbr_in_guest)

     msr_info->data |= (data & X86_PERF_CAP_MASK_LBR_FMT);

Quoted 7 lines · Wei Wang

Right, thanks. Probably better to change it to below:

msr_info->data = 0;
data = native_read_msr(MSR_IA32_PERF_CAPABILITIES); if (vcpu->kvm->arch.lbr_in_guest)

      msr_info->data |= (data & X86_PERF_CAP_MASK_LBR_FMT);

This still breaks backwards compatibility. Returning 0 and raising #GP are not the same.

Quoted 11 lines · Jim Mattson

Quoted 7 lines · Wei Wang

Right, thanks. Probably better to change it to below:

msr_info->data = 0;
data = native_read_msr(MSR_IA32_PERF_CAPABILITIES); if (vcpu->kvm->arch.lbr_in_guest)

      msr_info->data |= (data & X86_PERF_CAP_MASK_LBR_FMT);

This still breaks backwards compatibility. Returning 0 and raising #GP are not the same.

I'm not sure about raising GP# in this case.

This PERF_CAP msr contains more things than the lbr format. For example, a guest with lbr=false option could read it to get PEBS_FMT, which is PERF_CAP[11:8]. We should offer those bits in this case.

When lbr=false, the lbr feature is not usable by the guest, so I think whatever value (0 or other value) of the LBR_FMT bits that we give to the guest might not be important.

The issue is compatibility. Prior to your change, reading this MSR from a VM would raise #GP. After your change, it won't. That means that if you have a VM migrating between hosts with kernel versions before and after this change, the results will be inconsistent. In the

Quoted 5 lines

The issue is compatibility. Prior to your change, reading this MSR from a VM would raise #GP. After your change, it won't. That means that if you have a VM migrating between hosts with kernel versions before and after this change, the results will be inconsistent. In the

No it will not be. All Linux kernel uses of this MSR are guarded by a CPUID check.

Quoted 6 lines · Andi Kleen

Quoted 5 lines

The issue is compatibility. Prior to your change, reading this MSR from a VM would raise #GP. After your change, it won't. That means that if you have a VM migrating between hosts with kernel versions before and after this change, the results will be inconsistent. In the

Quoted 2 lines

No it will not be. All Linux kernel uses of this MSR are guarded by a CPUID check.

Linux usage is irrelevant to the architected behavior of the virtual CPU. According to volume 4 of the SDM, this MSR is only supported when CPUID.01H:ECX.PDCM [bit 15] is set. Therefore, kvm should raise #GP whenever a guest tries to read this MSR and the guest's CPUID.01H:ECX.PDCM [bit 15] is clear.

Quoted 10 lines · Jim Mattson

Quoted 8 lines · Andi Kleen

Quoted 5 lines

The issue is compatibility. Prior to your change, reading this MSR from a VM would raise #GP. After your change, it won't. That means that if you have a VM migrating between hosts with kernel versions before and after this change, the results will be inconsistent. In the

No it will not be. All Linux kernel uses of this MSR are guarded by a CPUID check.

Quoted 5 lines

Linux usage is irrelevant to the architected behavior of the virtual CPU. According to volume 4 of the SDM, this MSR is only supported when CPUID.01H:ECX.PDCM [bit 15] is set. Therefore, kvm should raise #GP whenever a guest tries to read this MSR and the guest's CPUID.01H:ECX.PDCM [bit 15] is clear.

That's not general practice in KVM. There are lots of MSRs that don't fault even if their CPUID doesn't support them.

It's also not generally true for instructions, where it is even impossible.

You could argue it should be done for MSRs, but that would be a much larger patch for lots of MSRs. It seems pointless to single out this particular one.

In practice I doubt it will make any difference for real software either way.

Quoted 4 lines · Andi Kleen

Quoted 4 lines

The issue is compatibility. Prior to your change, reading this MSR from a VM would raise #GP. After your change, it won't. That means that if you have a VM migrating between hosts with kernel versions before and after this change, the results will be inconsistent. In the

Quoted 2 lines

No it will not be. All Linux kernel uses of this MSR are guarded by a CPUID check.

Linux usage is irrelevant to the architected behavior of the virtual CPU. According to volume 4 of the SDM, this MSR is only supported when CPUID.01H:ECX.PDCM [bit 15] is set. Therefore, kvm should raise #GP whenever a guest tries to read this MSR and the guest's CPUID.01H:ECX.PDCM [bit 15] is clear.

Quoted 5 lines · Jim Mattson

Quoted 4 lines · Andi Kleen

Quoted 4 lines

The issue is compatibility. Prior to your change, reading this MSR from a VM would raise #GP. After your change, it won't. That means that if you have a VM migrating between hosts with kernel versions before and after this change, the results will be inconsistent. In the

Quoted 8 lines

Quoted 2 lines

No it will not be. All Linux kernel uses of this MSR are guarded by a CPUID check.

Linux usage is irrelevant to the architected behavior of the virtual CPU. According to volume 4 of the SDM, this MSR is only supported when CPUID.01H:ECX.PDCM [bit 15] is set. Therefore, kvm should raise #GP whenever a guest tries to read this MSR and the guest's CPUID.01H:ECX.PDCM [bit 15] is clear.

Probably one more check would be better:

if (!boot_cpu_has(X86_FEATURE_PDCM) ||

     !guest_cpuid_has(vcpu, X86_FEATURE_PDCM))
     return 1;

(host isn't expected to read this MSR when PDCM is not supported by the guest, so don't have "!msr_info->host_initiate" added to above)

When the vCPU is scheduled in:
- if the lbr feature was used in the last vCPU time slice, set the lbr stack to be interceptible, so that the host can capture whether the lbr feature will be used in this time slice;
- if the lbr feature wasn't used in the last vCPU time slice, disable the vCPU support of the guest lbr switching.

Upon the first access to one of the lbr related MSRs (since the vCPU was scheduled in):
- record that the guest has used the lbr;
- create a host perf event to help save/restore the guest lbr stack if the guest uses the user callstack mode lbr stack;

Thanks. This looks a lot better than the earlier versions.

Some more comments.

Quoted 6 lines · Wei Wang

When the vCPU is scheduled in:
- if the lbr feature was used in the last vCPU time slice, set the lbr stack to be interceptible, so that the host can capture whether the lbr feature will be used in this time slice;
- if the lbr feature wasn't used in the last vCPU time slice, disable the vCPU support of the guest lbr switching.

time slice is the time from exit to exit?

This might be rather short in some cases if the workload does a lot of exits (which I would expect PMU workloads to do) Would be better to use some explicit time check, or at least N exits.

Quoted 5 lines

Upon the first access to one of the lbr related MSRs (since the vCPU was scheduled in):
- record that the guest has used the lbr;
- create a host perf event to help save/restore the guest lbr stack if the guest uses the user callstack mode lbr stack;

This is a bit risky. It would be safer (but also more expensive) to always safe even for any guest LBR use independent of callstack.

Otherwise we might get into a situation where
a vCPU context switch inside the guest PMI will clear the LBRs before they can be read in the PMI, so some LBR samples will be fully or partially cleared. This would be user visible.

In theory could try to detect if the guest is inside a PMI and save/restore then, but that would likely be complicated. I would save/restore for all cases.

arch/x86/kvm/vmx.c static bool nested_vmx_is_page_fault_vmexit(struct vmcs12 *vmcs12, · line 1315

Quoted 4 lines

arch/x86/kvm/vmx.c +3 -0

+static void
+__always_inline vmx_set_intercept_for_msr(unsigned long *msr_bitmap, u32 msr,
+					  int type, bool value);

__always_inline should only be used if it's needed for functionality, or in a header.

Quoted 11 lines · Andi Kleen

Thanks. This looks a lot better than the earlier versions.

Some more comments.

Quoted 6 lines · Wei Wang

When the vCPU is scheduled in:
- if the lbr feature was used in the last vCPU time slice, set the lbr stack to be interceptible, so that the host can capture whether the lbr feature will be used in this time slice;
- if the lbr feature wasn't used in the last vCPU time slice, disable the vCPU support of the guest lbr switching.

Quoted 1 line

time slice is the time from exit to exit?

It's the vCPU thread time slice (e.g. 100ms).

Quoted 5 lines

This might be rather short in some cases if the workload does a lot of exits (which I would expect PMU workloads to do) Would be better to use some explicit time check, or at least N exits.

Did you mean further increasing the lazy time to multiple host thread scheduling time slices?
What would be a good value for "N"?

Quoted 5 lines

or partially cleared. This would be user visible.

In theory could try to detect if the guest is inside a PMI and save/restore then, but that would likely be complicated. I would save/restore for all cases.

Yes, it is easier to save for all the cases. But curious for the non-callstack
mode, it's just ponit sampling functions (kind of speculative in some degree).
Would rarely losing a few recordings important in that case?

Actually forgot one case.

In Arch Perfmon v4 the LBR freezing is also controlled through a GLOBAL_CTRL bit.
I didn't see any code handling that bit?