[PATCH v1 0/3] KVM: arm64: Do not communicate host pmu event changes by accessing hyp data

[PATCH v1 0/3] KVM: arm64: Do not communicate host pmu event changes by accessing hyp data

[Fuad Tabba]

Hi,

This series changes the way KVM communicates host pmu event changes to the
hypervisor in nvhe and protected mode. Instead of accessing hyp data directly
from the host, the data is passed to hyp via the loaded vcpu. This gives more
isolation between the host and the hypervisor, and would allow us to use pmu in
protected mode.

This series is based on kvmarm/next.

Thanks,
/fuad

Fuad Tabba (3):
  KVM: arm64: Wrapper for getting pmu_events
  KVM: arm64: Pass pmu events to hyp via vcpu
  KVM: arm64: Reenable pmu in Protected Mode

 arch/arm64/include/asm/kvm_host.h |  8 ++----
 arch/arm64/kvm/arm.c              |  2 +-
 arch/arm64/kvm/hyp/nvhe/switch.c  | 20 ++++---------
 arch/arm64/kvm/pmu-emul.c         |  3 +-
 arch/arm64/kvm/pmu.c              | 48 ++++++++++++++++++++-----------
 include/kvm/arm_pmu.h             |  6 ++++
 6 files changed, 47 insertions(+), 40 deletions(-)


base-commit: 21ea457842759a236eefed2cfaa8cc7e5dc967a0
-- 
2.35.1.1178.g4f1659d476-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/3] KVM: arm64: Wrapper for getting pmu_events

[Fuad Tabba]

Eases migrating away from using hyp data and simplifies the code.

No functional change intended.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/kvm/pmu.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
index 03a6c1f4a09a..310d47c9990f 100644
--- a/arch/arm64/kvm/pmu.c
+++ b/arch/arm64/kvm/pmu.c
@@ -25,21 +25,31 @@ static bool kvm_pmu_switch_needed(struct perf_event_attr *attr)
 	return (attr->exclude_host != attr->exclude_guest);
 }
 
+static struct kvm_pmu_events *get_kvm_pmu_events(void)
+{
+	struct kvm_host_data *ctx = this_cpu_ptr_hyp_sym(kvm_host_data);
+
+	if (!ctx)
+		return NULL;
+
+	return &ctx->pmu_events;
+}
+
 /*
  * Add events to track that we may want to switch at guest entry/exit
  * time.
  */
 void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr)
 {
-	struct kvm_host_data *ctx = this_cpu_ptr_hyp_sym(kvm_host_data);
+	struct kvm_pmu_events *pmu = get_kvm_pmu_events();
 
-	if (!kvm_arm_support_pmu_v3() || !ctx || !kvm_pmu_switch_needed(attr))
+	if (!kvm_arm_support_pmu_v3() || !pmu || !kvm_pmu_switch_needed(attr))
 		return;
 
 	if (!attr->exclude_host)
-		ctx->pmu_events.events_host |= set;
+		pmu->events_host |= set;
 	if (!attr->exclude_guest)
-		ctx->pmu_events.events_guest |= set;
+		pmu->events_guest |= set;
 }
 
 /*
@@ -47,13 +57,13 @@ void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr)
  */
 void kvm_clr_pmu_events(u32 clr)
 {
-	struct kvm_host_data *ctx = this_cpu_ptr_hyp_sym(kvm_host_data);
+	struct kvm_pmu_events *pmu = get_kvm_pmu_events();
 
-	if (!kvm_arm_support_pmu_v3() || !ctx)
+	if (!kvm_arm_support_pmu_v3() || !pmu)
 		return;
 
-	ctx->pmu_events.events_host &= ~clr;
-	ctx->pmu_events.events_guest &= ~clr;
+	pmu->events_host &= ~clr;
+	pmu->events_guest &= ~clr;
 }
 
 #define PMEVTYPER_READ_CASE(idx)				\
