Re: [PATCH v7 10/12] KVM/x86/lbr: lazy save the guest lbr stack

2019-07-09 Thread Peter Zijlstra 
On Mon, Jul 08, 2019 at 08:11:41AM -0700, Andi Kleen wrote:
> > I don't understand a word of that.
> > 
> > Who cares if the LBR MSRs are touched; the guest expects up-to-date
> > values when it does reads them.
> 
> This is for only when the LBRs are disabled in the guest.

But your Changelog didn't clearly state that. It was an unreadable mess.

Re: [PATCH v7 10/12] KVM/x86/lbr: lazy save the guest lbr stack

2019-07-08 Thread Wei Wang 

On 07/08/2019 10:53 PM, Peter Zijlstra wrote:

On Mon, Jul 08, 2019 at 09:23:17AM +0800, Wei Wang wrote:

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;
- pass the stack through to the guest.

I don't understand a word of that.

Who cares if the LBR MSRs are touched; the guest expects up-to-date
values when it does reads them.


Another host thread who shares the same pCPU with this vCPU thread
may use the lbr stack, so the host needs to save/restore the vCPU's lbr 
state.

Otherwise the guest perf inside the vCPU wouldn't read the correct data
from the lbr msr (as the msrs are changed by another host thread already).

As Andi also replied, if the vCPU isn't using lbr anymore, host doesn't need
to save the lbr msr then.

Best,
Wei

Re: [PATCH v7 10/12] KVM/x86/lbr: lazy save the guest lbr stack

2019-07-08 Thread Andi Kleen 
> I don't understand a word of that.
> 
> Who cares if the LBR MSRs are touched; the guest expects up-to-date
> values when it does reads them.

This is for only when the LBRs are disabled in the guest.

It doesn't make sense to constantly save/restore disabled LBRs, which
would be a large overhead for guests that don't use LBRs. 

When the LBRs are enabled they always need to be saved/restored as you
say.

-Andi

Re: [PATCH v7 10/12] KVM/x86/lbr: lazy save the guest lbr stack

2019-07-08 Thread Peter Zijlstra 
On Mon, Jul 08, 2019 at 09:23:17AM +0800, Wei Wang wrote:
> 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;
> - pass the stack through to the guest.

I don't understand a word of that.

Who cares if the LBR MSRs are touched; the guest expects up-to-date
values when it does reads them.

[PATCH v7 10/12] KVM/x86/lbr: lazy save the guest lbr stack

2019-07-07 Thread 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.

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;
- pass the stack through to the guest.

Suggested-by: Andi Kleen 
Signed-off-by: Wei Wang 
Cc: Paolo Bonzini 
Cc: Andi Kleen 
Cc: Peter Zijlstra 
---
 arch/x86/include/asm/kvm_host.h |   2 +
 arch/x86/kvm/pmu.c  |   6 ++
 arch/x86/kvm/pmu.h  |   2 +
 arch/x86/kvm/vmx/pmu_intel.c| 141 
 arch/x86/kvm/vmx/vmx.c  |   4 +-
 arch/x86/kvm/vmx/vmx.h  |   2 +
 arch/x86/kvm/x86.c  |   2 +
 7 files changed, 157 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 79e9c92..cf8996e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -469,6 +469,8 @@ struct kvm_pmu {
u64 global_ctrl_mask;
u64 global_ovf_ctrl_mask;
u64 reserved_bits;
+   /* Indicate if the lbr msrs were accessed in this vCPU time slice */
+   bool lbr_used;
u8 version;
struct kvm_pmc gp_counters[INTEL_PMC_MAX_GENERIC];
struct kvm_pmc fixed_counters[INTEL_PMC_MAX_FIXED];
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index ee6ed47..323bb45 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -325,6 +325,12 @@ int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data 
*msr_info)
return kvm_x86_ops->pmu_ops->set_msr(vcpu, msr_info);
 }
 
+void kvm_pmu_sched_in(struct kvm_vcpu *vcpu, int cpu)
+{
+   if (kvm_x86_ops->pmu_ops->sched_in)
+   kvm_x86_ops->pmu_ops->sched_in(vcpu, cpu);
+}
+
 /* refresh PMU settings. This function generally is called when underlying
  * settings are changed (such as changes of PMU CPUID by guest VMs), which
  * should rarely happen.
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
index 384a0b7..cadf91a 100644
--- a/arch/x86/kvm/pmu.h
+++ b/arch/x86/kvm/pmu.h
@@ -32,6 +32,7 @@ struct kvm_pmu_ops {
bool (*lbr_enable)(struct kvm_vcpu *vcpu);
int (*get_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
int (*set_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
+   void (*sched_in)(struct kvm_vcpu *vcpu, int cpu);
void (*refresh)(struct kvm_vcpu *vcpu);
void (*init)(struct kvm_vcpu *vcpu);
void (*reset)(struct kvm_vcpu *vcpu);
@@ -116,6 +117,7 @@ int kvm_pmu_is_valid_msr_idx(struct kvm_vcpu *vcpu, 
unsigned idx);
 bool kvm_pmu_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr);
 int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
 int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
+void kvm_pmu_sched_in(struct kvm_vcpu *vcpu, int cpu);
 void kvm_pmu_refresh(struct kvm_vcpu *vcpu);
 void kvm_pmu_reset(struct kvm_vcpu *vcpu);
 void kvm_pmu_init(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 24a544e..fd09777 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -13,10 +13,12 @@
 #include 
 #include 
 #include 
+#include 
 #include "x86.h"
 #include "cpuid.h"
 #include "lapic.h"
 #include "pmu.h"
+#include "vmx.h"
 
 static struct kvm_event_hw_type_mapping intel_arch_events[] = {
/* Index must match CPUID 0x0A.EBX bit vector */
@@ -141,6 +143,18 @@ static struct kvm_pmc *intel_msr_idx_to_pmc(struct 
kvm_vcpu *vcpu,
return [idx];
 }
 
+static inline bool is_lbr_msr(struct kvm_vcpu *vcpu, u32 index)
+{
+   struct x86_perf_lbr_stack *stack = >kvm->arch.lbr_stack;
+   int nr = stack->nr;
+
+   return !!(index == MSR_LBR_SELECT ||
+ index == stack->tos ||
+ (index >= stack->from && index < stack->from + nr) ||
+ (index >= stack->to && index < stack->to + nr) ||
+ (index >= stack->info && index < stack->info));
+}
+
 static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr)
 {
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
@@ -152,9 +166,12 @@ static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 
msr)
case MSR_CORE_PERF_GLOBAL_CTRL:
case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
case MSR_IA32_PERF_CAPABILITIES:
+   case MSR_IA32_DEBUGCTLMSR:
ret = pmu->version > 1;
break;
default:
+   if (is_lbr_msr(vcpu, msr))
+   return pmu->version > 1;
ret = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) ||
get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0) ||