Re: [PATCH v2 1/4] KVM: vmx/pmu: Add MSR_ARCH_LBR_DEPTH emulation for Arch LBR

2021-03-01 Thread Like Xu 

On 2021/3/2 6:34, Sean Christopherson wrote:

On Wed, Feb 03, 2021, Like Xu wrote:

@@ -348,10 +352,26 @@ static bool intel_pmu_handle_lbr_msrs_access(struct 
kvm_vcpu *vcpu,
return true;
  }
  
+/*

+ * Check if the requested depth values is supported
+ * based on the bits [0:7] of the guest cpuid.1c.eax.
+ */
+static bool arch_lbr_depth_is_valid(struct kvm_vcpu *vcpu, u64 depth)
+{
+   struct kvm_cpuid_entry2 *best;
+
+   best = kvm_find_cpuid_entry(vcpu, 0x1c, 0);
+   if (depth && best)



+   return (best->eax & 0xff) & (1ULL << (depth / 8 - 1));


I believe this will genereate undefined behavior if depth > 64.  Or if depth < 
8.
And I believe this check also needs to enforce that depth is a multiple of 8.

For each bit n set in this field, the IA32_LBR_DEPTH.DEPTH value 8*(n+1) is
supported.

Thus it's impossible for 0-7, 9-15, etc... to be legal depths.


Thank you! How about:

best = kvm_find_cpuid_entry(vcpu, 0x1c, 0);
if (best && depth && !(depth % 8))
return (best->eax & 0xff) & (1ULL << (depth / 8 - 1));

return false;





+
+   return false;
+}
+

Re: [PATCH v2 1/4] KVM: vmx/pmu: Add MSR_ARCH_LBR_DEPTH emulation for Arch LBR

2021-03-01 Thread Sean Christopherson 
On Wed, Feb 03, 2021, Like Xu wrote:
> @@ -348,10 +352,26 @@ static bool intel_pmu_handle_lbr_msrs_access(struct 
> kvm_vcpu *vcpu,
>   return true;
>  }
>  
> +/*
> + * Check if the requested depth values is supported
> + * based on the bits [0:7] of the guest cpuid.1c.eax.
> + */
> +static bool arch_lbr_depth_is_valid(struct kvm_vcpu *vcpu, u64 depth)
> +{
> + struct kvm_cpuid_entry2 *best;
> +
> + best = kvm_find_cpuid_entry(vcpu, 0x1c, 0);
> + if (depth && best)

> + return (best->eax & 0xff) & (1ULL << (depth / 8 - 1));

I believe this will genereate undefined behavior if depth > 64.  Or if depth < 
8.
And I believe this check also needs to enforce that depth is a multiple of 8.

   For each bit n set in this field, the IA32_LBR_DEPTH.DEPTH value 8*(n+1) is
   supported.

Thus it's impossible for 0-7, 9-15, etc... to be legal depths.


> +
> + return false;
> +}
> +

[PATCH v2 1/4] KVM: vmx/pmu: Add MSR_ARCH_LBR_DEPTH emulation for Arch LBR

2021-02-03 Thread Like Xu 
The number of Arch LBR entries available for recording operations
is dictated by the value in MSR_ARCH_LBR_DEPTH.DEPTH. The supported
LBR depth values can be found in CPUID.(EAX=01CH, ECX=0):EAX[7:0]
and for each bit n set in this field, the MSR_ARCH_LBR_DEPTH.DEPTH
value 8*(n+1) is supported.

On a software write to MSR_ARCH_LBR_DEPTH, all LBR entries are reset
to 0. Emulate the reset behavior by introducing lbr_desc->arch_lbr_reset
and sync it to the host MSR_ARCH_LBR_DEPTH msr when the guest LBR
event is ACTIVE and the LBR records msrs are pass-through to the guest.