@@ -169,16 +179,16 @@ static void kvm_vcpu_pmu_disable_el0(unsigned long events)
  */
 void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu)
 {
-	struct kvm_host_data *host;
+	struct kvm_pmu_events *pmu;
 	u32 events_guest, events_host;
 
 	if (!kvm_arm_support_pmu_v3() || !has_vhe())
 		return;
 
 	preempt_disable();
-	host = this_cpu_ptr_hyp_sym(kvm_host_data);
-	events_guest = host->pmu_events.events_guest;
-	events_host = host->pmu_events.events_host;
+	pmu = get_kvm_pmu_events();
+	events_guest = pmu->events_guest;
+	events_host = pmu->events_host;
 
 	kvm_vcpu_pmu_enable_el0(events_guest);
 	kvm_vcpu_pmu_disable_el0(events_host);
@@ -190,15 +200,15 @@ void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu)
  */
 void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu)
 {
-	struct kvm_host_data *host;
+	struct kvm_pmu_events *pmu;
 	u32 events_guest, events_host;
 
 	if (!kvm_arm_support_pmu_v3() || !has_vhe())
 		return;
 
-	host = this_cpu_ptr_hyp_sym(kvm_host_data);
-	events_guest = host->pmu_events.events_guest;
-	events_host = host->pmu_events.events_host;
+	pmu = get_kvm_pmu_events();
+	events_guest = pmu->events_guest;
+	events_host = pmu->events_host;
 
 	kvm_vcpu_pmu_enable_el0(events_host);
 	kvm_vcpu_pmu_disable_el0(events_guest);
-- 
2.35.1.1178.g4f1659d476-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/3] KVM: arm64: Pass pmu events to hyp via vcpu

[Fuad Tabba]

Instead of accessing hyp data, pass the pmu events of the current
cpu to hyp via the loaded vcpu.

No functional change intended.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/include/asm/kvm_host.h |  8 ++------
 arch/arm64/kvm/arm.c              |  2 +-
 arch/arm64/kvm/hyp/nvhe/switch.c  | 20 ++++++--------------
 arch/arm64/kvm/pmu.c              | 22 +++++++++++++---------
 include/kvm/arm_pmu.h             |  6 ++++++
 5 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 0e96087885fe..b5cdfb6cb9c7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -244,14 +244,8 @@ struct kvm_cpu_context {
 	struct kvm_vcpu *__hyp_running_vcpu;
 };
 
-struct kvm_pmu_events {
-	u32 events_host;
-	u32 events_guest;
-};
-
 struct kvm_host_data {
 	struct kvm_cpu_context host_ctxt;
-	struct kvm_pmu_events pmu_events;
 };
 
 struct kvm_host_psci_config {
@@ -728,6 +722,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
 
 DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data);
+DECLARE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events);
 
 static inline void kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt)
 {
@@ -781,6 +776,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
 void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr);
 void kvm_clr_pmu_events(u32 clr);
 
+void kvm_vcpu_pmu_load(struct kvm_vcpu *vcpu);
 void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu);
 void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
 #else
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index ba9165e84396..e6f76d843558 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -400,7 +400,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	if (has_vhe())
 		kvm_vcpu_load_sysregs_vhe(vcpu);
 	kvm_arch_vcpu_load_fp(vcpu);
-	kvm_vcpu_pmu_restore_guest(vcpu);
+	kvm_vcpu_pmu_load(vcpu);
 	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
 		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
 
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index 6410d21d8695..ff7b29fb9787 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -123,13 +123,9 @@ static void __hyp_vgic_restore_state(struct kvm_vcpu *vcpu)
 /**
  * Disable host events, enable guest events
  */
