diff mbox series

[v6,08/16] KVM: x86/pmu: Add IA32_DS_AREA MSR emulation to support guest DS

Message ID 20210511024214.280733-9-like.xu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series KVM: x86/pmu: Add *basic* support to enable guest PEBS via DS | expand

Commit Message

Like Xu May 11, 2021, 2:42 a.m. UTC 
When CPUID.01H:EDX.DS[21] is set, the IA32_DS_AREA MSR exists and points
to the linear address of the first byte of the DS buffer management area,
which is used to manage the PEBS records.

When guest PEBS is enabled, the MSR_IA32_DS_AREA MSR will be added to the
perf_guest_switch_msr() and switched during the VMX transitions just like
CORE_PERF_GLOBAL_CTRL MSR. The WRMSR to IA32_DS_AREA MSR brings a #GP(0)
if the source register contains a non-canonical address.

Originally-by: Andi Kleen <ak@linux.intel.com>
Co-developed-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 arch/x86/events/intel/core.c    | 11 ++++++++++-
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/vmx/pmu_intel.c    | 11 +++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

Comments

Xu, Like May 12, 2021, 5:16 a.m. UTC | #1 
On 2021/5/11 10:42, Like Xu wrote:
> @@ -3908,7 +3911,7 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
>   		.guest = intel_ctrl & (~cpuc->intel_ctrl_host_mask | ~pebs_mask),
>   	};
>   
> -	if (!x86_pmu.pebs)
> +	if (!pmu || !x86_pmu.pebs_vmx)
>   		return arr;
>   
>   	/*
> @@ -3931,6 +3934,12 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
>   	if (!x86_pmu.pebs_vmx)
>   		return arr;
>   
> +	arr[(*nr)++] = (struct perf_guest_switch_msr){
> +		.msr = MSR_IA32_DS_AREA,
> +		.host = (unsigned long)ds,
> +		.guest = pmu->ds_area,
> +	};
> +
>   	arr[*nr] = (struct perf_guest_switch_msr){
>   		.msr = MSR_IA32_PEBS_ENABLE,
>   		.host = cpuc->pebs_enabled & ~cpuc->intel_ctrl_guest_mask,

Sorry, this part should be:

@@ -3928,9 +3931,15 @@ static struct perf_guest_switch_msr 
*intel_guest_get_msrs(int *nr, void *data)
                 return arr;
         }

-       if (!x86_pmu.pebs_vmx)
+       if (!pmu || !x86_pmu.pebs_vmx)
                 return arr;

+       arr[(*nr)++] = (struct perf_guest_switch_msr){
+               .msr = MSR_IA32_DS_AREA,
+               .host = (unsigned long)ds,
+               .guest = pmu->ds_area,
+       };
+
         arr[*nr] = (struct perf_guest_switch_msr){
                 .msr = MSR_IA32_PEBS_ENABLE,
                 .host = cpuc->pebs_enabled & ~cpuc->intel_ctrl_guest_mask,
Peter Zijlstra May 17, 2021, 1:26 p.m. UTC | #2 
On Tue, May 11, 2021 at 10:42:06AM +0800, Like Xu wrote:
> @@ -3897,6 +3898,8 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
>  {
>  	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>  	struct perf_guest_switch_msr *arr = cpuc->guest_switch_msrs;
> +	struct debug_store *ds = __this_cpu_read(cpu_hw_events.ds);
> +	struct kvm_pmu *pmu = (struct kvm_pmu *)data;

You can do without the cast, this is C, 'void *' silently casts to any
other pointer type.

>  	u64 intel_ctrl = hybrid(cpuc->pmu, intel_ctrl);
>  	u64 pebs_mask = (x86_pmu.flags & PMU_FL_PEBS_ALL) ?
>  		cpuc->pebs_enabled : (cpuc->pebs_enabled & PEBS_COUNTER_MASK);

> @@ -3931,6 +3934,12 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
>  	if (!x86_pmu.pebs_vmx)
>  		return arr;
>  
> +	arr[(*nr)++] = (struct perf_guest_switch_msr){
> +		.msr = MSR_IA32_DS_AREA,
> +		.host = (unsigned long)ds,

Using:
		(unsigned long)cpuc->ds;

was too complicated? :-)

> +		.guest = pmu->ds_area,
> +	};
> +
>  	arr[*nr] = (struct perf_guest_switch_msr){
>  		.msr = MSR_IA32_PEBS_ENABLE,
>  		.host = cpuc->pebs_enabled & ~cpuc->intel_ctrl_guest_mask,
Andi Kleen May 17, 2021, 2:50 p.m. UTC | #3 
On 5/17/2021 6:26 AM, Peter Zijlstra wrote:
> On Tue, May 11, 2021 at 10:42:06AM +0800, Like Xu wrote:
>> @@ -3897,6 +3898,8 @@ static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
>>   {
>>   	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>   	struct perf_guest_switch_msr *arr = cpuc->guest_switch_msrs;
>> +	struct debug_store *ds = __this_cpu_read(cpu_hw_events.ds);
>> +	struct kvm_pmu *pmu = (struct kvm_pmu *)data;
> You can do without the cast, this is C, 'void *' silently casts to any
> other pointer type.

FWIW doing the C++ like casts for void * is fairly standard C coding 
style. I generally prefer it too for better documentation. K&R is 
written this way.

-Andi (my last email on this topic to avoid any bike shedding)
diff mbox series

Patch

diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
index c791765f4761..de3bc8dfe85e 100644
--- a/arch/x86/events/intel/core.c
+++ b/arch/x86/events/intel/core.c
@@ -21,6 +21,7 @@ 
 #include <asm/intel_pt.h>
 #include <asm/apic.h>
 #include <asm/cpu_device_id.h>
+#include <asm/kvm_host.h>
 
 #include "../perf_event.h"
 
@@ -3897,6 +3898,8 @@  static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
 {
 	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
 	struct perf_guest_switch_msr *arr = cpuc->guest_switch_msrs;
+	struct debug_store *ds = __this_cpu_read(cpu_hw_events.ds);
+	struct kvm_pmu *pmu = (struct kvm_pmu *)data;
 	u64 intel_ctrl = hybrid(cpuc->pmu, intel_ctrl);
 	u64 pebs_mask = (x86_pmu.flags & PMU_FL_PEBS_ALL) ?
 		cpuc->pebs_enabled : (cpuc->pebs_enabled & PEBS_COUNTER_MASK);
@@ -3908,7 +3911,7 @@  static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
 		.guest = intel_ctrl & (~cpuc->intel_ctrl_host_mask | ~pebs_mask),
 	};
 
-	if (!x86_pmu.pebs)
+	if (!pmu || !x86_pmu.pebs_vmx)
 		return arr;
 
 	/*
@@ -3931,6 +3934,12 @@  static struct perf_guest_switch_msr *intel_guest_get_msrs(int *nr, void *data)
 	if (!x86_pmu.pebs_vmx)
 		return arr;
 
+	arr[(*nr)++] = (struct perf_guest_switch_msr){
+		.msr = MSR_IA32_DS_AREA,
+		.host = (unsigned long)ds,
+		.guest = pmu->ds_area,
+	};
+
 	arr[*nr] = (struct perf_guest_switch_msr){
 		.msr = MSR_IA32_PEBS_ENABLE,
 		.host = cpuc->pebs_enabled & ~cpuc->intel_ctrl_guest_mask,
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 0a42079560ac..296bc3eecdc6 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -473,6 +473,7 @@  struct kvm_pmu {
 	DECLARE_BITMAP(all_valid_pmc_idx, X86_PMC_IDX_MAX);
 	DECLARE_BITMAP(pmc_in_use, X86_PMC_IDX_MAX);
 
+	u64 ds_area;
 	u64 pebs_enable;
 	u64 pebs_enable_mask;
 
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 9938b485c31c..5584b8dfadb3 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -223,6 +223,9 @@  static bool intel_is_valid_msr(struct kvm_vcpu *vcpu, u32 msr)
 	case MSR_IA32_PEBS_ENABLE:
 		ret = vcpu->arch.perf_capabilities & PERF_CAP_PEBS_FORMAT;
 		break;
+	case MSR_IA32_DS_AREA:
+		ret = guest_cpuid_has(vcpu, X86_FEATURE_DS);
+		break;
 	default:
 		ret = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) ||
 			get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0) ||
@@ -373,6 +376,9 @@  static int intel_pmu_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_PEBS_ENABLE:
 		msr_info->data = pmu->pebs_enable;
 		return 0;
+	case MSR_IA32_DS_AREA:
+		msr_info->data = pmu->ds_area;
+		return 0;
 	default:
 		if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
 		    (pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) {
@@ -441,6 +447,11 @@  static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			return 0;
 		}
 		break;
+	case MSR_IA32_DS_AREA:
+		if (is_noncanonical_address(data, vcpu))
+			return 1;
+		pmu->ds_area = data;
+		return 0;
 	default:
 		if ((pmc = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)) ||
 		    (pmc = get_gp_pmc(pmu, msr, MSR_IA32_PMC0))) {