Signed-off-by: Like Xu 
---
 arch/x86/kvm/vmx/pmu_intel.c | 43 
 arch/x86/kvm/vmx/vmx.h   |  3 +++
 2 files changed, 46 insertions(+)

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index d1df618cb7de..b550c4a6ce33 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -220,6 +220,9 @@ static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 
msr)
case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
ret = pmu->version > 1;
break;
+   case MSR_ARCH_LBR_DEPTH:
+   ret = guest_cpuid_has(vcpu, X86_FEATURE_ARCH_LBR);
+   break;
default:
ret = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) ||
get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0) ||
@@ -250,6 +253,7 @@ static inline void intel_pmu_release_guest_lbr_event(struct 
kvm_vcpu *vcpu)
if (lbr_desc->event) {
perf_event_release_kernel(lbr_desc->event);
lbr_desc->event = NULL;
+   lbr_desc->arch_lbr_reset = false;
vcpu_to_pmu(vcpu)->event_count--;
}
 }
@@ -348,10 +352,26 @@ static bool intel_pmu_handle_lbr_msrs_access(struct 
kvm_vcpu *vcpu,
return true;
 }
 
+/*
+ * Check if the requested depth values is supported
+ * based on the bits [0:7] of the guest cpuid.1c.eax.
+ */
+static bool arch_lbr_depth_is_valid(struct kvm_vcpu *vcpu, u64 depth)
+{
+   struct kvm_cpuid_entry2 *best;
+
+   best = kvm_find_cpuid_entry(vcpu, 0x1c, 0);
+   if (depth && best)
+   return (best->eax & 0xff) & (1ULL << (depth / 8 - 1));
+
+   return false;
+}
+
 static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
struct kvm_pmc *pmc;
+   struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
u32 msr = msr_info->index;
 
switch (msr) {
@@ -367,6 +387,9 @@ static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
case MSR_CORE_PERF_GLOBAL_OVF_CTRL:
msr_info->data = pmu->global_ovf_ctrl;
return 0;
+   case MSR_ARCH_LBR_DEPTH:
+   msr_info->data = lbr_desc->records.nr;
+   return 0;
default:
if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
(pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) {
@@ -393,6 +416,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
 {
struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
struct kvm_pmc *pmc;
+   struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
u32 msr = msr_info->index;
u64 data = msr_info->data;
 
@@ -427,6 +451,13 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
return 0;
}
break;
+   case MSR_ARCH_LBR_DEPTH:
+   if (!arch_lbr_depth_is_valid(vcpu, data))
+   return 1;
+   lbr_desc->records.nr = data;
+   lbr_desc->arch_lbr_reset = true;
+   __set_bit(INTEL_PMC_IDX_FIXED_VLBR, pmu->pmc_in_use);
+   return 0;
default:
if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
(pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) {
@@ -566,6 +597,7 @@ static void intel_pmu_init(struct kvm_vcpu *vcpu)
lbr_desc->records.nr = 0;
lbr_desc->event = NULL;
lbr_desc->msr_passthrough = false;
+   lbr_desc->arch_lbr_reset = false;
 }
 
 static void intel_pmu_reset(struct kvm_vcpu *vcpu)
@@ -623,6 +655,14 @@ static void intel_pmu_deliver_pmi(struct kvm_vcpu *vcpu)
intel_pmu_legacy_freezing_lbrs_on_pmi(vcpu);
 }
 
+static void intel_pmu_arch_lbr_reset(struct kvm_vcpu *vcpu)
+{
+   struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
+
+   wrmsrl(MSR_ARCH_LBR_DEPTH, lbr_desc->records.nr);
+   lbr_desc->arch_lbr_reset = false;
+}
+
 static void vmx_update_intercept_for_lbr_msrs(struct kvm_vcpu *vcpu, bool set)
 {
struct x86_pmu_lbr *lbr = vcpu_to_lbr_records(vcpu);
@@ -654,6 +694,9 @@ static inline void vmx_enable_lbr_msrs_passthrough(struct 
kvm_vcpu *vcpu)
 {
struct lbr_desc *lbr_desc = vcpu_to_lbr_desc(vcpu);
 
+   if