-static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
+static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu)
 {
-	struct kvm_host_data *host;
-	struct kvm_pmu_events *pmu;
-
-	host = container_of(host_ctxt, struct kvm_host_data, host_ctxt);
-	pmu = &host->pmu_events;
+	struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events;
 
 	if (pmu->events_host)
 		write_sysreg(pmu->events_host, pmcntenclr_el0);
@@ -143,13 +139,9 @@ static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
 /**
  * Disable guest events, enable host events
  */
-static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt)
+static void __pmu_switch_to_host(struct kvm_vcpu *vcpu)
 {
-	struct kvm_host_data *host;
-	struct kvm_pmu_events *pmu;
-
-	host = container_of(host_ctxt, struct kvm_host_data, host_ctxt);
-	pmu = &host->pmu_events;
+	struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events;
 
 	if (pmu->events_guest)
 		write_sysreg(pmu->events_guest, pmcntenclr_el0);
@@ -274,7 +266,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 	host_ctxt->__hyp_running_vcpu = vcpu;
 	guest_ctxt = &vcpu->arch.ctxt;
 
-	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
+	pmu_switch_needed = __pmu_switch_to_guest(vcpu);
 
 	__sysreg_save_state_nvhe(host_ctxt);
 	/*
@@ -336,7 +328,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 	__debug_restore_host_buffers_nvhe(vcpu);
 
 	if (pmu_switch_needed)
-		__pmu_switch_to_host(host_ctxt);
+		__pmu_switch_to_host(vcpu);
 
 	/* Returning to host will clear PSR.I, remask PMR if needed */
 	if (system_uses_irq_prio_masking())
diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
index 310d47c9990f..8f722692fb58 100644
--- a/arch/arm64/kvm/pmu.c
+++ b/arch/arm64/kvm/pmu.c
@@ -5,7 +5,8 @@
  */
 #include <linux/kvm_host.h>
 #include <linux/perf_event.h>
-#include <asm/kvm_hyp.h>
+
+DEFINE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events);
 
 /*
  * Given the perf event attributes and system type, determine
@@ -27,12 +28,7 @@ static bool kvm_pmu_switch_needed(struct perf_event_attr *attr)
 
 static struct kvm_pmu_events *get_kvm_pmu_events(void)
 {
-	struct kvm_host_data *ctx = this_cpu_ptr_hyp_sym(kvm_host_data);
-
-	if (!ctx)
-		return NULL;
-
-	return &ctx->pmu_events;
+	return this_cpu_ptr(&kvm_pmu_events);
 }
 
 /*
@@ -43,7 +39,7 @@ void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr)
 {
 	struct kvm_pmu_events *pmu = get_kvm_pmu_events();
 
-	if (!kvm_arm_support_pmu_v3() || !pmu || !kvm_pmu_switch_needed(attr))
+	if (!kvm_arm_support_pmu_v3() || !kvm_pmu_switch_needed(attr))
 		return;
 
 	if (!attr->exclude_host)
@@ -59,7 +55,7 @@ void kvm_clr_pmu_events(u32 clr)
 {
 	struct kvm_pmu_events *pmu = get_kvm_pmu_events();
 
-	if (!kvm_arm_support_pmu_v3() || !pmu)
+	if (!kvm_arm_support_pmu_v3())
 		return;
 
 	pmu->events_host &= ~clr;
@@ -213,3 +209,11 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu)
 	kvm_vcpu_pmu_enable_el0(events_host);
 	kvm_vcpu_pmu_disable_el0(events_guest);
 }
+
+void kvm_vcpu_pmu_load(struct kvm_vcpu *vcpu)
+{
+	kvm_vcpu_pmu_restore_guest(vcpu);
+
+	if (kvm_arm_support_pmu_v3() && !has_vhe())
+		vcpu->arch.pmu.events = *get_kvm_pmu_events();
+}
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 20193416d214..0b3898e0313f 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -20,6 +20,11 @@ struct kvm_pmc {
 	struct perf_event *perf_event;
 };
 
+struct kvm_pmu_events {
+	u32 events_host;
+	u32 events_guest;
+};
+
 struct kvm_pmu {
 	int irq_num;
 	struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS];
@@ -27,6 +32,7 @@ struct kvm_pmu {
 	bool created;
 	bool irq_level;
 	struct irq_work overflow_work;
+	struct kvm_pmu_events events;
 };
 
 struct arm_pmu_entry {
-- 
2.35.1.1178.g4f1659d476-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/3] KVM: arm64: Reenable pmu in Protected Mode

[Fuad Tabba]

Now that the pmu code does not access hyp data, reenable it in
protected mode.

No functional change intended outside of protected mode.

Signed-off-by: Fuad Tabba <tabba@google.com>
---
 arch/arm64/kvm/pmu-emul.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 78fdc443adc7..dc1779d4c7dd 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -756,8 +756,7 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
 {
 	struct arm_pmu_entry *entry;
 
-	if (pmu->pmuver == 0 || pmu->pmuver == ID_AA64DFR0_PMUVER_IMP_DEF ||
-	    is_protected_kvm_enabled())
+	if (pmu->pmuver == 0 || pmu->pmuver == ID_AA64DFR0_PMUVER_IMP_DEF)
 		return;
 
 	mutex_lock(&arm_pmus_lock);
-- 
2.35.1.1178.g4f1659d476-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] KVM: arm64: Pass pmu events to hyp via vcpu

[Marc Zyngier]

On Fri, 08 Apr 2022 09:40:51 +0100,
Fuad Tabba <tabba@google.com> wrote:
> 
> Instead of accessing hyp data, pass the pmu events of the current
> cpu to hyp via the loaded vcpu.

This really deserves a more thorough explanation. What this is doing
is moving the part of the shared memory between EL1 and EL2 into each
vcpu, effectively multiplying the memory usage by the number of vcpus
in the system.

It isn't a big deal (what are 64 bits between friends?), but this is
definitely worth calling out.

> 
> No functional change intended.
> 
> Signed-off-by: Fuad Tabba <tabba@google.com>
> ---
>  arch/arm64/include/asm/kvm_host.h |  8 ++------
>  arch/arm64/kvm/arm.c              |  2 +-
>  arch/arm64/kvm/hyp/nvhe/switch.c  | 20 ++++++--------------
>  arch/arm64/kvm/pmu.c              | 22 +++++++++++++---------
>  include/kvm/arm_pmu.h             |  6 ++++++
>  5 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 0e96087885fe..b5cdfb6cb9c7 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -244,14 +244,8 @@ struct kvm_cpu_context {
>  	struct kvm_vcpu *__hyp_running_vcpu;
>  };
>  
> -struct kvm_pmu_events {
> -	u32 events_host;
> -	u32 events_guest;
> -};
> -
>  struct kvm_host_data {
>  	struct kvm_cpu_context host_ctxt;
> -	struct kvm_pmu_events pmu_events;
>  };
>  
>  struct kvm_host_psci_config {
> @@ -728,6 +722,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
>  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
>  
>  DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data);
> +DECLARE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events);
>  
>  static inline void kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt)
>  {
> @@ -781,6 +776,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
>  void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr);
>  void kvm_clr_pmu_events(u32 clr);
>  
> +void kvm_vcpu_pmu_load(struct kvm_vcpu *vcpu);
>  void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu);
>  void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
>  #else
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index ba9165e84396..e6f76d843558 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -400,7 +400,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	if (has_vhe())
>  		kvm_vcpu_load_sysregs_vhe(vcpu);
>  	kvm_arch_vcpu_load_fp(vcpu);
> -	kvm_vcpu_pmu_restore_guest(vcpu);
> +	kvm_vcpu_pmu_load(vcpu);
>  	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
>  		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
>  
> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index 6410d21d8695..ff7b29fb9787 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -123,13 +123,9 @@ static void __hyp_vgic_restore_state(struct kvm_vcpu *vcpu)
>  /**
>   * Disable host events, enable guest events
>   */
> -static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
> +static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu)
>  {
> -	struct kvm_host_data *host;
> -	struct kvm_pmu_events *pmu;
> -
> -	host = container_of(host_ctxt, struct kvm_host_data, host_ctxt);
> -	pmu = &host->pmu_events;
> +	struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events;
>  
>  	if (pmu->events_host)
>  		write_sysreg(pmu->events_host, pmcntenclr_el0);
> @@ -143,13 +139,9 @@ static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
>  /**
>   * Disable guest events, enable host events
>   */
> -static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt)
> +static void __pmu_switch_to_host(struct kvm_vcpu *vcpu)
>  {
> -	struct kvm_host_data *host;
> -	struct kvm_pmu_events *pmu;
> -
> -	host = container_of(host_ctxt, struct kvm_host_data, host_ctxt);
> -	pmu = &host->pmu_events;
> +	struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events;
>  
>  	if (pmu->events_guest)
>  		write_sysreg(pmu->events_guest, pmcntenclr_el0);
> @@ -274,7 +266,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  	host_ctxt->__hyp_running_vcpu = vcpu;
>  	guest_ctxt = &vcpu->arch.ctxt;
>  
> -	pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
> +	pmu_switch_needed = __pmu_switch_to_guest(vcpu);
>  
>  	__sysreg_save_state_nvhe(host_ctxt);
>  	/*
> @@ -336,7 +328,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
>  	__debug_restore_host_buffers_nvhe(vcpu);
>  
>  	if (pmu_switch_needed)
> -		__pmu_switch_to_host(host_ctxt);
> +		__pmu_switch_to_host(vcpu);
>  
>  	/* Returning to host will clear PSR.I, remask PMR if needed */
>  	if (system_uses_irq_prio_masking())
> diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> index 310d47c9990f..8f722692fb58 100644
> --- a/arch/arm64/kvm/pmu.c
> +++ b/arch/arm64/kvm/pmu.c
> @@ -5,7 +5,8 @@
>   */
>  #include <linux/kvm_host.h>
>  #include <linux/perf_event.h>
> -#include <asm/kvm_hyp.h>
> +
> +DEFINE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events);
>  
>  /*
>   * Given the perf event attributes and system type, determine
> @@ -27,12 +28,7 @@ static bool kvm_pmu_switch_needed(struct perf_event_attr *attr)
>  
>  static struct kvm_pmu_events *get_kvm_pmu_events(void)
>  {
> -	struct kvm_host_data *ctx = this_cpu_ptr_hyp_sym(kvm_host_data);
> -
> -	if (!ctx)
> -		return NULL;
> -
> -	return &ctx->pmu_events;
> +	return this_cpu_ptr(&kvm_pmu_events);
>  }
>  
>  /*
> @@ -43,7 +39,7 @@ void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr)
>  {
>  	struct kvm_pmu_events *pmu = get_kvm_pmu_events();
>  
> -	if (!kvm_arm_support_pmu_v3() || !pmu || !kvm_pmu_switch_needed(attr))
> +	if (!kvm_arm_support_pmu_v3() || !kvm_pmu_switch_needed(attr))
>  		return;
>  
>  	if (!attr->exclude_host)
> @@ -59,7 +55,7 @@ void kvm_clr_pmu_events(u32 clr)
>  {
>  	struct kvm_pmu_events *pmu = get_kvm_pmu_events();
>  
> -	if (!kvm_arm_support_pmu_v3() || !pmu)
> +	if (!kvm_arm_support_pmu_v3())
>  		return;
>  
>  	pmu->events_host &= ~clr;
> @@ -213,3 +209,11 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu)
>  	kvm_vcpu_pmu_enable_el0(events_host);
>  	kvm_vcpu_pmu_disable_el0(events_guest);
>  }
> +
> +void kvm_vcpu_pmu_load(struct kvm_vcpu *vcpu)
> +{
> +	kvm_vcpu_pmu_restore_guest(vcpu);
> +
> +	if (kvm_arm_support_pmu_v3() && !has_vhe())
> +		vcpu->arch.pmu.events = *get_kvm_pmu_events();

What guarantees do you have that the structure isn't updated between
load() and the point where the data is used? For example, can an
interrupt enable or disable a counter behind our back? I have the
feeling it can happen.

> +}
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 20193416d214..0b3898e0313f 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -20,6 +20,11 @@ struct kvm_pmc {
>  	struct perf_event *perf_event;
>  };
>  
> +struct kvm_pmu_events {
> +	u32 events_host;
> +	u32 events_guest;
> +};
> +
>  struct kvm_pmu {
>  	int irq_num;
>  	struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS];
> @@ -27,6 +32,7 @@ struct kvm_pmu {
>  	bool created;
>  	bool irq_level;
>  	struct irq_work overflow_work;
> +	struct kvm_pmu_events events;

This makes be realise that the packing of this structure is absolutely
awful, and that has ripple effects in the vcpu struct.

Can you please reorganise the struct so that irq_num, created and
irq_level are at the end?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] KVM: arm64: Pass pmu events to hyp via vcpu

[Fuad Tabba]

Hi Marc,

On Thu, May 5, 2022 at 10:00 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 08 Apr 2022 09:40:51 +0100,
> Fuad Tabba <tabba@google.com> wrote:
> >
> > Instead of accessing hyp data, pass the pmu events of the current
> > cpu to hyp via the loaded vcpu.
>
> This really deserves a more thorough explanation. What this is doing
> is moving the part of the shared memory between EL1 and EL2 into each
> vcpu, effectively multiplying the memory usage by the number of vcpus
> in the system.
>
> It isn't a big deal (what are 64 bits between friends?), but this is
> definitely worth calling out.

I will explain it and motivate it better in the next respin. The
reasoning behind this is to better isolate hyp (EL2) from the host
(EL1), and not allow the host to simply peek into hyp.

>
> >
> > No functional change intended.
> >
> > Signed-off-by: Fuad Tabba <tabba@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  8 ++------
> >  arch/arm64/kvm/arm.c              |  2 +-
> >  arch/arm64/kvm/hyp/nvhe/switch.c  | 20 ++++++--------------
> >  arch/arm64/kvm/pmu.c              | 22 +++++++++++++---------
> >  include/kvm/arm_pmu.h             |  6 ++++++
> >  5 files changed, 28 insertions(+), 30 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 0e96087885fe..b5cdfb6cb9c7 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -244,14 +244,8 @@ struct kvm_cpu_context {
> >       struct kvm_vcpu *__hyp_running_vcpu;
> >  };
> >
> > -struct kvm_pmu_events {
> > -     u32 events_host;
> > -     u32 events_guest;
> > -};
> > -
> >  struct kvm_host_data {
> >       struct kvm_cpu_context host_ctxt;
> > -     struct kvm_pmu_events pmu_events;
> >  };
> >
> >  struct kvm_host_psci_config {
> > @@ -728,6 +722,7 @@ void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
> >  struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
> >
> >  DECLARE_KVM_HYP_PER_CPU(struct kvm_host_data, kvm_host_data);
> > +DECLARE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events);
> >
> >  static inline void kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt)
> >  {
> > @@ -781,6 +776,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
> >  void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr);
> >  void kvm_clr_pmu_events(u32 clr);
> >
> > +void kvm_vcpu_pmu_load(struct kvm_vcpu *vcpu);
> >  void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu);
> >  void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
> >  #else
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index ba9165e84396..e6f76d843558 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -400,7 +400,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >       if (has_vhe())
> >               kvm_vcpu_load_sysregs_vhe(vcpu);
> >       kvm_arch_vcpu_load_fp(vcpu);
> > -     kvm_vcpu_pmu_restore_guest(vcpu);
> > +     kvm_vcpu_pmu_load(vcpu);
> >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> >
> > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> > index 6410d21d8695..ff7b29fb9787 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> > @@ -123,13 +123,9 @@ static void __hyp_vgic_restore_state(struct kvm_vcpu *vcpu)
> >  /**
> >   * Disable host events, enable guest events
> >   */
> > -static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
> > +static bool __pmu_switch_to_guest(struct kvm_vcpu *vcpu)
> >  {
> > -     struct kvm_host_data *host;
> > -     struct kvm_pmu_events *pmu;
> > -
> > -     host = container_of(host_ctxt, struct kvm_host_data, host_ctxt);
> > -     pmu = &host->pmu_events;
> > +     struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events;
> >
> >       if (pmu->events_host)
> >               write_sysreg(pmu->events_host, pmcntenclr_el0);
> > @@ -143,13 +139,9 @@ static bool __pmu_switch_to_guest(struct kvm_cpu_context *host_ctxt)
> >  /**
> >   * Disable guest events, enable host events
> >   */
> > -static void __pmu_switch_to_host(struct kvm_cpu_context *host_ctxt)
> > +static void __pmu_switch_to_host(struct kvm_vcpu *vcpu)
> >  {
> > -     struct kvm_host_data *host;
> > -     struct kvm_pmu_events *pmu;
> > -
> > -     host = container_of(host_ctxt, struct kvm_host_data, host_ctxt);
> > -     pmu = &host->pmu_events;
> > +     struct kvm_pmu_events *pmu = &vcpu->arch.pmu.events;
> >
> >       if (pmu->events_guest)
> >               write_sysreg(pmu->events_guest, pmcntenclr_el0);
> > @@ -274,7 +266,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> >       host_ctxt->__hyp_running_vcpu = vcpu;
> >       guest_ctxt = &vcpu->arch.ctxt;
> >
> > -     pmu_switch_needed = __pmu_switch_to_guest(host_ctxt);
> > +     pmu_switch_needed = __pmu_switch_to_guest(vcpu);
> >
> >       __sysreg_save_state_nvhe(host_ctxt);
> >       /*
> > @@ -336,7 +328,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
> >       __debug_restore_host_buffers_nvhe(vcpu);
> >
> >       if (pmu_switch_needed)
> > -             __pmu_switch_to_host(host_ctxt);
> > +             __pmu_switch_to_host(vcpu);
> >
> >       /* Returning to host will clear PSR.I, remask PMR if needed */
> >       if (system_uses_irq_prio_masking())
> > diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> > index 310d47c9990f..8f722692fb58 100644
> > --- a/arch/arm64/kvm/pmu.c
> > +++ b/arch/arm64/kvm/pmu.c
> > @@ -5,7 +5,8 @@
> >   */
> >  #include <linux/kvm_host.h>
> >  #include <linux/perf_event.h>
> > -#include <asm/kvm_hyp.h>
> > +
> > +DEFINE_PER_CPU(struct kvm_pmu_events, kvm_pmu_events);
> >
> >  /*
> >   * Given the perf event attributes and system type, determine
> > @@ -27,12 +28,7 @@ static bool kvm_pmu_switch_needed(struct perf_event_attr *attr)
> >
> >  static struct kvm_pmu_events *get_kvm_pmu_events(void)
> >  {
> > -     struct kvm_host_data *ctx = this_cpu_ptr_hyp_sym(kvm_host_data);
> > -
> > -     if (!ctx)
> > -             return NULL;
> > -
> > -     return &ctx->pmu_events;
> > +     return this_cpu_ptr(&kvm_pmu_events);
> >  }
> >
> >  /*
> > @@ -43,7 +39,7 @@ void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr)
> >  {
> >       struct kvm_pmu_events *pmu = get_kvm_pmu_events();
> >
> > -     if (!kvm_arm_support_pmu_v3() || !pmu || !kvm_pmu_switch_needed(attr))
> > +     if (!kvm_arm_support_pmu_v3() || !kvm_pmu_switch_needed(attr))
> >               return;
> >
> >       if (!attr->exclude_host)
> > @@ -59,7 +55,7 @@ void kvm_clr_pmu_events(u32 clr)
> >  {
> >       struct kvm_pmu_events *pmu = get_kvm_pmu_events();
> >
> > -     if (!kvm_arm_support_pmu_v3() || !pmu)
> > +     if (!kvm_arm_support_pmu_v3())
> >               return;
> >
> >       pmu->events_host &= ~clr;
> > @@ -213,3 +209,11 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu)
> >       kvm_vcpu_pmu_enable_el0(events_host);
> >       kvm_vcpu_pmu_disable_el0(events_guest);
> >  }
> > +
> > +void kvm_vcpu_pmu_load(struct kvm_vcpu *vcpu)
> > +{
> > +     kvm_vcpu_pmu_restore_guest(vcpu);
> > +
> > +     if (kvm_arm_support_pmu_v3() && !has_vhe())
> > +             vcpu->arch.pmu.events = *get_kvm_pmu_events();
>
> What guarantees do you have that the structure isn't updated between
> load() and the point where the data is used? For example, can an
> interrupt enable or disable a counter behind our back? I have the
> feeling it can happen.

You're right. It can happen -- I just missed it. I think that the
solution would be to do it at vcpu_run() rather than at vcpu_load().
It would incur more copying, but as someone said, "what are 64 bits
between friends?" :)

> > +}
> > diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> > index 20193416d214..0b3898e0313f 100644
> > --- a/include/kvm/arm_pmu.h
> > +++ b/include/kvm/arm_pmu.h
> > @@ -20,6 +20,11 @@ struct kvm_pmc {
> >       struct perf_event *perf_event;
> >  };
> >
> > +struct kvm_pmu_events {
> > +     u32 events_host;
> > +     u32 events_guest;
> > +};
> > +
> >  struct kvm_pmu {
> >       int irq_num;
> >       struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS];
> > @@ -27,6 +32,7 @@ struct kvm_pmu {
> >       bool created;
> >       bool irq_level;
> >       struct irq_work overflow_work;
> > +     struct kvm_pmu_events events;
>
> This makes be realise that the packing of this structure is absolutely
> awful, and that has ripple effects in the vcpu struct.
>
> Can you please reorganise the struct so that irq_num, created and
> irq_level are at the end?

Will do.

Cheers,
/fuad

>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel