All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-03 11:05 ` Jinank Jain
  0 siblings, 0 replies; 26+ messages in thread
From: Jinank Jain @ 2021-06-03 11:05 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, linux-kernel
  Cc: Jinank Jain, Alexander Graf, Marc Zyngier, James Morse,
	Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon

Currently if a guest is live-migrated while it is actively using perf
counters, then after live-migrate it will notice that all counters would
suddenly start reporting 0s. This is due to the fact we are not
re-creating the relevant perf events inside the kernel.

Usually on live-migration guest state is restored using KVM_SET_ONE_REG
ioctl interface, which simply restores the value of PMU registers
values but does not re-program the perf events so that the guest can seamlessly
use these counters even after live-migration like it was doing before
live-migration.

Instead there are two completely different code path between guest
accessing PMU registers and VMM restoring counters on
live-migration.

In case of KVM_SET_ONE_REG:

kvm_arm_set_reg()
...... kvm_arm_sys_reg_set_reg()
........... reg_from_user()

but in case when guest tries to access these counters:

handle_exit()
..... kvm_handle_sys_reg()
..........perform_access()
...............access_pmu_evcntr()
...................kvm_pmu_set_counter_value()
.......................kvm_pmu_create_perf_event()

The drawback of using the KVM_SET_ONE_REG interface is that the host pmu
events which were registered for the source instance and not present for
the destination instance. Thus passively restoring PMCR_EL0 using
KVM_SET_ONE_REG interface would not create the necessary host pmu events
which are crucial for seamless guest experience across live migration.

In ordet to fix the situation, on first vcpu load we should restore
PMCR_EL0 in the same exact way like the guest was trying to access
these counters. And then we will also recreate the relevant host pmu
events.

Signed-off-by: Jinank Jain <jinankj@amazon.de>
Cc: Alexander Graf (AWS) <graf@amazon.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/arm.c              |  1 +
 arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
 arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
 include/kvm/arm_pmu.h             |  3 +++
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cd7d5c8c4bc..2376ad3c2fc2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -745,6 +745,7 @@ static inline int kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -408,6 +408,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(vcpu);
 	kvm_vcpu_pmu_restore_guest(vcpu);
 	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
 		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fd167d4f4215..12a40f4b5f0d 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
 		kvm_pmu_disable_counter_mask(vcpu, mask);
 	}
 
-	if (val & ARMV8_PMU_PMCR_C)
+	/*
+	 * Cycle counter needs to reset in case of first vcpu load.
+	 */
+	if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))
 		kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
 
-	if (val & ARMV8_PMU_PMCR_P) {
+	/*
+	 * All the counters needs to reset in case of first vcpu load.
+	 */
+	if (val & ARMV8_PMU_PMCR_P || !kvm_arm_pmu_v3_restored(vcpu)) {
 		for_each_set_bit(i, &mask, 32)
 			kvm_pmu_set_counter_value(vcpu, i, 0);
 	}
diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
index 03a6c1f4a09a..574daeeaa4e4 100644
--- a/arch/arm64/kvm/pmu.c
+++ b/arch/arm64/kvm/pmu.c
@@ -161,6 +161,21 @@ static void kvm_vcpu_pmu_disable_el0(unsigned long events)
 	}
 }
 
+/*
+ * Restore PMU events on first vcpu load.
+ */
+void kvm_vcpu_pmu_restore(struct kvm_vcpu *vcpu)
+{
+	if (kvm_arm_pmu_v3_restored(vcpu))
+		return;
+ 
+	u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
+ 
+	kvm_pmu_handle_pmcr(vcpu, val);
+ 
+	vcpu->arch.pmu.restored = true;
+}
+
 /*
  * On VHE ensure that only guest events have EL0 counting enabled.
  * This is called from both vcpu_{load,put} and the sysreg handling.
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 864b9997efb2..7444fd894cf3 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -32,10 +32,12 @@ struct kvm_pmu {
 	struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS];
 	DECLARE_BITMAP(chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
 	bool created;
+	bool restored;
 	bool irq_level;
 	struct irq_work overflow_work;
 };
 
+#define kvm_arm_pmu_v3_restored(v)	((v)->arch.pmu.restored)
 #define kvm_arm_pmu_irq_initialized(v)	((v)->arch.pmu.irq_num >= VGIC_NR_SGIS)
 u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
 void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
@@ -67,6 +69,7 @@ struct kvm_pmu {
 };
 
 #define kvm_arm_pmu_irq_initialized(v)	(false)
+#define kvm_arm_pmu_v3_restored(v)	(false)
 static inline u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu,
 					    u64 select_idx)
 {
-- 
2.31.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-03 11:05 ` Jinank Jain
  0 siblings, 0 replies; 26+ messages in thread
From: Jinank Jain @ 2021-06-03 11:05 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, linux-kernel
  Cc: Alexander Graf, Will Deacon, Marc Zyngier, Catalin Marinas, Jinank Jain

Currently if a guest is live-migrated while it is actively using perf
counters, then after live-migrate it will notice that all counters would
suddenly start reporting 0s. This is due to the fact we are not
re-creating the relevant perf events inside the kernel.

Usually on live-migration guest state is restored using KVM_SET_ONE_REG
ioctl interface, which simply restores the value of PMU registers
values but does not re-program the perf events so that the guest can seamlessly
use these counters even after live-migration like it was doing before
live-migration.

Instead there are two completely different code path between guest
accessing PMU registers and VMM restoring counters on
live-migration.

In case of KVM_SET_ONE_REG:

kvm_arm_set_reg()
...... kvm_arm_sys_reg_set_reg()
........... reg_from_user()

but in case when guest tries to access these counters:

handle_exit()
..... kvm_handle_sys_reg()
..........perform_access()
...............access_pmu_evcntr()
...................kvm_pmu_set_counter_value()
.......................kvm_pmu_create_perf_event()

The drawback of using the KVM_SET_ONE_REG interface is that the host pmu
events which were registered for the source instance and not present for
the destination instance. Thus passively restoring PMCR_EL0 using
KVM_SET_ONE_REG interface would not create the necessary host pmu events
which are crucial for seamless guest experience across live migration.

In ordet to fix the situation, on first vcpu load we should restore
PMCR_EL0 in the same exact way like the guest was trying to access
these counters. And then we will also recreate the relevant host pmu
events.

Signed-off-by: Jinank Jain <jinankj@amazon.de>
Cc: Alexander Graf (AWS) <graf@amazon.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/arm.c              |  1 +
 arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
 arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
 include/kvm/arm_pmu.h             |  3 +++
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cd7d5c8c4bc..2376ad3c2fc2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -745,6 +745,7 @@ static inline int kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -408,6 +408,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(vcpu);
 	kvm_vcpu_pmu_restore_guest(vcpu);
 	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
 		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fd167d4f4215..12a40f4b5f0d 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
 		kvm_pmu_disable_counter_mask(vcpu, mask);
 	}
 
-	if (val & ARMV8_PMU_PMCR_C)
+	/*
+	 * Cycle counter needs to reset in case of first vcpu load.
+	 */
+	if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))
 		kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
 
-	if (val & ARMV8_PMU_PMCR_P) {
+	/*
+	 * All the counters needs to reset in case of first vcpu load.
+	 */
+	if (val & ARMV8_PMU_PMCR_P || !kvm_arm_pmu_v3_restored(vcpu)) {
 		for_each_set_bit(i, &mask, 32)
 			kvm_pmu_set_counter_value(vcpu, i, 0);
 	}
diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
index 03a6c1f4a09a..574daeeaa4e4 100644
--- a/arch/arm64/kvm/pmu.c
+++ b/arch/arm64/kvm/pmu.c
@@ -161,6 +161,21 @@ static void kvm_vcpu_pmu_disable_el0(unsigned long events)
 	}
 }
 
+/*
+ * Restore PMU events on first vcpu load.
+ */
+void kvm_vcpu_pmu_restore(struct kvm_vcpu *vcpu)
+{
+	if (kvm_arm_pmu_v3_restored(vcpu))
+		return;
+ 
+	u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
+ 
+	kvm_pmu_handle_pmcr(vcpu, val);
+ 
+	vcpu->arch.pmu.restored = true;
+}
+
 /*
  * On VHE ensure that only guest events have EL0 counting enabled.
  * This is called from both vcpu_{load,put} and the sysreg handling.
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 864b9997efb2..7444fd894cf3 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -32,10 +32,12 @@ struct kvm_pmu {
 	struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS];
 	DECLARE_BITMAP(chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
 	bool created;
+	bool restored;
 	bool irq_level;
 	struct irq_work overflow_work;
 };
 
+#define kvm_arm_pmu_v3_restored(v)	((v)->arch.pmu.restored)
 #define kvm_arm_pmu_irq_initialized(v)	((v)->arch.pmu.irq_num >= VGIC_NR_SGIS)
 u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
 void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
@@ -67,6 +69,7 @@ struct kvm_pmu {
 };
 
 #define kvm_arm_pmu_irq_initialized(v)	(false)
+#define kvm_arm_pmu_v3_restored(v)	(false)
 static inline u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu,
 					    u64 select_idx)
 {
-- 
2.31.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-03 11:05 ` Jinank Jain
  0 siblings, 0 replies; 26+ messages in thread
From: Jinank Jain @ 2021-06-03 11:05 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, linux-kernel
  Cc: Jinank Jain, Alexander Graf, Marc Zyngier, James Morse,
	Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon

Currently if a guest is live-migrated while it is actively using perf
counters, then after live-migrate it will notice that all counters would
suddenly start reporting 0s. This is due to the fact we are not
re-creating the relevant perf events inside the kernel.

Usually on live-migration guest state is restored using KVM_SET_ONE_REG
ioctl interface, which simply restores the value of PMU registers
values but does not re-program the perf events so that the guest can seamlessly
use these counters even after live-migration like it was doing before
live-migration.

Instead there are two completely different code path between guest
accessing PMU registers and VMM restoring counters on
live-migration.

In case of KVM_SET_ONE_REG:

kvm_arm_set_reg()
...... kvm_arm_sys_reg_set_reg()
........... reg_from_user()

but in case when guest tries to access these counters:

handle_exit()
..... kvm_handle_sys_reg()
..........perform_access()
...............access_pmu_evcntr()
...................kvm_pmu_set_counter_value()
.......................kvm_pmu_create_perf_event()

The drawback of using the KVM_SET_ONE_REG interface is that the host pmu
events which were registered for the source instance and not present for
the destination instance. Thus passively restoring PMCR_EL0 using
KVM_SET_ONE_REG interface would not create the necessary host pmu events
which are crucial for seamless guest experience across live migration.

In ordet to fix the situation, on first vcpu load we should restore
PMCR_EL0 in the same exact way like the guest was trying to access
these counters. And then we will also recreate the relevant host pmu
events.

Signed-off-by: Jinank Jain <jinankj@amazon.de>
Cc: Alexander Graf (AWS) <graf@amazon.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  1 +
 arch/arm64/kvm/arm.c              |  1 +
 arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
 arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
 include/kvm/arm_pmu.h             |  3 +++
 5 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cd7d5c8c4bc..2376ad3c2fc2 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -745,6 +745,7 @@ static inline int kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -408,6 +408,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(vcpu);
 	kvm_vcpu_pmu_restore_guest(vcpu);
 	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
 		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fd167d4f4215..12a40f4b5f0d 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
 		kvm_pmu_disable_counter_mask(vcpu, mask);
 	}
 
-	if (val & ARMV8_PMU_PMCR_C)
+	/*
+	 * Cycle counter needs to reset in case of first vcpu load.
+	 */
+	if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))
 		kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
 
-	if (val & ARMV8_PMU_PMCR_P) {
+	/*
+	 * All the counters needs to reset in case of first vcpu load.
+	 */
+	if (val & ARMV8_PMU_PMCR_P || !kvm_arm_pmu_v3_restored(vcpu)) {
 		for_each_set_bit(i, &mask, 32)
 			kvm_pmu_set_counter_value(vcpu, i, 0);
 	}
diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
index 03a6c1f4a09a..574daeeaa4e4 100644
--- a/arch/arm64/kvm/pmu.c
+++ b/arch/arm64/kvm/pmu.c
@@ -161,6 +161,21 @@ static void kvm_vcpu_pmu_disable_el0(unsigned long events)
 	}
 }
 
+/*
+ * Restore PMU events on first vcpu load.
+ */
+void kvm_vcpu_pmu_restore(struct kvm_vcpu *vcpu)
+{
+	if (kvm_arm_pmu_v3_restored(vcpu))
+		return;
+ 
+	u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
+ 
+	kvm_pmu_handle_pmcr(vcpu, val);
+ 
+	vcpu->arch.pmu.restored = true;
+}
+
 /*
  * On VHE ensure that only guest events have EL0 counting enabled.
  * This is called from both vcpu_{load,put} and the sysreg handling.
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 864b9997efb2..7444fd894cf3 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -32,10 +32,12 @@ struct kvm_pmu {
 	struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS];
 	DECLARE_BITMAP(chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
 	bool created;
+	bool restored;
 	bool irq_level;
 	struct irq_work overflow_work;
 };
 
+#define kvm_arm_pmu_v3_restored(v)	((v)->arch.pmu.restored)
 #define kvm_arm_pmu_irq_initialized(v)	((v)->arch.pmu.irq_num >= VGIC_NR_SGIS)
 u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
 void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
@@ -67,6 +69,7 @@ struct kvm_pmu {
 };
 
 #define kvm_arm_pmu_irq_initialized(v)	(false)
+#define kvm_arm_pmu_v3_restored(v)	(false)
 static inline u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu,
 					    u64 select_idx)
 {
-- 
2.31.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
  2021-06-03 11:05 ` Jinank Jain
  (?)
@ 2021-06-03 15:20   ` kernel test robot
  -1 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2021-06-03 15:20 UTC (permalink / raw)
  To: Jinank Jain, linux-arm-kernel, kvmarm, linux-kernel
  Cc: kbuild-all, Jinank Jain, Alexander Graf, Marc Zyngier,
	James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

Hi Jinank,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kvmarm/next]
[also build test WARNING on arm/for-next soc/for-next arm64/for-next/core v5.13-rc4 next-20210603]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jinank-Jain/KVM-arm64-Properly-restore-PMU-state-during-live-migration/20210603-190755
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/08cb3fc07cb1a68f15eb4a3e68e886c40732e699
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jinank-Jain/KVM-arm64-Properly-restore-PMU-state-during-live-migration/20210603-190755
        git checkout 08cb3fc07cb1a68f15eb4a3e68e886c40732e699
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   arch/arm64/kvm/pmu.c: In function 'kvm_vcpu_pmu_restore':
>> arch/arm64/kvm/pmu.c:172:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
     172 |  u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
         |  ^~~


vim +172 arch/arm64/kvm/pmu.c

   163	
   164	/*
   165	 * Restore PMU events on first vcpu load.
   166	 */
   167	void kvm_vcpu_pmu_restore(struct kvm_vcpu *vcpu)
   168	{
   169		if (kvm_arm_pmu_v3_restored(vcpu))
   170			return;
   171	 
 > 172		u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
   173	 
   174		kvm_pmu_handle_pmcr(vcpu, val);
   175	 
   176		vcpu->arch.pmu.restored = true;
   177	}
   178	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 77689 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-03 15:20   ` kernel test robot
  0 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2021-06-03 15:20 UTC (permalink / raw)
  To: Jinank Jain, linux-arm-kernel, kvmarm, linux-kernel
  Cc: Alexander Graf, kbuild-all, Marc Zyngier, Catalin Marinas, Jinank Jain

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

Hi Jinank,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kvmarm/next]
[also build test WARNING on arm/for-next soc/for-next arm64/for-next/core v5.13-rc4 next-20210603]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jinank-Jain/KVM-arm64-Properly-restore-PMU-state-during-live-migration/20210603-190755
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/08cb3fc07cb1a68f15eb4a3e68e886c40732e699
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jinank-Jain/KVM-arm64-Properly-restore-PMU-state-during-live-migration/20210603-190755
        git checkout 08cb3fc07cb1a68f15eb4a3e68e886c40732e699
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   arch/arm64/kvm/pmu.c: In function 'kvm_vcpu_pmu_restore':
>> arch/arm64/kvm/pmu.c:172:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
     172 |  u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
         |  ^~~


vim +172 arch/arm64/kvm/pmu.c

   163	
   164	/*
   165	 * Restore PMU events on first vcpu load.
   166	 */
   167	void kvm_vcpu_pmu_restore(struct kvm_vcpu *vcpu)
   168	{
   169		if (kvm_arm_pmu_v3_restored(vcpu))
   170			return;
   171	 
 > 172		u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
   173	 
   174		kvm_pmu_handle_pmcr(vcpu, val);
   175	 
   176		vcpu->arch.pmu.restored = true;
   177	}
   178	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 77689 bytes --]

[-- Attachment #3: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-03 15:20   ` kernel test robot
  0 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2021-06-03 15:20 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2374 bytes --]

Hi Jinank,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kvmarm/next]
[also build test WARNING on arm/for-next soc/for-next arm64/for-next/core v5.13-rc4 next-20210603]
[cannot apply to xlnx/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jinank-Jain/KVM-arm64-Properly-restore-PMU-state-during-live-migration/20210603-190755
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/08cb3fc07cb1a68f15eb4a3e68e886c40732e699
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jinank-Jain/KVM-arm64-Properly-restore-PMU-state-during-live-migration/20210603-190755
        git checkout 08cb3fc07cb1a68f15eb4a3e68e886c40732e699
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   arch/arm64/kvm/pmu.c: In function 'kvm_vcpu_pmu_restore':
>> arch/arm64/kvm/pmu.c:172:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
     172 |  u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
         |  ^~~


vim +172 arch/arm64/kvm/pmu.c

   163	
   164	/*
   165	 * Restore PMU events on first vcpu load.
   166	 */
   167	void kvm_vcpu_pmu_restore(struct kvm_vcpu *vcpu)
   168	{
   169		if (kvm_arm_pmu_v3_restored(vcpu))
   170			return;
   171	 
 > 172		u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
   173	 
   174		kvm_pmu_handle_pmcr(vcpu, val);
   175	 
   176		vcpu->arch.pmu.restored = true;
   177	}
   178	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 77689 bytes --]

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
  2021-06-03 11:05 ` Jinank Jain
  (?)
@ 2021-06-03 16:03   ` Marc Zyngier
  -1 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2021-06-03 16:03 UTC (permalink / raw)
  To: Jinank Jain
  Cc: linux-arm-kernel, kvmarm, linux-kernel, Alexander Graf,
	James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas,
	Will Deacon

Hi Jinank,

On Thu, 03 Jun 2021 12:05:54 +0100,
Jinank Jain <jinankj@amazon.de> wrote:
> 
> Currently if a guest is live-migrated while it is actively using perf
> counters, then after live-migrate it will notice that all counters would
> suddenly start reporting 0s. This is due to the fact we are not
> re-creating the relevant perf events inside the kernel.
> 
> Usually on live-migration guest state is restored using KVM_SET_ONE_REG
> ioctl interface, which simply restores the value of PMU registers
> values but does not re-program the perf events so that the guest can seamlessly
> use these counters even after live-migration like it was doing before
> live-migration.
> 
> Instead there are two completely different code path between guest
> accessing PMU registers and VMM restoring counters on
> live-migration.
> 
> In case of KVM_SET_ONE_REG:
> 
> kvm_arm_set_reg()
> ...... kvm_arm_sys_reg_set_reg()
> ........... reg_from_user()
> 
> but in case when guest tries to access these counters:
> 
> handle_exit()
> ..... kvm_handle_sys_reg()
> ..........perform_access()
> ...............access_pmu_evcntr()
> ...................kvm_pmu_set_counter_value()
> .......................kvm_pmu_create_perf_event()
> 
> The drawback of using the KVM_SET_ONE_REG interface is that the host pmu
> events which were registered for the source instance and not present for
> the destination instance.

I can't parse this sentence. Do you mean "are not present"?

> Thus passively restoring PMCR_EL0 using
> KVM_SET_ONE_REG interface would not create the necessary host pmu events
> which are crucial for seamless guest experience across live migration.
> 
> In ordet to fix the situation, on first vcpu load we should restore
> PMCR_EL0 in the same exact way like the guest was trying to access
> these counters. And then we will also recreate the relevant host pmu
> events.
> 
> Signed-off-by: Jinank Jain <jinankj@amazon.de>
> Cc: Alexander Graf (AWS) <graf@amazon.de>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/arm.c              |  1 +
>  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
>  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
>  include/kvm/arm_pmu.h             |  3 +++
>  5 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -745,6 +745,7 @@ static inline int kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -408,6 +408,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(vcpu);

If this only needs to be run once per vcpu, why not trigger it from
kvm_arm_pmu_v3_enable(), which is also called once per vcpu?

This can done on the back of a request, saving most of the overhead
and not requiring any extra field. Essentially, something like the
(untested) patch below.

>  	kvm_vcpu_pmu_restore_guest(vcpu);
>  	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
>  		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index fd167d4f4215..12a40f4b5f0d 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
>  		kvm_pmu_disable_counter_mask(vcpu, mask);
>  	}
>  
> -	if (val & ARMV8_PMU_PMCR_C)
> +	/*
> +	 * Cycle counter needs to reset in case of first vcpu load.
> +	 */
> +	if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))

Why? There is no architectural guarantee that a counter resets to 0
without writing PMCR_EL0.C. And if you want the guest to continue
counting where it left off, resetting the counter is at best
counter-productive.

So I must be missing something...

>  		kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
>  
> -	if (val & ARMV8_PMU_PMCR_P) {
> +	/*
> +	 * All the counters needs to reset in case of first vcpu load.
> +	 */
> +	if (val & ARMV8_PMU_PMCR_P || !kvm_arm_pmu_v3_restored(vcpu)) {

Same thing here.

>  		for_each_set_bit(i, &mask, 32)
>  			kvm_pmu_set_counter_value(vcpu, i, 0);
>  	}

The rest of the changes should be unnecessary with the patch below.

Thanks,

	M.

From 1be188ab71867632a8c17384be6e55f47f42aa8b Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Thu, 3 Jun 2021 16:50:02 +0100
Subject: [PATCH] KVM: arm64: Restore PMU configuration on first run

Restoring a guest with an active virtual PMU results in no perf
counters being instanciated on the host side. Not quite what
you'd expect from a restore.

In order to fix this, force a writeback of PMCR_EL0 on the first
run of a vcpu (using a new request so that it happens once the
vcpu has been loaded). This will in turn create all the host-side
counters that were missing.

Reported-by: Jinank Jain <jinankj@amazon.de>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/arm.c              | 4 ++++
 arch/arm64/kvm/pmu-emul.c         | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cd7d5c8c4bc..6336b4309114 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -46,6 +46,7 @@
 #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
 #define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
 #define KVM_REQ_RELOAD_GICv4	KVM_ARCH_REQ(4)
+#define KVM_REQ_RELOAD_PMU	KVM_ARCH_REQ(5)
 
 #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
 				     KVM_DIRTY_LOG_INITIALLY_SET)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e720148232a0..facf4d41d32a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -689,6 +689,10 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 			vgic_v4_load(vcpu);
 			preempt_enable();
 		}
+
+		if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
+			kvm_pmu_handle_pmcr(vcpu,
+					    __vcpu_sys_reg(vcpu, PMCR_EL0));
 	}
 }
 
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fd167d4f4215..a0bbb7111f57 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -850,6 +850,9 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 		   return -EINVAL;
 	}
 
+	/* One-off reload of the PMU on first run */
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+
 	return 0;
 }
 
-- 
2.30.2


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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-03 16:03   ` Marc Zyngier
  0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2021-06-03 16:03 UTC (permalink / raw)
  To: Jinank Jain
  Cc: Alexander Graf, Will Deacon, Catalin Marinas, linux-kernel,
	kvmarm, linux-arm-kernel

Hi Jinank,

On Thu, 03 Jun 2021 12:05:54 +0100,
Jinank Jain <jinankj@amazon.de> wrote:
> 
> Currently if a guest is live-migrated while it is actively using perf
> counters, then after live-migrate it will notice that all counters would
> suddenly start reporting 0s. This is due to the fact we are not
> re-creating the relevant perf events inside the kernel.
> 
> Usually on live-migration guest state is restored using KVM_SET_ONE_REG
> ioctl interface, which simply restores the value of PMU registers
> values but does not re-program the perf events so that the guest can seamlessly
> use these counters even after live-migration like it was doing before
> live-migration.
> 
> Instead there are two completely different code path between guest
> accessing PMU registers and VMM restoring counters on
> live-migration.
> 
> In case of KVM_SET_ONE_REG:
> 
> kvm_arm_set_reg()
> ...... kvm_arm_sys_reg_set_reg()
> ........... reg_from_user()
> 
> but in case when guest tries to access these counters:
> 
> handle_exit()
> ..... kvm_handle_sys_reg()
> ..........perform_access()
> ...............access_pmu_evcntr()
> ...................kvm_pmu_set_counter_value()
> .......................kvm_pmu_create_perf_event()
> 
> The drawback of using the KVM_SET_ONE_REG interface is that the host pmu
> events which were registered for the source instance and not present for
> the destination instance.

I can't parse this sentence. Do you mean "are not present"?

> Thus passively restoring PMCR_EL0 using
> KVM_SET_ONE_REG interface would not create the necessary host pmu events
> which are crucial for seamless guest experience across live migration.
> 
> In ordet to fix the situation, on first vcpu load we should restore
> PMCR_EL0 in the same exact way like the guest was trying to access
> these counters. And then we will also recreate the relevant host pmu
> events.
> 
> Signed-off-by: Jinank Jain <jinankj@amazon.de>
> Cc: Alexander Graf (AWS) <graf@amazon.de>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/arm.c              |  1 +
>  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
>  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
>  include/kvm/arm_pmu.h             |  3 +++
>  5 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -745,6 +745,7 @@ static inline int kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -408,6 +408,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(vcpu);

If this only needs to be run once per vcpu, why not trigger it from
kvm_arm_pmu_v3_enable(), which is also called once per vcpu?

This can done on the back of a request, saving most of the overhead
and not requiring any extra field. Essentially, something like the
(untested) patch below.

>  	kvm_vcpu_pmu_restore_guest(vcpu);
>  	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
>  		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index fd167d4f4215..12a40f4b5f0d 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
>  		kvm_pmu_disable_counter_mask(vcpu, mask);
>  	}
>  
> -	if (val & ARMV8_PMU_PMCR_C)
> +	/*
> +	 * Cycle counter needs to reset in case of first vcpu load.
> +	 */
> +	if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))

Why? There is no architectural guarantee that a counter resets to 0
without writing PMCR_EL0.C. And if you want the guest to continue
counting where it left off, resetting the counter is at best
counter-productive.

So I must be missing something...

>  		kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
>  
> -	if (val & ARMV8_PMU_PMCR_P) {
> +	/*
> +	 * All the counters needs to reset in case of first vcpu load.
> +	 */
> +	if (val & ARMV8_PMU_PMCR_P || !kvm_arm_pmu_v3_restored(vcpu)) {

Same thing here.

>  		for_each_set_bit(i, &mask, 32)
>  			kvm_pmu_set_counter_value(vcpu, i, 0);
>  	}

The rest of the changes should be unnecessary with the patch below.

Thanks,

	M.

From 1be188ab71867632a8c17384be6e55f47f42aa8b Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Thu, 3 Jun 2021 16:50:02 +0100
Subject: [PATCH] KVM: arm64: Restore PMU configuration on first run

Restoring a guest with an active virtual PMU results in no perf
counters being instanciated on the host side. Not quite what
you'd expect from a restore.

In order to fix this, force a writeback of PMCR_EL0 on the first
run of a vcpu (using a new request so that it happens once the
vcpu has been loaded). This will in turn create all the host-side
counters that were missing.

Reported-by: Jinank Jain <jinankj@amazon.de>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/arm.c              | 4 ++++
 arch/arm64/kvm/pmu-emul.c         | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cd7d5c8c4bc..6336b4309114 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -46,6 +46,7 @@
 #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
 #define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
 #define KVM_REQ_RELOAD_GICv4	KVM_ARCH_REQ(4)
+#define KVM_REQ_RELOAD_PMU	KVM_ARCH_REQ(5)
 
 #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
 				     KVM_DIRTY_LOG_INITIALLY_SET)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e720148232a0..facf4d41d32a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -689,6 +689,10 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 			vgic_v4_load(vcpu);
 			preempt_enable();
 		}
+
+		if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
+			kvm_pmu_handle_pmcr(vcpu,
+					    __vcpu_sys_reg(vcpu, PMCR_EL0));
 	}
 }
 
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fd167d4f4215..a0bbb7111f57 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -850,6 +850,9 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 		   return -EINVAL;
 	}
 
+	/* One-off reload of the PMU on first run */
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+
 	return 0;
 }
 
-- 
2.30.2


-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-03 16:03   ` Marc Zyngier
  0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2021-06-03 16:03 UTC (permalink / raw)
  To: Jinank Jain
  Cc: linux-arm-kernel, kvmarm, linux-kernel, Alexander Graf,
	James Morse, Alexandru Elisei, Suzuki K Poulose, Catalin Marinas,
	Will Deacon

Hi Jinank,

On Thu, 03 Jun 2021 12:05:54 +0100,
Jinank Jain <jinankj@amazon.de> wrote:
> 
> Currently if a guest is live-migrated while it is actively using perf
> counters, then after live-migrate it will notice that all counters would
> suddenly start reporting 0s. This is due to the fact we are not
> re-creating the relevant perf events inside the kernel.
> 
> Usually on live-migration guest state is restored using KVM_SET_ONE_REG
> ioctl interface, which simply restores the value of PMU registers
> values but does not re-program the perf events so that the guest can seamlessly
> use these counters even after live-migration like it was doing before
> live-migration.
> 
> Instead there are two completely different code path between guest
> accessing PMU registers and VMM restoring counters on
> live-migration.
> 
> In case of KVM_SET_ONE_REG:
> 
> kvm_arm_set_reg()
> ...... kvm_arm_sys_reg_set_reg()
> ........... reg_from_user()
> 
> but in case when guest tries to access these counters:
> 
> handle_exit()
> ..... kvm_handle_sys_reg()
> ..........perform_access()
> ...............access_pmu_evcntr()
> ...................kvm_pmu_set_counter_value()
> .......................kvm_pmu_create_perf_event()
> 
> The drawback of using the KVM_SET_ONE_REG interface is that the host pmu
> events which were registered for the source instance and not present for
> the destination instance.

I can't parse this sentence. Do you mean "are not present"?

> Thus passively restoring PMCR_EL0 using
> KVM_SET_ONE_REG interface would not create the necessary host pmu events
> which are crucial for seamless guest experience across live migration.
> 
> In ordet to fix the situation, on first vcpu load we should restore
> PMCR_EL0 in the same exact way like the guest was trying to access
> these counters. And then we will also recreate the relevant host pmu
> events.
> 
> Signed-off-by: Jinank Jain <jinankj@amazon.de>
> Cc: Alexander Graf (AWS) <graf@amazon.de>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: James Morse <james.morse@arm.com>
> Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/arm.c              |  1 +
>  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
>  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
>  include/kvm/arm_pmu.h             |  3 +++
>  5 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -745,6 +745,7 @@ static inline int kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -408,6 +408,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(vcpu);

If this only needs to be run once per vcpu, why not trigger it from
kvm_arm_pmu_v3_enable(), which is also called once per vcpu?

This can done on the back of a request, saving most of the overhead
and not requiring any extra field. Essentially, something like the
(untested) patch below.

>  	kvm_vcpu_pmu_restore_guest(vcpu);
>  	if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
>  		kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index fd167d4f4215..12a40f4b5f0d 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
>  		kvm_pmu_disable_counter_mask(vcpu, mask);
>  	}
>  
> -	if (val & ARMV8_PMU_PMCR_C)
> +	/*
> +	 * Cycle counter needs to reset in case of first vcpu load.
> +	 */
> +	if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))

Why? There is no architectural guarantee that a counter resets to 0
without writing PMCR_EL0.C. And if you want the guest to continue
counting where it left off, resetting the counter is at best
counter-productive.

So I must be missing something...

>  		kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
>  
> -	if (val & ARMV8_PMU_PMCR_P) {
> +	/*
> +	 * All the counters needs to reset in case of first vcpu load.
> +	 */
> +	if (val & ARMV8_PMU_PMCR_P || !kvm_arm_pmu_v3_restored(vcpu)) {

Same thing here.

>  		for_each_set_bit(i, &mask, 32)
>  			kvm_pmu_set_counter_value(vcpu, i, 0);
>  	}

The rest of the changes should be unnecessary with the patch below.

Thanks,

	M.

From 1be188ab71867632a8c17384be6e55f47f42aa8b Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Thu, 3 Jun 2021 16:50:02 +0100
Subject: [PATCH] KVM: arm64: Restore PMU configuration on first run

Restoring a guest with an active virtual PMU results in no perf
counters being instanciated on the host side. Not quite what
you'd expect from a restore.

In order to fix this, force a writeback of PMCR_EL0 on the first
run of a vcpu (using a new request so that it happens once the
vcpu has been loaded). This will in turn create all the host-side
counters that were missing.

Reported-by: Jinank Jain <jinankj@amazon.de>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/arm.c              | 4 ++++
 arch/arm64/kvm/pmu-emul.c         | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cd7d5c8c4bc..6336b4309114 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -46,6 +46,7 @@
 #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
 #define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
 #define KVM_REQ_RELOAD_GICv4	KVM_ARCH_REQ(4)
+#define KVM_REQ_RELOAD_PMU	KVM_ARCH_REQ(5)
 
 #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
 				     KVM_DIRTY_LOG_INITIALLY_SET)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e720148232a0..facf4d41d32a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -689,6 +689,10 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 			vgic_v4_load(vcpu);
 			preempt_enable();
 		}
+
+		if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
+			kvm_pmu_handle_pmcr(vcpu,
+					    __vcpu_sys_reg(vcpu, PMCR_EL0));
 	}
 }
 
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fd167d4f4215..a0bbb7111f57 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -850,6 +850,9 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 		   return -EINVAL;
 	}
 
+	/* One-off reload of the PMU on first run */
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+
 	return 0;
 }
 
-- 
2.30.2


-- 
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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
  2021-06-03 16:03   ` Marc Zyngier
  (?)
@ 2021-06-07 16:05     ` Jain, Jinank
  -1 siblings, 0 replies; 26+ messages in thread
From: Jain, Jinank @ 2021-06-07 16:05 UTC (permalink / raw)
  To: maz
  Cc: james.morse, kvmarm, suzuki.poulose, linux-kernel,
	catalin.marinas, linux-arm-kernel, will, alexandru.elisei,
	Graf (AWS),
	Alexander

On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> Hi Jinank,
> 
> On Thu, 03 Jun 2021 12:05:54 +0100,
> Jinank Jain <jinankj@amazon.de> wrote:
> > Currently if a guest is live-migrated while it is actively using
> > perf
> > counters, then after live-migrate it will notice that all counters
> > would
> > suddenly start reporting 0s. This is due to the fact we are not
> > re-creating the relevant perf events inside the kernel.
> > 
> > Usually on live-migration guest state is restored using
> > KVM_SET_ONE_REG
> > ioctl interface, which simply restores the value of PMU registers
> > values but does not re-program the perf events so that the guest
> > can seamlessly
> > use these counters even after live-migration like it was doing
> > before
> > live-migration.
> > 
> > Instead there are two completely different code path between guest
> > accessing PMU registers and VMM restoring counters on
> > live-migration.
> > 
> > In case of KVM_SET_ONE_REG:
> > 
> > kvm_arm_set_reg()
> > ...... kvm_arm_sys_reg_set_reg()
> > ........... reg_from_user()
> > 
> > but in case when guest tries to access these counters:
> > 
> > handle_exit()
> > ..... kvm_handle_sys_reg()
> > ..........perform_access()
> > ...............access_pmu_evcntr()
> > ...................kvm_pmu_set_counter_value()
> > .......................kvm_pmu_create_perf_event()
> > 
> > The drawback of using the KVM_SET_ONE_REG interface is that the
> > host pmu
> > events which were registered for the source instance and not
> > present for
> > the destination instance.
> 
> I can't parse this sentence. Do you mean "are not present"?
> 
> > Thus passively restoring PMCR_EL0 using
> > KVM_SET_ONE_REG interface would not create the necessary host pmu
> > events
> > which are crucial for seamless guest experience across live
> > migration.
> > 
> > In ordet to fix the situation, on first vcpu load we should restore
> > PMCR_EL0 in the same exact way like the guest was trying to access
> > these counters. And then we will also recreate the relevant host
> > pmu
> > events.
> > 
> > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  arch/arm64/kvm/arm.c              |  1 +
> >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> >  include/kvm/arm_pmu.h             |  3 +++
> >  5 files changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -745,6 +745,7 @@ static inline int
> > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -408,6 +408,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(vcpu);
> 
> If this only needs to be run once per vcpu, why not trigger it from
> kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> 
> This can done on the back of a request, saving most of the overhead
> and not requiring any extra field. Essentially, something like the
> (untested) patch below.
> 
> >       kvm_vcpu_pmu_restore_guest(vcpu);
> >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index fd167d4f4215..12a40f4b5f0d 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > *vcpu, u64 val)
> >               kvm_pmu_disable_counter_mask(vcpu, mask);
> >       }
> > 
> > -     if (val & ARMV8_PMU_PMCR_C)
> > +     /*
> > +      * Cycle counter needs to reset in case of first vcpu load.
> > +      */
> > +     if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))
> 
> Why? There is no architectural guarantee that a counter resets to 0
> without writing PMCR_EL0.C. And if you want the guest to continue
> counting where it left off, resetting the counter is at best
> counter-productive.

Without this we would not be resetting PMU which is required for
creating host perf events. With the patch that you suggested we are
restoring PMCR_EL0 properly but still missing recreation of host perf
events. And without host perf events, guest would still zeros after
live migration. In my opinion we have two ways to fix it. We can fix it
inside the kernel or let userspace/VMM set those bits before restarting
the guest on the destination machine. What do you think?

> 
> So I must be missing something...
> 
> >               kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX,
> > 0);
> > 
> > -     if (val & ARMV8_PMU_PMCR_P) {
> > +     /*
> > +      * All the counters needs to reset in case of first vcpu
> > load.
> > +      */
> > +     if (val & ARMV8_PMU_PMCR_P || !kvm_arm_pmu_v3_restored(vcpu))
> > {
> 
> Same thing here.
> 
> >               for_each_set_bit(i, &mask, 32)
> >                       kvm_pmu_set_counter_value(vcpu, i, 0);
> >       }
> 
> The rest of the changes should be unnecessary with the patch below.
> 
> Thanks,
> 
>         M.
> 
> From 1be188ab71867632a8c17384be6e55f47f42aa8b Mon Sep 17 00:00:00
> 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Thu, 3 Jun 2021 16:50:02 +0100
> Subject: [PATCH] KVM: arm64: Restore PMU configuration on first run
> 
> Restoring a guest with an active virtual PMU results in no perf
> counters being instanciated on the host side. Not quite what
> you'd expect from a restore.
> 
> In order to fix this, force a writeback of PMCR_EL0 on the first
> run of a vcpu (using a new request so that it happens once the
> vcpu has been loaded). This will in turn create all the host-side
> counters that were missing.
> 
> Reported-by: Jinank Jain <jinankj@amazon.de>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h | 1 +
>  arch/arm64/kvm/arm.c              | 4 ++++
>  arch/arm64/kvm/pmu-emul.c         | 3 +++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 7cd7d5c8c4bc..6336b4309114 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -46,6 +46,7 @@
>  #define KVM_REQ_VCPU_RESET     KVM_ARCH_REQ(2)
>  #define KVM_REQ_RECORD_STEAL   KVM_ARCH_REQ(3)
>  #define KVM_REQ_RELOAD_GICv4   KVM_ARCH_REQ(4)
> +#define KVM_REQ_RELOAD_PMU     KVM_ARCH_REQ(5)
> 
>  #define
> KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
>                                      KVM_DIRTY_LOG_INITIALLY_SET)
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e720148232a0..facf4d41d32a 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -689,6 +689,10 @@ static void check_vcpu_requests(struct kvm_vcpu
> *vcpu)
>                         vgic_v4_load(vcpu);
>                         preempt_enable();
>                 }
> +
> +               if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
> +                       kvm_pmu_handle_pmcr(vcpu,
> +                                           __vcpu_sys_reg(vcpu,
> PMCR_EL0));
>         }
>  }
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index fd167d4f4215..a0bbb7111f57 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -850,6 +850,9 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>                    return -EINVAL;
>         }
> 
> +       /* One-off reload of the PMU on first run */
> +       kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +
>         return 0;
>  }
> 
> --
> 2.30.2
> 
> 
> --
> Without deviation from the norm, progress is not possible.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-07 16:05     ` Jain, Jinank
  0 siblings, 0 replies; 26+ messages in thread
From: Jain, Jinank @ 2021-06-07 16:05 UTC (permalink / raw)
  To: maz
  Cc: Graf (AWS),
	Alexander, catalin.marinas, linux-kernel, will, kvmarm,
	linux-arm-kernel

On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> Hi Jinank,
> 
> On Thu, 03 Jun 2021 12:05:54 +0100,
> Jinank Jain <jinankj@amazon.de> wrote:
> > Currently if a guest is live-migrated while it is actively using
> > perf
> > counters, then after live-migrate it will notice that all counters
> > would
> > suddenly start reporting 0s. This is due to the fact we are not
> > re-creating the relevant perf events inside the kernel.
> > 
> > Usually on live-migration guest state is restored using
> > KVM_SET_ONE_REG
> > ioctl interface, which simply restores the value of PMU registers
> > values but does not re-program the perf events so that the guest
> > can seamlessly
> > use these counters even after live-migration like it was doing
> > before
> > live-migration.
> > 
> > Instead there are two completely different code path between guest
> > accessing PMU registers and VMM restoring counters on
> > live-migration.
> > 
> > In case of KVM_SET_ONE_REG:
> > 
> > kvm_arm_set_reg()
> > ...... kvm_arm_sys_reg_set_reg()
> > ........... reg_from_user()
> > 
> > but in case when guest tries to access these counters:
> > 
> > handle_exit()
> > ..... kvm_handle_sys_reg()
> > ..........perform_access()
> > ...............access_pmu_evcntr()
> > ...................kvm_pmu_set_counter_value()
> > .......................kvm_pmu_create_perf_event()
> > 
> > The drawback of using the KVM_SET_ONE_REG interface is that the
> > host pmu
> > events which were registered for the source instance and not
> > present for
> > the destination instance.
> 
> I can't parse this sentence. Do you mean "are not present"?
> 
> > Thus passively restoring PMCR_EL0 using
> > KVM_SET_ONE_REG interface would not create the necessary host pmu
> > events
> > which are crucial for seamless guest experience across live
> > migration.
> > 
> > In ordet to fix the situation, on first vcpu load we should restore
> > PMCR_EL0 in the same exact way like the guest was trying to access
> > these counters. And then we will also recreate the relevant host
> > pmu
> > events.
> > 
> > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  arch/arm64/kvm/arm.c              |  1 +
> >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> >  include/kvm/arm_pmu.h             |  3 +++
> >  5 files changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -745,6 +745,7 @@ static inline int
> > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -408,6 +408,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(vcpu);
> 
> If this only needs to be run once per vcpu, why not trigger it from
> kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> 
> This can done on the back of a request, saving most of the overhead
> and not requiring any extra field. Essentially, something like the
> (untested) patch below.
> 
> >       kvm_vcpu_pmu_restore_guest(vcpu);
> >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index fd167d4f4215..12a40f4b5f0d 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > *vcpu, u64 val)
> >               kvm_pmu_disable_counter_mask(vcpu, mask);
> >       }
> > 
> > -     if (val & ARMV8_PMU_PMCR_C)
> > +     /*
> > +      * Cycle counter needs to reset in case of first vcpu load.
> > +      */
> > +     if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))
> 
> Why? There is no architectural guarantee that a counter resets to 0
> without writing PMCR_EL0.C. And if you want the guest to continue
> counting where it left off, resetting the counter is at best
> counter-productive.

Without this we would not be resetting PMU which is required for
creating host perf events. With the patch that you suggested we are
restoring PMCR_EL0 properly but still missing recreation of host perf
events. And without host perf events, guest would still zeros after
live migration. In my opinion we have two ways to fix it. We can fix it
inside the kernel or let userspace/VMM set those bits before restarting
the guest on the destination machine. What do you think?

> 
> So I must be missing something...
> 
> >               kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX,
> > 0);
> > 
> > -     if (val & ARMV8_PMU_PMCR_P) {
> > +     /*
> > +      * All the counters needs to reset in case of first vcpu
> > load.
> > +      */
> > +     if (val & ARMV8_PMU_PMCR_P || !kvm_arm_pmu_v3_restored(vcpu))
> > {
> 
> Same thing here.
> 
> >               for_each_set_bit(i, &mask, 32)
> >                       kvm_pmu_set_counter_value(vcpu, i, 0);
> >       }
> 
> The rest of the changes should be unnecessary with the patch below.
> 
> Thanks,
> 
>         M.
> 
> From 1be188ab71867632a8c17384be6e55f47f42aa8b Mon Sep 17 00:00:00
> 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Thu, 3 Jun 2021 16:50:02 +0100
> Subject: [PATCH] KVM: arm64: Restore PMU configuration on first run
> 
> Restoring a guest with an active virtual PMU results in no perf
> counters being instanciated on the host side. Not quite what
> you'd expect from a restore.
> 
> In order to fix this, force a writeback of PMCR_EL0 on the first
> run of a vcpu (using a new request so that it happens once the
> vcpu has been loaded). This will in turn create all the host-side
> counters that were missing.
> 
> Reported-by: Jinank Jain <jinankj@amazon.de>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h | 1 +
>  arch/arm64/kvm/arm.c              | 4 ++++
>  arch/arm64/kvm/pmu-emul.c         | 3 +++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 7cd7d5c8c4bc..6336b4309114 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -46,6 +46,7 @@
>  #define KVM_REQ_VCPU_RESET     KVM_ARCH_REQ(2)
>  #define KVM_REQ_RECORD_STEAL   KVM_ARCH_REQ(3)
>  #define KVM_REQ_RELOAD_GICv4   KVM_ARCH_REQ(4)
> +#define KVM_REQ_RELOAD_PMU     KVM_ARCH_REQ(5)
> 
>  #define
> KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
>                                      KVM_DIRTY_LOG_INITIALLY_SET)
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e720148232a0..facf4d41d32a 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -689,6 +689,10 @@ static void check_vcpu_requests(struct kvm_vcpu
> *vcpu)
>                         vgic_v4_load(vcpu);
>                         preempt_enable();
>                 }
> +
> +               if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
> +                       kvm_pmu_handle_pmcr(vcpu,
> +                                           __vcpu_sys_reg(vcpu,
> PMCR_EL0));
>         }
>  }
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index fd167d4f4215..a0bbb7111f57 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -850,6 +850,9 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>                    return -EINVAL;
>         }
> 
> +       /* One-off reload of the PMU on first run */
> +       kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +
>         return 0;
>  }
> 
> --
> 2.30.2
> 
> 
> --
> Without deviation from the norm, progress is not possible.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-07 16:05     ` Jain, Jinank
  0 siblings, 0 replies; 26+ messages in thread
From: Jain, Jinank @ 2021-06-07 16:05 UTC (permalink / raw)
  To: maz
  Cc: james.morse, kvmarm, suzuki.poulose, linux-kernel,
	catalin.marinas, linux-arm-kernel, will, alexandru.elisei,
	Graf (AWS),
	Alexander

On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> Hi Jinank,
> 
> On Thu, 03 Jun 2021 12:05:54 +0100,
> Jinank Jain <jinankj@amazon.de> wrote:
> > Currently if a guest is live-migrated while it is actively using
> > perf
> > counters, then after live-migrate it will notice that all counters
> > would
> > suddenly start reporting 0s. This is due to the fact we are not
> > re-creating the relevant perf events inside the kernel.
> > 
> > Usually on live-migration guest state is restored using
> > KVM_SET_ONE_REG
> > ioctl interface, which simply restores the value of PMU registers
> > values but does not re-program the perf events so that the guest
> > can seamlessly
> > use these counters even after live-migration like it was doing
> > before
> > live-migration.
> > 
> > Instead there are two completely different code path between guest
> > accessing PMU registers and VMM restoring counters on
> > live-migration.
> > 
> > In case of KVM_SET_ONE_REG:
> > 
> > kvm_arm_set_reg()
> > ...... kvm_arm_sys_reg_set_reg()
> > ........... reg_from_user()
> > 
> > but in case when guest tries to access these counters:
> > 
> > handle_exit()
> > ..... kvm_handle_sys_reg()
> > ..........perform_access()
> > ...............access_pmu_evcntr()
> > ...................kvm_pmu_set_counter_value()
> > .......................kvm_pmu_create_perf_event()
> > 
> > The drawback of using the KVM_SET_ONE_REG interface is that the
> > host pmu
> > events which were registered for the source instance and not
> > present for
> > the destination instance.
> 
> I can't parse this sentence. Do you mean "are not present"?
> 
> > Thus passively restoring PMCR_EL0 using
> > KVM_SET_ONE_REG interface would not create the necessary host pmu
> > events
> > which are crucial for seamless guest experience across live
> > migration.
> > 
> > In ordet to fix the situation, on first vcpu load we should restore
> > PMCR_EL0 in the same exact way like the guest was trying to access
> > these counters. And then we will also recreate the relevant host
> > pmu
> > events.
> > 
> > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > ---
> >  arch/arm64/include/asm/kvm_host.h |  1 +
> >  arch/arm64/kvm/arm.c              |  1 +
> >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> >  include/kvm/arm_pmu.h             |  3 +++
> >  5 files changed, 28 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -745,6 +745,7 @@ static inline int
> > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -408,6 +408,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(vcpu);
> 
> If this only needs to be run once per vcpu, why not trigger it from
> kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> 
> This can done on the back of a request, saving most of the overhead
> and not requiring any extra field. Essentially, something like the
> (untested) patch below.
> 
> >       kvm_vcpu_pmu_restore_guest(vcpu);
> >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index fd167d4f4215..12a40f4b5f0d 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > *vcpu, u64 val)
> >               kvm_pmu_disable_counter_mask(vcpu, mask);
> >       }
> > 
> > -     if (val & ARMV8_PMU_PMCR_C)
> > +     /*
> > +      * Cycle counter needs to reset in case of first vcpu load.
> > +      */
> > +     if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))
> 
> Why? There is no architectural guarantee that a counter resets to 0
> without writing PMCR_EL0.C. And if you want the guest to continue
> counting where it left off, resetting the counter is at best
> counter-productive.

Without this we would not be resetting PMU which is required for
creating host perf events. With the patch that you suggested we are
restoring PMCR_EL0 properly but still missing recreation of host perf
events. And without host perf events, guest would still zeros after
live migration. In my opinion we have two ways to fix it. We can fix it
inside the kernel or let userspace/VMM set those bits before restarting
the guest on the destination machine. What do you think?

> 
> So I must be missing something...
> 
> >               kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX,
> > 0);
> > 
> > -     if (val & ARMV8_PMU_PMCR_P) {
> > +     /*
> > +      * All the counters needs to reset in case of first vcpu
> > load.
> > +      */
> > +     if (val & ARMV8_PMU_PMCR_P || !kvm_arm_pmu_v3_restored(vcpu))
> > {
> 
> Same thing here.
> 
> >               for_each_set_bit(i, &mask, 32)
> >                       kvm_pmu_set_counter_value(vcpu, i, 0);
> >       }
> 
> The rest of the changes should be unnecessary with the patch below.
> 
> Thanks,
> 
>         M.
> 
> From 1be188ab71867632a8c17384be6e55f47f42aa8b Mon Sep 17 00:00:00
> 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Thu, 3 Jun 2021 16:50:02 +0100
> Subject: [PATCH] KVM: arm64: Restore PMU configuration on first run
> 
> Restoring a guest with an active virtual PMU results in no perf
> counters being instanciated on the host side. Not quite what
> you'd expect from a restore.
> 
> In order to fix this, force a writeback of PMCR_EL0 on the first
> run of a vcpu (using a new request so that it happens once the
> vcpu has been loaded). This will in turn create all the host-side
> counters that were missing.
> 
> Reported-by: Jinank Jain <jinankj@amazon.de>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h | 1 +
>  arch/arm64/kvm/arm.c              | 4 ++++
>  arch/arm64/kvm/pmu-emul.c         | 3 +++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 7cd7d5c8c4bc..6336b4309114 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -46,6 +46,7 @@
>  #define KVM_REQ_VCPU_RESET     KVM_ARCH_REQ(2)
>  #define KVM_REQ_RECORD_STEAL   KVM_ARCH_REQ(3)
>  #define KVM_REQ_RELOAD_GICv4   KVM_ARCH_REQ(4)
> +#define KVM_REQ_RELOAD_PMU     KVM_ARCH_REQ(5)
> 
>  #define
> KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
>                                      KVM_DIRTY_LOG_INITIALLY_SET)
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e720148232a0..facf4d41d32a 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -689,6 +689,10 @@ static void check_vcpu_requests(struct kvm_vcpu
> *vcpu)
>                         vgic_v4_load(vcpu);
>                         preempt_enable();
>                 }
> +
> +               if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
> +                       kvm_pmu_handle_pmcr(vcpu,
> +                                           __vcpu_sys_reg(vcpu,
> PMCR_EL0));
>         }
>  }
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index fd167d4f4215..a0bbb7111f57 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -850,6 +850,9 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>                    return -EINVAL;
>         }
> 
> +       /* One-off reload of the PMU on first run */
> +       kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +
>         return 0;
>  }
> 
> --
> 2.30.2
> 
> 
> --
> Without deviation from the norm, progress is not possible.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
  2021-06-07 16:05     ` Jain, Jinank
@ 2021-06-07 16:35       ` Marc Zyngier
  -1 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2021-06-07 16:35 UTC (permalink / raw)
  To: Jain, Jinank
  Cc: Graf (AWS),
	Alexander, catalin.marinas, linux-kernel, will, kvmarm,
	linux-arm-kernel

On Mon, 07 Jun 2021 17:05:01 +0100,
"Jain, Jinank" <jinankj@amazon.de> wrote:
> 
> On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> >
> > Hi Jinank,
> > 
> > On Thu, 03 Jun 2021 12:05:54 +0100,
> > Jinank Jain <jinankj@amazon.de> wrote:
> > > Currently if a guest is live-migrated while it is actively using
> > > perf
> > > counters, then after live-migrate it will notice that all counters
> > > would
> > > suddenly start reporting 0s. This is due to the fact we are not
> > > re-creating the relevant perf events inside the kernel.
> > > 
> > > Usually on live-migration guest state is restored using
> > > KVM_SET_ONE_REG
> > > ioctl interface, which simply restores the value of PMU registers
> > > values but does not re-program the perf events so that the guest
> > > can seamlessly
> > > use these counters even after live-migration like it was doing
> > > before
> > > live-migration.
> > > 
> > > Instead there are two completely different code path between guest
> > > accessing PMU registers and VMM restoring counters on
> > > live-migration.
> > > 
> > > In case of KVM_SET_ONE_REG:
> > > 
> > > kvm_arm_set_reg()
> > > ...... kvm_arm_sys_reg_set_reg()
> > > ........... reg_from_user()
> > > 
> > > but in case when guest tries to access these counters:
> > > 
> > > handle_exit()
> > > ..... kvm_handle_sys_reg()
> > > ..........perform_access()
> > > ...............access_pmu_evcntr()
> > > ...................kvm_pmu_set_counter_value()
> > > .......................kvm_pmu_create_perf_event()
> > > 
> > > The drawback of using the KVM_SET_ONE_REG interface is that the
> > > host pmu
> > > events which were registered for the source instance and not
> > > present for
> > > the destination instance.
> > 
> > I can't parse this sentence. Do you mean "are not present"?
> > 
> > > Thus passively restoring PMCR_EL0 using
> > > KVM_SET_ONE_REG interface would not create the necessary host pmu
> > > events
> > > which are crucial for seamless guest experience across live
> > > migration.
> > > 
> > > In ordet to fix the situation, on first vcpu load we should restore
> > > PMCR_EL0 in the same exact way like the guest was trying to access
> > > these counters. And then we will also recreate the relevant host
> > > pmu
> > > events.
> > > 
> > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: James Morse <james.morse@arm.com>
> > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > >  arch/arm64/kvm/arm.c              |  1 +
> > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > >  include/kvm/arm_pmu.h             |  3 +++
> > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > b/arch/arm64/include/asm/kvm_host.h
> > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -745,6 +745,7 @@ static inline int
> > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -408,6 +408,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(vcpu);
> > 
> > If this only needs to be run once per vcpu, why not trigger it from
> > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > 
> > This can done on the back of a request, saving most of the overhead
> > and not requiring any extra field. Essentially, something like the
> > (untested) patch below.
> > 
> > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > > index fd167d4f4215..12a40f4b5f0d 100644
> > > --- a/arch/arm64/kvm/pmu-emul.c
> > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > > *vcpu, u64 val)
> > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > >       }
> > > 
> > > -     if (val & ARMV8_PMU_PMCR_C)
> > > +     /*
> > > +      * Cycle counter needs to reset in case of first vcpu load.
> > > +      */
> > > +     if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))
> > 
> > Why? There is no architectural guarantee that a counter resets to 0
> > without writing PMCR_EL0.C. And if you want the guest to continue
> > counting where it left off, resetting the counter is at best
> > counter-productive.
> 
> Without this we would not be resetting PMU which is required for
> creating host perf events. With the patch that you suggested we are
> restoring PMCR_EL0 properly but still missing recreation of host perf
> events.

How? The request that gets set on the first vcpu run will call
kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
kvm_pmu_create_perf_event(). What are we missing?

> And without host perf events, guest would still zeros after live
> migration. In my opinion we have two ways to fix it. We can fix it
> inside the kernel or let userspace/VMM set those bits before
> restarting the guest on the destination machine. What do you think?

I think either you're missing my point above, or I'm completely
missing yours. And I still don't understand why you want to zero the
counters that you have just restored. How does that help?

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-07 16:35       ` Marc Zyngier
  0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2021-06-07 16:35 UTC (permalink / raw)
  To: Jain, Jinank
  Cc: james.morse, kvmarm, suzuki.poulose, linux-kernel,
	catalin.marinas, linux-arm-kernel, will, alexandru.elisei,
	Graf (AWS),
	Alexander

On Mon, 07 Jun 2021 17:05:01 +0100,
"Jain, Jinank" <jinankj@amazon.de> wrote:
> 
> On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> >
> > Hi Jinank,
> > 
> > On Thu, 03 Jun 2021 12:05:54 +0100,
> > Jinank Jain <jinankj@amazon.de> wrote:
> > > Currently if a guest is live-migrated while it is actively using
> > > perf
> > > counters, then after live-migrate it will notice that all counters
> > > would
> > > suddenly start reporting 0s. This is due to the fact we are not
> > > re-creating the relevant perf events inside the kernel.
> > > 
> > > Usually on live-migration guest state is restored using
> > > KVM_SET_ONE_REG
> > > ioctl interface, which simply restores the value of PMU registers
> > > values but does not re-program the perf events so that the guest
> > > can seamlessly
> > > use these counters even after live-migration like it was doing
> > > before
> > > live-migration.
> > > 
> > > Instead there are two completely different code path between guest
> > > accessing PMU registers and VMM restoring counters on
> > > live-migration.
> > > 
> > > In case of KVM_SET_ONE_REG:
> > > 
> > > kvm_arm_set_reg()
> > > ...... kvm_arm_sys_reg_set_reg()
> > > ........... reg_from_user()
> > > 
> > > but in case when guest tries to access these counters:
> > > 
> > > handle_exit()
> > > ..... kvm_handle_sys_reg()
> > > ..........perform_access()
> > > ...............access_pmu_evcntr()
> > > ...................kvm_pmu_set_counter_value()
> > > .......................kvm_pmu_create_perf_event()
> > > 
> > > The drawback of using the KVM_SET_ONE_REG interface is that the
> > > host pmu
> > > events which were registered for the source instance and not
> > > present for
> > > the destination instance.
> > 
> > I can't parse this sentence. Do you mean "are not present"?
> > 
> > > Thus passively restoring PMCR_EL0 using
> > > KVM_SET_ONE_REG interface would not create the necessary host pmu
> > > events
> > > which are crucial for seamless guest experience across live
> > > migration.
> > > 
> > > In ordet to fix the situation, on first vcpu load we should restore
> > > PMCR_EL0 in the same exact way like the guest was trying to access
> > > these counters. And then we will also recreate the relevant host
> > > pmu
> > > events.
> > > 
> > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: James Morse <james.morse@arm.com>
> > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > ---
> > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > >  arch/arm64/kvm/arm.c              |  1 +
> > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > >  include/kvm/arm_pmu.h             |  3 +++
> > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > b/arch/arm64/include/asm/kvm_host.h
> > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -745,6 +745,7 @@ static inline int
> > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -408,6 +408,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(vcpu);
> > 
> > If this only needs to be run once per vcpu, why not trigger it from
> > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > 
> > This can done on the back of a request, saving most of the overhead
> > and not requiring any extra field. Essentially, something like the
> > (untested) patch below.
> > 
> > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > > index fd167d4f4215..12a40f4b5f0d 100644
> > > --- a/arch/arm64/kvm/pmu-emul.c
> > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > > *vcpu, u64 val)
> > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > >       }
> > > 
> > > -     if (val & ARMV8_PMU_PMCR_C)
> > > +     /*
> > > +      * Cycle counter needs to reset in case of first vcpu load.
> > > +      */
> > > +     if (val & ARMV8_PMU_PMCR_C || !kvm_arm_pmu_v3_restored(vcpu))
> > 
> > Why? There is no architectural guarantee that a counter resets to 0
> > without writing PMCR_EL0.C. And if you want the guest to continue
> > counting where it left off, resetting the counter is at best
> > counter-productive.
> 
> Without this we would not be resetting PMU which is required for
> creating host perf events. With the patch that you suggested we are
> restoring PMCR_EL0 properly but still missing recreation of host perf
> events.

How? The request that gets set on the first vcpu run will call
kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
kvm_pmu_create_perf_event(). What are we missing?

> And without host perf events, guest would still zeros after live
> migration. In my opinion we have two ways to fix it. We can fix it
> inside the kernel or let userspace/VMM set those bits before
> restarting the guest on the destination machine. What do you think?

I think either you're missing my point above, or I'm completely
missing yours. And I still don't understand why you want to zero the
counters that you have just restored. How does that help?

	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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
  2021-06-07 16:35       ` Marc Zyngier
  (?)
@ 2021-06-07 18:34         ` Jain, Jinank
  -1 siblings, 0 replies; 26+ messages in thread
From: Jain, Jinank @ 2021-06-07 18:34 UTC (permalink / raw)
  To: maz
  Cc: james.morse, kvmarm, suzuki.poulose, linux-kernel,
	alexandru.elisei, linux-arm-kernel, will, catalin.marinas,
	Graf (AWS),
	Alexander

Hi Marc.

On Mon, 2021-06-07 at 17:35 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> On Mon, 07 Jun 2021 17:05:01 +0100,
> "Jain, Jinank" <jinankj@amazon.de> wrote:
> > On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> > > Hi Jinank,
> > > 
> > > On Thu, 03 Jun 2021 12:05:54 +0100,
> > > Jinank Jain <jinankj@amazon.de> wrote:
> > > > Currently if a guest is live-migrated while it is actively
> > > > using
> > > > perf
> > > > counters, then after live-migrate it will notice that all
> > > > counters
> > > > would
> > > > suddenly start reporting 0s. This is due to the fact we are not
> > > > re-creating the relevant perf events inside the kernel.
> > > > 
> > > > Usually on live-migration guest state is restored using
> > > > KVM_SET_ONE_REG
> > > > ioctl interface, which simply restores the value of PMU
> > > > registers
> > > > values but does not re-program the perf events so that the
> > > > guest
> > > > can seamlessly
> > > > use these counters even after live-migration like it was doing
> > > > before
> > > > live-migration.
> > > > 
> > > > Instead there are two completely different code path between
> > > > guest
> > > > accessing PMU registers and VMM restoring counters on
> > > > live-migration.
> > > > 
> > > > In case of KVM_SET_ONE_REG:
> > > > 
> > > > kvm_arm_set_reg()
> > > > ...... kvm_arm_sys_reg_set_reg()
> > > > ........... reg_from_user()
> > > > 
> > > > but in case when guest tries to access these counters:
> > > > 
> > > > handle_exit()
> > > > ..... kvm_handle_sys_reg()
> > > > ..........perform_access()
> > > > ...............access_pmu_evcntr()
> > > > ...................kvm_pmu_set_counter_value()
> > > > .......................kvm_pmu_create_perf_event()
> > > > 
> > > > The drawback of using the KVM_SET_ONE_REG interface is that the
> > > > host pmu
> > > > events which were registered for the source instance and not
> > > > present for
> > > > the destination instance.
> > > 
> > > I can't parse this sentence. Do you mean "are not present"?
> > > 
> > > > Thus passively restoring PMCR_EL0 using
> > > > KVM_SET_ONE_REG interface would not create the necessary host
> > > > pmu
> > > > events
> > > > which are crucial for seamless guest experience across live
> > > > migration.
> > > > 
> > > > In ordet to fix the situation, on first vcpu load we should
> > > > restore
> > > > PMCR_EL0 in the same exact way like the guest was trying to
> > > > access
> > > > these counters. And then we will also recreate the relevant
> > > > host
> > > > pmu
> > > > events.
> > > > 
> > > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > Cc: James Morse <james.morse@arm.com>
> > > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Will Deacon <will@kernel.org>
> > > > ---
> > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > >  arch/arm64/kvm/arm.c              |  1 +
> > > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > > >  include/kvm/arm_pmu.h             |  3 +++
> > > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > b/arch/arm64/include/asm/kvm_host.h
> > > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -745,6 +745,7 @@ static inline int
> > > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > > --- a/arch/arm64/kvm/arm.c
> > > > +++ b/arch/arm64/kvm/arm.c
> > > > @@ -408,6 +408,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(vcpu);
> > > 
> > > If this only needs to be run once per vcpu, why not trigger it
> > > from
> > > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > > 
> > > This can done on the back of a request, saving most of the
> > > overhead
> > > and not requiring any extra field. Essentially, something like
> > > the
> > > (untested) patch below.
> > > 
> > > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-
> > > > emul.c
> > > > index fd167d4f4215..12a40f4b5f0d 100644
> > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > > > *vcpu, u64 val)
> > > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > > >       }
> > > > 
> > > > -     if (val & ARMV8_PMU_PMCR_C)
> > > > +     /*
> > > > +      * Cycle counter needs to reset in case of first vcpu
> > > > load.
> > > > +      */
> > > > +     if (val & ARMV8_PMU_PMCR_C ||
> > > > !kvm_arm_pmu_v3_restored(vcpu))
> > > 
> > > Why? There is no architectural guarantee that a counter resets to
> > > 0
> > > without writing PMCR_EL0.C. And if you want the guest to continue
> > > counting where it left off, resetting the counter is at best
> > > counter-productive.
> > 
> > Without this we would not be resetting PMU which is required for
> > creating host perf events. With the patch that you suggested we are
> > restoring PMCR_EL0 properly but still missing recreation of host
> > perf
> > events.
> 
> How? The request that gets set on the first vcpu run will call
> kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
> kvm_pmu_create_perf_event(). What are we missing?
> 

I found out what I was missing. I was working with an older kernel
which was missing this upstream patch:

https://lore.kernel.org/lkml/20200124142535.29386-3-eric.auger@redhat.com/

> > And without host perf events, guest would still zeros after live
> > migration. In my opinion we have two ways to fix it. We can fix it
> > inside the kernel or let userspace/VMM set those bits before
> > restarting the guest on the destination machine. What do you think?
> 
> I think either you're missing my point above, or I'm completely
> missing yours. And I still don't understand why you want to zero the
> counters that you have just restored. How does that help?
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-07 18:34         ` Jain, Jinank
  0 siblings, 0 replies; 26+ messages in thread
From: Jain, Jinank @ 2021-06-07 18:34 UTC (permalink / raw)
  To: maz
  Cc: Graf (AWS),
	Alexander, will, linux-kernel, catalin.marinas, kvmarm,
	linux-arm-kernel

Hi Marc.

On Mon, 2021-06-07 at 17:35 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> On Mon, 07 Jun 2021 17:05:01 +0100,
> "Jain, Jinank" <jinankj@amazon.de> wrote:
> > On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> > > Hi Jinank,
> > > 
> > > On Thu, 03 Jun 2021 12:05:54 +0100,
> > > Jinank Jain <jinankj@amazon.de> wrote:
> > > > Currently if a guest is live-migrated while it is actively
> > > > using
> > > > perf
> > > > counters, then after live-migrate it will notice that all
> > > > counters
> > > > would
> > > > suddenly start reporting 0s. This is due to the fact we are not
> > > > re-creating the relevant perf events inside the kernel.
> > > > 
> > > > Usually on live-migration guest state is restored using
> > > > KVM_SET_ONE_REG
> > > > ioctl interface, which simply restores the value of PMU
> > > > registers
> > > > values but does not re-program the perf events so that the
> > > > guest
> > > > can seamlessly
> > > > use these counters even after live-migration like it was doing
> > > > before
> > > > live-migration.
> > > > 
> > > > Instead there are two completely different code path between
> > > > guest
> > > > accessing PMU registers and VMM restoring counters on
> > > > live-migration.
> > > > 
> > > > In case of KVM_SET_ONE_REG:
> > > > 
> > > > kvm_arm_set_reg()
> > > > ...... kvm_arm_sys_reg_set_reg()
> > > > ........... reg_from_user()
> > > > 
> > > > but in case when guest tries to access these counters:
> > > > 
> > > > handle_exit()
> > > > ..... kvm_handle_sys_reg()
> > > > ..........perform_access()
> > > > ...............access_pmu_evcntr()
> > > > ...................kvm_pmu_set_counter_value()
> > > > .......................kvm_pmu_create_perf_event()
> > > > 
> > > > The drawback of using the KVM_SET_ONE_REG interface is that the
> > > > host pmu
> > > > events which were registered for the source instance and not
> > > > present for
> > > > the destination instance.
> > > 
> > > I can't parse this sentence. Do you mean "are not present"?
> > > 
> > > > Thus passively restoring PMCR_EL0 using
> > > > KVM_SET_ONE_REG interface would not create the necessary host
> > > > pmu
> > > > events
> > > > which are crucial for seamless guest experience across live
> > > > migration.
> > > > 
> > > > In ordet to fix the situation, on first vcpu load we should
> > > > restore
> > > > PMCR_EL0 in the same exact way like the guest was trying to
> > > > access
> > > > these counters. And then we will also recreate the relevant
> > > > host
> > > > pmu
> > > > events.
> > > > 
> > > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > Cc: James Morse <james.morse@arm.com>
> > > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Will Deacon <will@kernel.org>
> > > > ---
> > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > >  arch/arm64/kvm/arm.c              |  1 +
> > > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > > >  include/kvm/arm_pmu.h             |  3 +++
> > > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > b/arch/arm64/include/asm/kvm_host.h
> > > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -745,6 +745,7 @@ static inline int
> > > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > > --- a/arch/arm64/kvm/arm.c
> > > > +++ b/arch/arm64/kvm/arm.c
> > > > @@ -408,6 +408,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(vcpu);
> > > 
> > > If this only needs to be run once per vcpu, why not trigger it
> > > from
> > > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > > 
> > > This can done on the back of a request, saving most of the
> > > overhead
> > > and not requiring any extra field. Essentially, something like
> > > the
> > > (untested) patch below.
> > > 
> > > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-
> > > > emul.c
> > > > index fd167d4f4215..12a40f4b5f0d 100644
> > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > > > *vcpu, u64 val)
> > > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > > >       }
> > > > 
> > > > -     if (val & ARMV8_PMU_PMCR_C)
> > > > +     /*
> > > > +      * Cycle counter needs to reset in case of first vcpu
> > > > load.
> > > > +      */
> > > > +     if (val & ARMV8_PMU_PMCR_C ||
> > > > !kvm_arm_pmu_v3_restored(vcpu))
> > > 
> > > Why? There is no architectural guarantee that a counter resets to
> > > 0
> > > without writing PMCR_EL0.C. And if you want the guest to continue
> > > counting where it left off, resetting the counter is at best
> > > counter-productive.
> > 
> > Without this we would not be resetting PMU which is required for
> > creating host perf events. With the patch that you suggested we are
> > restoring PMCR_EL0 properly but still missing recreation of host
> > perf
> > events.
> 
> How? The request that gets set on the first vcpu run will call
> kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
> kvm_pmu_create_perf_event(). What are we missing?
> 

I found out what I was missing. I was working with an older kernel
which was missing this upstream patch:

https://lore.kernel.org/lkml/20200124142535.29386-3-eric.auger@redhat.com/

> > And without host perf events, guest would still zeros after live
> > migration. In my opinion we have two ways to fix it. We can fix it
> > inside the kernel or let userspace/VMM set those bits before
> > restarting the guest on the destination machine. What do you think?
> 
> I think either you're missing my point above, or I'm completely
> missing yours. And I still don't understand why you want to zero the
> counters that you have just restored. How does that help?
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-07 18:34         ` Jain, Jinank
  0 siblings, 0 replies; 26+ messages in thread
From: Jain, Jinank @ 2021-06-07 18:34 UTC (permalink / raw)
  To: maz
  Cc: james.morse, kvmarm, suzuki.poulose, linux-kernel,
	alexandru.elisei, linux-arm-kernel, will, catalin.marinas,
	Graf (AWS),
	Alexander

Hi Marc.

On Mon, 2021-06-07 at 17:35 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> On Mon, 07 Jun 2021 17:05:01 +0100,
> "Jain, Jinank" <jinankj@amazon.de> wrote:
> > On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> > > Hi Jinank,
> > > 
> > > On Thu, 03 Jun 2021 12:05:54 +0100,
> > > Jinank Jain <jinankj@amazon.de> wrote:
> > > > Currently if a guest is live-migrated while it is actively
> > > > using
> > > > perf
> > > > counters, then after live-migrate it will notice that all
> > > > counters
> > > > would
> > > > suddenly start reporting 0s. This is due to the fact we are not
> > > > re-creating the relevant perf events inside the kernel.
> > > > 
> > > > Usually on live-migration guest state is restored using
> > > > KVM_SET_ONE_REG
> > > > ioctl interface, which simply restores the value of PMU
> > > > registers
> > > > values but does not re-program the perf events so that the
> > > > guest
> > > > can seamlessly
> > > > use these counters even after live-migration like it was doing
> > > > before
> > > > live-migration.
> > > > 
> > > > Instead there are two completely different code path between
> > > > guest
> > > > accessing PMU registers and VMM restoring counters on
> > > > live-migration.
> > > > 
> > > > In case of KVM_SET_ONE_REG:
> > > > 
> > > > kvm_arm_set_reg()
> > > > ...... kvm_arm_sys_reg_set_reg()
> > > > ........... reg_from_user()
> > > > 
> > > > but in case when guest tries to access these counters:
> > > > 
> > > > handle_exit()
> > > > ..... kvm_handle_sys_reg()
> > > > ..........perform_access()
> > > > ...............access_pmu_evcntr()
> > > > ...................kvm_pmu_set_counter_value()
> > > > .......................kvm_pmu_create_perf_event()
> > > > 
> > > > The drawback of using the KVM_SET_ONE_REG interface is that the
> > > > host pmu
> > > > events which were registered for the source instance and not
> > > > present for
> > > > the destination instance.
> > > 
> > > I can't parse this sentence. Do you mean "are not present"?
> > > 
> > > > Thus passively restoring PMCR_EL0 using
> > > > KVM_SET_ONE_REG interface would not create the necessary host
> > > > pmu
> > > > events
> > > > which are crucial for seamless guest experience across live
> > > > migration.
> > > > 
> > > > In ordet to fix the situation, on first vcpu load we should
> > > > restore
> > > > PMCR_EL0 in the same exact way like the guest was trying to
> > > > access
> > > > these counters. And then we will also recreate the relevant
> > > > host
> > > > pmu
> > > > events.
> > > > 
> > > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > Cc: James Morse <james.morse@arm.com>
> > > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Will Deacon <will@kernel.org>
> > > > ---
> > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > >  arch/arm64/kvm/arm.c              |  1 +
> > > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > > >  include/kvm/arm_pmu.h             |  3 +++
> > > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > b/arch/arm64/include/asm/kvm_host.h
> > > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -745,6 +745,7 @@ static inline int
> > > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > > --- a/arch/arm64/kvm/arm.c
> > > > +++ b/arch/arm64/kvm/arm.c
> > > > @@ -408,6 +408,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(vcpu);
> > > 
> > > If this only needs to be run once per vcpu, why not trigger it
> > > from
> > > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > > 
> > > This can done on the back of a request, saving most of the
> > > overhead
> > > and not requiring any extra field. Essentially, something like
> > > the
> > > (untested) patch below.
> > > 
> > > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-
> > > > emul.c
> > > > index fd167d4f4215..12a40f4b5f0d 100644
> > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > > > *vcpu, u64 val)
> > > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > > >       }
> > > > 
> > > > -     if (val & ARMV8_PMU_PMCR_C)
> > > > +     /*
> > > > +      * Cycle counter needs to reset in case of first vcpu
> > > > load.
> > > > +      */
> > > > +     if (val & ARMV8_PMU_PMCR_C ||
> > > > !kvm_arm_pmu_v3_restored(vcpu))
> > > 
> > > Why? There is no architectural guarantee that a counter resets to
> > > 0
> > > without writing PMCR_EL0.C. And if you want the guest to continue
> > > counting where it left off, resetting the counter is at best
> > > counter-productive.
> > 
> > Without this we would not be resetting PMU which is required for
> > creating host perf events. With the patch that you suggested we are
> > restoring PMCR_EL0 properly but still missing recreation of host
> > perf
> > events.
> 
> How? The request that gets set on the first vcpu run will call
> kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
> kvm_pmu_create_perf_event(). What are we missing?
> 

I found out what I was missing. I was working with an older kernel
which was missing this upstream patch:

https://lore.kernel.org/lkml/20200124142535.29386-3-eric.auger@redhat.com/

> > And without host perf events, guest would still zeros after live
> > migration. In my opinion we have two ways to fix it. We can fix it
> > inside the kernel or let userspace/VMM set those bits before
> > restarting the guest on the destination machine. What do you think?
> 
> I think either you're missing my point above, or I'm completely
> missing yours. And I still don't understand why you want to zero the
> counters that you have just restored. How does that help?
> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2] KVM: arm64: Properly restore PMU state during live-migration
  2021-06-03 11:05 ` Jinank Jain
  (?)
@ 2021-06-07 18:58   ` Jinank Jain
  -1 siblings, 0 replies; 26+ messages in thread
From: Jinank Jain @ 2021-06-07 18:58 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, linux-kernel
  Cc: Jinank Jain, Marc Zyngier, Alexander Graf, James Morse,
	Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon

Currently if a guest is live-migrated while it is actively using perf
counters, then after live-migrate it will notice that all counters would
suddenly start reporting 0s. This is due to the fact we are not
re-creating the relevant perf events inside the kernel.

Usually on live-migration guest state is restored using KVM_SET_ONE_REG
ioctl interface, which simply restores the value of PMU registers
values but does not re-program the perf events so that the guest can seamlessly
use these counters even after live-migration like it was doing before
live-migration.

Instead there are two completely different code path between guest
accessing PMU registers and VMM restoring counters on
live-migration.

In case of KVM_SET_ONE_REG:

kvm_arm_set_reg()
...... kvm_arm_sys_reg_set_reg()
........... reg_from_user()

but in case when guest tries to access these counters:

handle_exit()
..... kvm_handle_sys_reg()
..........perform_access()
...............access_pmu_evcntr()
...................kvm_pmu_set_counter_value()
.......................kvm_pmu_create_perf_event()

The drawback of using the KVM_SET_ONE_REG interface is that the host pmu
events which were registered for the source instance are not present for
the destination instance. Thus, passively restoring PMCR_EL0 using
KVM_SET_ONE_REG interface would not create the necessary host pmu events
which are crucial for seamless guest experience across live migration.

In ordet to fix the situation, on first vcpu load we should restore
PMCR_EL0 in the same exact way like the guest was trying to access
these counters. And then we will also recreate the relevant host pmu
events.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Jinank Jain <jinankj@amazon.de>
Cc: Alexander Graf (AWS) <graf@amazon.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/arm.c              | 4 ++++
 arch/arm64/kvm/pmu-emul.c         | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cd7d5c8c4bc..6336b4309114 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -46,6 +46,7 @@
 #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
 #define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
 #define KVM_REQ_RELOAD_GICv4	KVM_ARCH_REQ(4)
+#define KVM_REQ_RELOAD_PMU	KVM_ARCH_REQ(5)
 
 #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
 				     KVM_DIRTY_LOG_INITIALLY_SET)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e720148232a0..facf4d41d32a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -689,6 +689,10 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 			vgic_v4_load(vcpu);
 			preempt_enable();
 		}
+
+		if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
+			kvm_pmu_handle_pmcr(vcpu,
+					    __vcpu_sys_reg(vcpu, PMCR_EL0));
 	}
 }
 
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fd167d4f4215..a0bbb7111f57 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -850,6 +850,9 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 		   return -EINVAL;
 	}
 
+	/* One-off reload of the PMU on first run */
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+
 	return 0;
 }
 
-- 
2.31.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v2] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-07 18:58   ` Jinank Jain
  0 siblings, 0 replies; 26+ messages in thread
From: Jinank Jain @ 2021-06-07 18:58 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, linux-kernel
  Cc: Alexander Graf, Will Deacon, Marc Zyngier, Catalin Marinas, Jinank Jain

Currently if a guest is live-migrated while it is actively using perf
counters, then after live-migrate it will notice that all counters would
suddenly start reporting 0s. This is due to the fact we are not
re-creating the relevant perf events inside the kernel.

Usually on live-migration guest state is restored using KVM_SET_ONE_REG
ioctl interface, which simply restores the value of PMU registers
values but does not re-program the perf events so that the guest can seamlessly
use these counters even after live-migration like it was doing before
live-migration.

Instead there are two completely different code path between guest
accessing PMU registers and VMM restoring counters on
live-migration.

In case of KVM_SET_ONE_REG:

kvm_arm_set_reg()
...... kvm_arm_sys_reg_set_reg()
........... reg_from_user()

but in case when guest tries to access these counters:

handle_exit()
..... kvm_handle_sys_reg()
..........perform_access()
...............access_pmu_evcntr()
...................kvm_pmu_set_counter_value()
.......................kvm_pmu_create_perf_event()

The drawback of using the KVM_SET_ONE_REG interface is that the host pmu
events which were registered for the source instance are not present for
the destination instance. Thus, passively restoring PMCR_EL0 using
KVM_SET_ONE_REG interface would not create the necessary host pmu events
which are crucial for seamless guest experience across live migration.

In ordet to fix the situation, on first vcpu load we should restore
PMCR_EL0 in the same exact way like the guest was trying to access
these counters. And then we will also recreate the relevant host pmu
events.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Jinank Jain <jinankj@amazon.de>
Cc: Alexander Graf (AWS) <graf@amazon.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/arm.c              | 4 ++++
 arch/arm64/kvm/pmu-emul.c         | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cd7d5c8c4bc..6336b4309114 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -46,6 +46,7 @@
 #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
 #define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
 #define KVM_REQ_RELOAD_GICv4	KVM_ARCH_REQ(4)
+#define KVM_REQ_RELOAD_PMU	KVM_ARCH_REQ(5)
 
 #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
 				     KVM_DIRTY_LOG_INITIALLY_SET)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e720148232a0..facf4d41d32a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -689,6 +689,10 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 			vgic_v4_load(vcpu);
 			preempt_enable();
 		}
+
+		if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
+			kvm_pmu_handle_pmcr(vcpu,
+					    __vcpu_sys_reg(vcpu, PMCR_EL0));
 	}
 }
 
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fd167d4f4215..a0bbb7111f57 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -850,6 +850,9 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 		   return -EINVAL;
 	}
 
+	/* One-off reload of the PMU on first run */
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+
 	return 0;
 }
 
-- 
2.31.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v2] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-07 18:58   ` Jinank Jain
  0 siblings, 0 replies; 26+ messages in thread
From: Jinank Jain @ 2021-06-07 18:58 UTC (permalink / raw)
  To: linux-arm-kernel, kvmarm, linux-kernel
  Cc: Jinank Jain, Marc Zyngier, Alexander Graf, James Morse,
	Alexandru Elisei, Suzuki K Poulose, Catalin Marinas, Will Deacon

Currently if a guest is live-migrated while it is actively using perf
counters, then after live-migrate it will notice that all counters would
suddenly start reporting 0s. This is due to the fact we are not
re-creating the relevant perf events inside the kernel.

Usually on live-migration guest state is restored using KVM_SET_ONE_REG
ioctl interface, which simply restores the value of PMU registers
values but does not re-program the perf events so that the guest can seamlessly
use these counters even after live-migration like it was doing before
live-migration.

Instead there are two completely different code path between guest
accessing PMU registers and VMM restoring counters on
live-migration.

In case of KVM_SET_ONE_REG:

kvm_arm_set_reg()
...... kvm_arm_sys_reg_set_reg()
........... reg_from_user()

but in case when guest tries to access these counters:

handle_exit()
..... kvm_handle_sys_reg()
..........perform_access()
...............access_pmu_evcntr()
...................kvm_pmu_set_counter_value()
.......................kvm_pmu_create_perf_event()

The drawback of using the KVM_SET_ONE_REG interface is that the host pmu
events which were registered for the source instance are not present for
the destination instance. Thus, passively restoring PMCR_EL0 using
KVM_SET_ONE_REG interface would not create the necessary host pmu events
which are crucial for seamless guest experience across live migration.

In ordet to fix the situation, on first vcpu load we should restore
PMCR_EL0 in the same exact way like the guest was trying to access
these counters. And then we will also recreate the relevant host pmu
events.

Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Jinank Jain <jinankj@amazon.de>
Cc: Alexander Graf (AWS) <graf@amazon.de>
Cc: Marc Zyngier <maz@kernel.org>
Cc: James Morse <james.morse@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h | 1 +
 arch/arm64/kvm/arm.c              | 4 ++++
 arch/arm64/kvm/pmu-emul.c         | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 7cd7d5c8c4bc..6336b4309114 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -46,6 +46,7 @@
 #define KVM_REQ_VCPU_RESET	KVM_ARCH_REQ(2)
 #define KVM_REQ_RECORD_STEAL	KVM_ARCH_REQ(3)
 #define KVM_REQ_RELOAD_GICv4	KVM_ARCH_REQ(4)
+#define KVM_REQ_RELOAD_PMU	KVM_ARCH_REQ(5)
 
 #define KVM_DIRTY_LOG_MANUAL_CAPS   (KVM_DIRTY_LOG_MANUAL_PROTECT_ENABLE | \
 				     KVM_DIRTY_LOG_INITIALLY_SET)
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index e720148232a0..facf4d41d32a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -689,6 +689,10 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
 			vgic_v4_load(vcpu);
 			preempt_enable();
 		}
+
+		if (kvm_check_request(KVM_REQ_RELOAD_PMU, vcpu))
+			kvm_pmu_handle_pmcr(vcpu,
+					    __vcpu_sys_reg(vcpu, PMCR_EL0));
 	}
 }
 
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index fd167d4f4215..a0bbb7111f57 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -850,6 +850,9 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 		   return -EINVAL;
 	}
 
+	/* One-off reload of the PMU on first run */
+	kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+
 	return 0;
 }
 
-- 
2.31.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
  2021-06-07 18:34         ` Jain, Jinank
  (?)
@ 2021-06-08  8:18           ` Marc Zyngier
  -1 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2021-06-08  8:18 UTC (permalink / raw)
  To: Jain, Jinank
  Cc: james.morse, kvmarm, suzuki.poulose, linux-kernel,
	alexandru.elisei, linux-arm-kernel, will, catalin.marinas,
	Graf (AWS),
	Alexander

On Mon, 07 Jun 2021 19:34:08 +0100,
"Jain, Jinank" <jinankj@amazon.de> wrote:
> 
> Hi Marc.
> 
> On Mon, 2021-06-07 at 17:35 +0100, Marc Zyngier wrote:
> > CAUTION: This email originated from outside of the organization. Do
> > not click links or open attachments unless you can confirm the sender
> > and know the content is safe.
> > 
> > 
> > 
> > On Mon, 07 Jun 2021 17:05:01 +0100,
> > "Jain, Jinank" <jinankj@amazon.de> wrote:
> > > On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> > > > Hi Jinank,
> > > > 
> > > > On Thu, 03 Jun 2021 12:05:54 +0100,
> > > > Jinank Jain <jinankj@amazon.de> wrote:
> > > > > Currently if a guest is live-migrated while it is actively
> > > > > using
> > > > > perf
> > > > > counters, then after live-migrate it will notice that all
> > > > > counters
> > > > > would
> > > > > suddenly start reporting 0s. This is due to the fact we are not
> > > > > re-creating the relevant perf events inside the kernel.
> > > > > 
> > > > > Usually on live-migration guest state is restored using
> > > > > KVM_SET_ONE_REG
> > > > > ioctl interface, which simply restores the value of PMU
> > > > > registers
> > > > > values but does not re-program the perf events so that the
> > > > > guest
> > > > > can seamlessly
> > > > > use these counters even after live-migration like it was doing
> > > > > before
> > > > > live-migration.
> > > > > 
> > > > > Instead there are two completely different code path between
> > > > > guest
> > > > > accessing PMU registers and VMM restoring counters on
> > > > > live-migration.
> > > > > 
> > > > > In case of KVM_SET_ONE_REG:
> > > > > 
> > > > > kvm_arm_set_reg()
> > > > > ...... kvm_arm_sys_reg_set_reg()
> > > > > ........... reg_from_user()
> > > > > 
> > > > > but in case when guest tries to access these counters:
> > > > > 
> > > > > handle_exit()
> > > > > ..... kvm_handle_sys_reg()
> > > > > ..........perform_access()
> > > > > ...............access_pmu_evcntr()
> > > > > ...................kvm_pmu_set_counter_value()
> > > > > .......................kvm_pmu_create_perf_event()
> > > > > 
> > > > > The drawback of using the KVM_SET_ONE_REG interface is that the
> > > > > host pmu
> > > > > events which were registered for the source instance and not
> > > > > present for
> > > > > the destination instance.
> > > > 
> > > > I can't parse this sentence. Do you mean "are not present"?
> > > > 
> > > > > Thus passively restoring PMCR_EL0 using
> > > > > KVM_SET_ONE_REG interface would not create the necessary host
> > > > > pmu
> > > > > events
> > > > > which are crucial for seamless guest experience across live
> > > > > migration.
> > > > > 
> > > > > In ordet to fix the situation, on first vcpu load we should
> > > > > restore
> > > > > PMCR_EL0 in the same exact way like the guest was trying to
> > > > > access
> > > > > these counters. And then we will also recreate the relevant
> > > > > host
> > > > > pmu
> > > > > events.
> > > > > 
> > > > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > > Cc: James Morse <james.morse@arm.com>
> > > > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Cc: Will Deacon <will@kernel.org>
> > > > > ---
> > > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > > >  arch/arm64/kvm/arm.c              |  1 +
> > > > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > > > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > > > >  include/kvm/arm_pmu.h             |  3 +++
> > > > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > > b/arch/arm64/include/asm/kvm_host.h
> > > > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > @@ -745,6 +745,7 @@ static inline int
> > > > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > > > --- a/arch/arm64/kvm/arm.c
> > > > > +++ b/arch/arm64/kvm/arm.c
> > > > > @@ -408,6 +408,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(vcpu);
> > > > 
> > > > If this only needs to be run once per vcpu, why not trigger it
> > > > from
> > > > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > > > 
> > > > This can done on the back of a request, saving most of the
> > > > overhead
> > > > and not requiring any extra field. Essentially, something like
> > > > the
> > > > (untested) patch below.
> > > > 
> > > > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > > > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > > > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-
> > > > > emul.c
> > > > > index fd167d4f4215..12a40f4b5f0d 100644
> > > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > > > > *vcpu, u64 val)
> > > > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > > > >       }
> > > > > 
> > > > > -     if (val & ARMV8_PMU_PMCR_C)
> > > > > +     /*
> > > > > +      * Cycle counter needs to reset in case of first vcpu
> > > > > load.
> > > > > +      */
> > > > > +     if (val & ARMV8_PMU_PMCR_C ||
> > > > > !kvm_arm_pmu_v3_restored(vcpu))
> > > > 
> > > > Why? There is no architectural guarantee that a counter resets to
> > > > 0
> > > > without writing PMCR_EL0.C. And if you want the guest to continue
> > > > counting where it left off, resetting the counter is at best
> > > > counter-productive.
> > > 
> > > Without this we would not be resetting PMU which is required for
> > > creating host perf events. With the patch that you suggested we are
> > > restoring PMCR_EL0 properly but still missing recreation of host
> > > perf
> > > events.
> > 
> > How? The request that gets set on the first vcpu run will call
> > kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
> > kvm_pmu_create_perf_event(). What are we missing?
> > 
> 
> I found out what I was missing. I was working with an older kernel
> which was missing this upstream patch:
> 
> https://lore.kernel.org/lkml/20200124142535.29386-3-eric.auger@redhat.com/

:-(

Please test whatever you send with an upstream kernel. Actually,
please *develop* on an upstream kernel. This will avoid this kind of
discussion where we talk past each other, and make it plain that your
production kernel is lacking all sorts of fixes.

Now, can you please state whether or not this patch fixes it for you
*on an upstream kernel*? I have no interest in results from a
production kernel.

	M.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-08  8:18           ` Marc Zyngier
  0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2021-06-08  8:18 UTC (permalink / raw)
  To: Jain, Jinank
  Cc: Graf (AWS),
	Alexander, will, linux-kernel, catalin.marinas, kvmarm,
	linux-arm-kernel

On Mon, 07 Jun 2021 19:34:08 +0100,
"Jain, Jinank" <jinankj@amazon.de> wrote:
> 
> Hi Marc.
> 
> On Mon, 2021-06-07 at 17:35 +0100, Marc Zyngier wrote:
> > CAUTION: This email originated from outside of the organization. Do
> > not click links or open attachments unless you can confirm the sender
> > and know the content is safe.
> > 
> > 
> > 
> > On Mon, 07 Jun 2021 17:05:01 +0100,
> > "Jain, Jinank" <jinankj@amazon.de> wrote:
> > > On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> > > > Hi Jinank,
> > > > 
> > > > On Thu, 03 Jun 2021 12:05:54 +0100,
> > > > Jinank Jain <jinankj@amazon.de> wrote:
> > > > > Currently if a guest is live-migrated while it is actively
> > > > > using
> > > > > perf
> > > > > counters, then after live-migrate it will notice that all
> > > > > counters
> > > > > would
> > > > > suddenly start reporting 0s. This is due to the fact we are not
> > > > > re-creating the relevant perf events inside the kernel.
> > > > > 
> > > > > Usually on live-migration guest state is restored using
> > > > > KVM_SET_ONE_REG
> > > > > ioctl interface, which simply restores the value of PMU
> > > > > registers
> > > > > values but does not re-program the perf events so that the
> > > > > guest
> > > > > can seamlessly
> > > > > use these counters even after live-migration like it was doing
> > > > > before
> > > > > live-migration.
> > > > > 
> > > > > Instead there are two completely different code path between
> > > > > guest
> > > > > accessing PMU registers and VMM restoring counters on
> > > > > live-migration.
> > > > > 
> > > > > In case of KVM_SET_ONE_REG:
> > > > > 
> > > > > kvm_arm_set_reg()
> > > > > ...... kvm_arm_sys_reg_set_reg()
> > > > > ........... reg_from_user()
> > > > > 
> > > > > but in case when guest tries to access these counters:
> > > > > 
> > > > > handle_exit()
> > > > > ..... kvm_handle_sys_reg()
> > > > > ..........perform_access()
> > > > > ...............access_pmu_evcntr()
> > > > > ...................kvm_pmu_set_counter_value()
> > > > > .......................kvm_pmu_create_perf_event()
> > > > > 
> > > > > The drawback of using the KVM_SET_ONE_REG interface is that the
> > > > > host pmu
> > > > > events which were registered for the source instance and not
> > > > > present for
> > > > > the destination instance.
> > > > 
> > > > I can't parse this sentence. Do you mean "are not present"?
> > > > 
> > > > > Thus passively restoring PMCR_EL0 using
> > > > > KVM_SET_ONE_REG interface would not create the necessary host
> > > > > pmu
> > > > > events
> > > > > which are crucial for seamless guest experience across live
> > > > > migration.
> > > > > 
> > > > > In ordet to fix the situation, on first vcpu load we should
> > > > > restore
> > > > > PMCR_EL0 in the same exact way like the guest was trying to
> > > > > access
> > > > > these counters. And then we will also recreate the relevant
> > > > > host
> > > > > pmu
> > > > > events.
> > > > > 
> > > > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > > Cc: James Morse <james.morse@arm.com>
> > > > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Cc: Will Deacon <will@kernel.org>
> > > > > ---
> > > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > > >  arch/arm64/kvm/arm.c              |  1 +
> > > > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > > > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > > > >  include/kvm/arm_pmu.h             |  3 +++
> > > > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > > b/arch/arm64/include/asm/kvm_host.h
> > > > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > @@ -745,6 +745,7 @@ static inline int
> > > > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > > > --- a/arch/arm64/kvm/arm.c
> > > > > +++ b/arch/arm64/kvm/arm.c
> > > > > @@ -408,6 +408,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(vcpu);
> > > > 
> > > > If this only needs to be run once per vcpu, why not trigger it
> > > > from
> > > > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > > > 
> > > > This can done on the back of a request, saving most of the
> > > > overhead
> > > > and not requiring any extra field. Essentially, something like
> > > > the
> > > > (untested) patch below.
> > > > 
> > > > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > > > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > > > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-
> > > > > emul.c
> > > > > index fd167d4f4215..12a40f4b5f0d 100644
> > > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > > > > *vcpu, u64 val)
> > > > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > > > >       }
> > > > > 
> > > > > -     if (val & ARMV8_PMU_PMCR_C)
> > > > > +     /*
> > > > > +      * Cycle counter needs to reset in case of first vcpu
> > > > > load.
> > > > > +      */
> > > > > +     if (val & ARMV8_PMU_PMCR_C ||
> > > > > !kvm_arm_pmu_v3_restored(vcpu))
> > > > 
> > > > Why? There is no architectural guarantee that a counter resets to
> > > > 0
> > > > without writing PMCR_EL0.C. And if you want the guest to continue
> > > > counting where it left off, resetting the counter is at best
> > > > counter-productive.
> > > 
> > > Without this we would not be resetting PMU which is required for
> > > creating host perf events. With the patch that you suggested we are
> > > restoring PMCR_EL0 properly but still missing recreation of host
> > > perf
> > > events.
> > 
> > How? The request that gets set on the first vcpu run will call
> > kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
> > kvm_pmu_create_perf_event(). What are we missing?
> > 
> 
> I found out what I was missing. I was working with an older kernel
> which was missing this upstream patch:
> 
> https://lore.kernel.org/lkml/20200124142535.29386-3-eric.auger@redhat.com/

:-(

Please test whatever you send with an upstream kernel. Actually,
please *develop* on an upstream kernel. This will avoid this kind of
discussion where we talk past each other, and make it plain that your
production kernel is lacking all sorts of fixes.

Now, can you please state whether or not this patch fixes it for you
*on an upstream kernel*? I have no interest in results from a
production kernel.

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-08  8:18           ` Marc Zyngier
  0 siblings, 0 replies; 26+ messages in thread
From: Marc Zyngier @ 2021-06-08  8:18 UTC (permalink / raw)
  To: Jain, Jinank
  Cc: james.morse, kvmarm, suzuki.poulose, linux-kernel,
	alexandru.elisei, linux-arm-kernel, will, catalin.marinas,
	Graf (AWS),
	Alexander

On Mon, 07 Jun 2021 19:34:08 +0100,
"Jain, Jinank" <jinankj@amazon.de> wrote:
> 
> Hi Marc.
> 
> On Mon, 2021-06-07 at 17:35 +0100, Marc Zyngier wrote:
> > CAUTION: This email originated from outside of the organization. Do
> > not click links or open attachments unless you can confirm the sender
> > and know the content is safe.
> > 
> > 
> > 
> > On Mon, 07 Jun 2021 17:05:01 +0100,
> > "Jain, Jinank" <jinankj@amazon.de> wrote:
> > > On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> > > > Hi Jinank,
> > > > 
> > > > On Thu, 03 Jun 2021 12:05:54 +0100,
> > > > Jinank Jain <jinankj@amazon.de> wrote:
> > > > > Currently if a guest is live-migrated while it is actively
> > > > > using
> > > > > perf
> > > > > counters, then after live-migrate it will notice that all
> > > > > counters
> > > > > would
> > > > > suddenly start reporting 0s. This is due to the fact we are not
> > > > > re-creating the relevant perf events inside the kernel.
> > > > > 
> > > > > Usually on live-migration guest state is restored using
> > > > > KVM_SET_ONE_REG
> > > > > ioctl interface, which simply restores the value of PMU
> > > > > registers
> > > > > values but does not re-program the perf events so that the
> > > > > guest
> > > > > can seamlessly
> > > > > use these counters even after live-migration like it was doing
> > > > > before
> > > > > live-migration.
> > > > > 
> > > > > Instead there are two completely different code path between
> > > > > guest
> > > > > accessing PMU registers and VMM restoring counters on
> > > > > live-migration.
> > > > > 
> > > > > In case of KVM_SET_ONE_REG:
> > > > > 
> > > > > kvm_arm_set_reg()
> > > > > ...... kvm_arm_sys_reg_set_reg()
> > > > > ........... reg_from_user()
> > > > > 
> > > > > but in case when guest tries to access these counters:
> > > > > 
> > > > > handle_exit()
> > > > > ..... kvm_handle_sys_reg()
> > > > > ..........perform_access()
> > > > > ...............access_pmu_evcntr()
> > > > > ...................kvm_pmu_set_counter_value()
> > > > > .......................kvm_pmu_create_perf_event()
> > > > > 
> > > > > The drawback of using the KVM_SET_ONE_REG interface is that the
> > > > > host pmu
> > > > > events which were registered for the source instance and not
> > > > > present for
> > > > > the destination instance.
> > > > 
> > > > I can't parse this sentence. Do you mean "are not present"?
> > > > 
> > > > > Thus passively restoring PMCR_EL0 using
> > > > > KVM_SET_ONE_REG interface would not create the necessary host
> > > > > pmu
> > > > > events
> > > > > which are crucial for seamless guest experience across live
> > > > > migration.
> > > > > 
> > > > > In ordet to fix the situation, on first vcpu load we should
> > > > > restore
> > > > > PMCR_EL0 in the same exact way like the guest was trying to
> > > > > access
> > > > > these counters. And then we will also recreate the relevant
> > > > > host
> > > > > pmu
> > > > > events.
> > > > > 
> > > > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > > Cc: James Morse <james.morse@arm.com>
> > > > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Cc: Will Deacon <will@kernel.org>
> > > > > ---
> > > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > > >  arch/arm64/kvm/arm.c              |  1 +
> > > > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > > > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > > > >  include/kvm/arm_pmu.h             |  3 +++
> > > > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > > b/arch/arm64/include/asm/kvm_host.h
> > > > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > @@ -745,6 +745,7 @@ static inline int
> > > > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > > > --- a/arch/arm64/kvm/arm.c
> > > > > +++ b/arch/arm64/kvm/arm.c
> > > > > @@ -408,6 +408,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(vcpu);
> > > > 
> > > > If this only needs to be run once per vcpu, why not trigger it
> > > > from
> > > > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > > > 
> > > > This can done on the back of a request, saving most of the
> > > > overhead
> > > > and not requiring any extra field. Essentially, something like
> > > > the
> > > > (untested) patch below.
> > > > 
> > > > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > > > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > > > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-
> > > > > emul.c
> > > > > index fd167d4f4215..12a40f4b5f0d 100644
> > > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu
> > > > > *vcpu, u64 val)
> > > > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > > > >       }
> > > > > 
> > > > > -     if (val & ARMV8_PMU_PMCR_C)
> > > > > +     /*
> > > > > +      * Cycle counter needs to reset in case of first vcpu
> > > > > load.
> > > > > +      */
> > > > > +     if (val & ARMV8_PMU_PMCR_C ||
> > > > > !kvm_arm_pmu_v3_restored(vcpu))
> > > > 
> > > > Why? There is no architectural guarantee that a counter resets to
> > > > 0
> > > > without writing PMCR_EL0.C. And if you want the guest to continue
> > > > counting where it left off, resetting the counter is at best
> > > > counter-productive.
> > > 
> > > Without this we would not be resetting PMU which is required for
> > > creating host perf events. With the patch that you suggested we are
> > > restoring PMCR_EL0 properly but still missing recreation of host
> > > perf
> > > events.
> > 
> > How? The request that gets set on the first vcpu run will call
> > kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
> > kvm_pmu_create_perf_event(). What are we missing?
> > 
> 
> I found out what I was missing. I was working with an older kernel
> which was missing this upstream patch:
> 
> https://lore.kernel.org/lkml/20200124142535.29386-3-eric.auger@redhat.com/

:-(

Please test whatever you send with an upstream kernel. Actually,
please *develop* on an upstream kernel. This will avoid this kind of
discussion where we talk past each other, and make it plain that your
production kernel is lacking all sorts of fixes.

Now, can you please state whether or not this patch fixes it for you
*on an upstream kernel*? I have no interest in results from a
production kernel.

	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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
  2021-06-08  8:18           ` Marc Zyngier
  (?)
@ 2021-06-08  8:24             ` Jain, Jinank
  -1 siblings, 0 replies; 26+ messages in thread
From: Jain, Jinank @ 2021-06-08  8:24 UTC (permalink / raw)
  To: maz
  Cc: james.morse, kvmarm, suzuki.poulose, linux-kernel,
	alexandru.elisei, linux-arm-kernel, will, catalin.marinas,
	Graf (AWS),
	Alexander

On Tue, 2021-06-08 at 09:18 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> On Mon, 07 Jun 2021 19:34:08 +0100,
> "Jain, Jinank" <jinankj@amazon.de> wrote:
> > Hi Marc.
> > 
> > On Mon, 2021-06-07 at 17:35 +0100, Marc Zyngier wrote:
> > > CAUTION: This email originated from outside of the organization.
> > > Do
> > > not click links or open attachments unless you can confirm the
> > > sender
> > > and know the content is safe.
> > > 
> > > 
> > > 
> > > On Mon, 07 Jun 2021 17:05:01 +0100,
> > > "Jain, Jinank" <jinankj@amazon.de> wrote:
> > > > On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> > > > > Hi Jinank,
> > > > > 
> > > > > On Thu, 03 Jun 2021 12:05:54 +0100,
> > > > > Jinank Jain <jinankj@amazon.de> wrote:
> > > > > > Currently if a guest is live-migrated while it is actively
> > > > > > using
> > > > > > perf
> > > > > > counters, then after live-migrate it will notice that all
> > > > > > counters
> > > > > > would
> > > > > > suddenly start reporting 0s. This is due to the fact we are
> > > > > > not
> > > > > > re-creating the relevant perf events inside the kernel.
> > > > > > 
> > > > > > Usually on live-migration guest state is restored using
> > > > > > KVM_SET_ONE_REG
> > > > > > ioctl interface, which simply restores the value of PMU
> > > > > > registers
> > > > > > values but does not re-program the perf events so that the
> > > > > > guest
> > > > > > can seamlessly
> > > > > > use these counters even after live-migration like it was
> > > > > > doing
> > > > > > before
> > > > > > live-migration.
> > > > > > 
> > > > > > Instead there are two completely different code path
> > > > > > between
> > > > > > guest
> > > > > > accessing PMU registers and VMM restoring counters on
> > > > > > live-migration.
> > > > > > 
> > > > > > In case of KVM_SET_ONE_REG:
> > > > > > 
> > > > > > kvm_arm_set_reg()
> > > > > > ...... kvm_arm_sys_reg_set_reg()
> > > > > > ........... reg_from_user()
> > > > > > 
> > > > > > but in case when guest tries to access these counters:
> > > > > > 
> > > > > > handle_exit()
> > > > > > ..... kvm_handle_sys_reg()
> > > > > > ..........perform_access()
> > > > > > ...............access_pmu_evcntr()
> > > > > > ...................kvm_pmu_set_counter_value()
> > > > > > .......................kvm_pmu_create_perf_event()
> > > > > > 
> > > > > > The drawback of using the KVM_SET_ONE_REG interface is that
> > > > > > the
> > > > > > host pmu
> > > > > > events which were registered for the source instance and
> > > > > > not
> > > > > > present for
> > > > > > the destination instance.
> > > > > 
> > > > > I can't parse this sentence. Do you mean "are not present"?
> > > > > 
> > > > > > Thus passively restoring PMCR_EL0 using
> > > > > > KVM_SET_ONE_REG interface would not create the necessary
> > > > > > host
> > > > > > pmu
> > > > > > events
> > > > > > which are crucial for seamless guest experience across live
> > > > > > migration.
> > > > > > 
> > > > > > In ordet to fix the situation, on first vcpu load we should
> > > > > > restore
> > > > > > PMCR_EL0 in the same exact way like the guest was trying to
> > > > > > access
> > > > > > these counters. And then we will also recreate the relevant
> > > > > > host
> > > > > > pmu
> > > > > > events.
> > > > > > 
> > > > > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > > > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > > > Cc: James Morse <james.morse@arm.com>
> > > > > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > > Cc: Will Deacon <will@kernel.org>
> > > > > > ---
> > > > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > > > >  arch/arm64/kvm/arm.c              |  1 +
> > > > > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > > > > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > > > > >  include/kvm/arm_pmu.h             |  3 +++
> > > > > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > > > b/arch/arm64/include/asm/kvm_host.h
> > > > > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > > @@ -745,6 +745,7 @@ static inline int
> > > > > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > > > > --- a/arch/arm64/kvm/arm.c
> > > > > > +++ b/arch/arm64/kvm/arm.c
> > > > > > @@ -408,6 +408,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(vcpu);
> > > > > 
> > > > > If this only needs to be run once per vcpu, why not trigger
> > > > > it
> > > > > from
> > > > > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > > > > 
> > > > > This can done on the back of a request, saving most of the
> > > > > overhead
> > > > > and not requiring any extra field. Essentially, something
> > > > > like
> > > > > the
> > > > > (untested) patch below.
> > > > > 
> > > > > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > > > > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > > > > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > > > > diff --git a/arch/arm64/kvm/pmu-emul.c
> > > > > > b/arch/arm64/kvm/pmu-
> > > > > > emul.c
> > > > > > index fd167d4f4215..12a40f4b5f0d 100644
> > > > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct
> > > > > > kvm_vcpu
> > > > > > *vcpu, u64 val)
> > > > > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > > > > >       }
> > > > > > 
> > > > > > -     if (val & ARMV8_PMU_PMCR_C)
> > > > > > +     /*
> > > > > > +      * Cycle counter needs to reset in case of first vcpu
> > > > > > load.
> > > > > > +      */
> > > > > > +     if (val & ARMV8_PMU_PMCR_C ||
> > > > > > !kvm_arm_pmu_v3_restored(vcpu))
> > > > > 
> > > > > Why? There is no architectural guarantee that a counter
> > > > > resets to
> > > > > 0
> > > > > without writing PMCR_EL0.C. And if you want the guest to
> > > > > continue
> > > > > counting where it left off, resetting the counter is at best
> > > > > counter-productive.
> > > > 
> > > > Without this we would not be resetting PMU which is required
> > > > for
> > > > creating host perf events. With the patch that you suggested we
> > > > are
> > > > restoring PMCR_EL0 properly but still missing recreation of
> > > > host
> > > > perf
> > > > events.
> > > 
> > > How? The request that gets set on the first vcpu run will call
> > > kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
> > > kvm_pmu_create_perf_event(). What are we missing?
> > > 
> > 
> > I found out what I was missing. I was working with an older kernel
> > which was missing this upstream patch:
> > 
> > https://lore.kernel.org/lkml/20200124142535.29386-3-eric.auger@redhat.com/
> 
> :-(
> 
> Please test whatever you send with an upstream kernel. Actually,
> please *develop* on an upstream kernel. This will avoid this kind of
> discussion where we talk past each other, and make it plain that your
> production kernel is lacking all sorts of fixes.
> 
> Now, can you please state whether or not this patch fixes it for you
> *on an upstream kernel*? I have no interest in results from a
> production kernel.
> 
>         M.
> 

Really sorry for the noise and I can confirm that your suggested patch
fixes the problem for the upstream kernel i.e., if I live migrate a
guest which is actively using perf events then the guest can continue
using them even after live migration without interruption.

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



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-08  8:24             ` Jain, Jinank
  0 siblings, 0 replies; 26+ messages in thread
From: Jain, Jinank @ 2021-06-08  8:24 UTC (permalink / raw)
  To: maz
  Cc: Graf (AWS),
	Alexander, will, linux-kernel, catalin.marinas, kvmarm,
	linux-arm-kernel

On Tue, 2021-06-08 at 09:18 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> On Mon, 07 Jun 2021 19:34:08 +0100,
> "Jain, Jinank" <jinankj@amazon.de> wrote:
> > Hi Marc.
> > 
> > On Mon, 2021-06-07 at 17:35 +0100, Marc Zyngier wrote:
> > > CAUTION: This email originated from outside of the organization.
> > > Do
> > > not click links or open attachments unless you can confirm the
> > > sender
> > > and know the content is safe.
> > > 
> > > 
> > > 
> > > On Mon, 07 Jun 2021 17:05:01 +0100,
> > > "Jain, Jinank" <jinankj@amazon.de> wrote:
> > > > On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> > > > > Hi Jinank,
> > > > > 
> > > > > On Thu, 03 Jun 2021 12:05:54 +0100,
> > > > > Jinank Jain <jinankj@amazon.de> wrote:
> > > > > > Currently if a guest is live-migrated while it is actively
> > > > > > using
> > > > > > perf
> > > > > > counters, then after live-migrate it will notice that all
> > > > > > counters
> > > > > > would
> > > > > > suddenly start reporting 0s. This is due to the fact we are
> > > > > > not
> > > > > > re-creating the relevant perf events inside the kernel.
> > > > > > 
> > > > > > Usually on live-migration guest state is restored using
> > > > > > KVM_SET_ONE_REG
> > > > > > ioctl interface, which simply restores the value of PMU
> > > > > > registers
> > > > > > values but does not re-program the perf events so that the
> > > > > > guest
> > > > > > can seamlessly
> > > > > > use these counters even after live-migration like it was
> > > > > > doing
> > > > > > before
> > > > > > live-migration.
> > > > > > 
> > > > > > Instead there are two completely different code path
> > > > > > between
> > > > > > guest
> > > > > > accessing PMU registers and VMM restoring counters on
> > > > > > live-migration.
> > > > > > 
> > > > > > In case of KVM_SET_ONE_REG:
> > > > > > 
> > > > > > kvm_arm_set_reg()
> > > > > > ...... kvm_arm_sys_reg_set_reg()
> > > > > > ........... reg_from_user()
> > > > > > 
> > > > > > but in case when guest tries to access these counters:
> > > > > > 
> > > > > > handle_exit()
> > > > > > ..... kvm_handle_sys_reg()
> > > > > > ..........perform_access()
> > > > > > ...............access_pmu_evcntr()
> > > > > > ...................kvm_pmu_set_counter_value()
> > > > > > .......................kvm_pmu_create_perf_event()
> > > > > > 
> > > > > > The drawback of using the KVM_SET_ONE_REG interface is that
> > > > > > the
> > > > > > host pmu
> > > > > > events which were registered for the source instance and
> > > > > > not
> > > > > > present for
> > > > > > the destination instance.
> > > > > 
> > > > > I can't parse this sentence. Do you mean "are not present"?
> > > > > 
> > > > > > Thus passively restoring PMCR_EL0 using
> > > > > > KVM_SET_ONE_REG interface would not create the necessary
> > > > > > host
> > > > > > pmu
> > > > > > events
> > > > > > which are crucial for seamless guest experience across live
> > > > > > migration.
> > > > > > 
> > > > > > In ordet to fix the situation, on first vcpu load we should
> > > > > > restore
> > > > > > PMCR_EL0 in the same exact way like the guest was trying to
> > > > > > access
> > > > > > these counters. And then we will also recreate the relevant
> > > > > > host
> > > > > > pmu
> > > > > > events.
> > > > > > 
> > > > > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > > > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > > > Cc: James Morse <james.morse@arm.com>
> > > > > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > > Cc: Will Deacon <will@kernel.org>
> > > > > > ---
> > > > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > > > >  arch/arm64/kvm/arm.c              |  1 +
> > > > > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > > > > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > > > > >  include/kvm/arm_pmu.h             |  3 +++
> > > > > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > > > b/arch/arm64/include/asm/kvm_host.h
> > > > > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > > @@ -745,6 +745,7 @@ static inline int
> > > > > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > > > > --- a/arch/arm64/kvm/arm.c
> > > > > > +++ b/arch/arm64/kvm/arm.c
> > > > > > @@ -408,6 +408,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(vcpu);
> > > > > 
> > > > > If this only needs to be run once per vcpu, why not trigger
> > > > > it
> > > > > from
> > > > > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > > > > 
> > > > > This can done on the back of a request, saving most of the
> > > > > overhead
> > > > > and not requiring any extra field. Essentially, something
> > > > > like
> > > > > the
> > > > > (untested) patch below.
> > > > > 
> > > > > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > > > > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > > > > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > > > > diff --git a/arch/arm64/kvm/pmu-emul.c
> > > > > > b/arch/arm64/kvm/pmu-
> > > > > > emul.c
> > > > > > index fd167d4f4215..12a40f4b5f0d 100644
> > > > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct
> > > > > > kvm_vcpu
> > > > > > *vcpu, u64 val)
> > > > > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > > > > >       }
> > > > > > 
> > > > > > -     if (val & ARMV8_PMU_PMCR_C)
> > > > > > +     /*
> > > > > > +      * Cycle counter needs to reset in case of first vcpu
> > > > > > load.
> > > > > > +      */
> > > > > > +     if (val & ARMV8_PMU_PMCR_C ||
> > > > > > !kvm_arm_pmu_v3_restored(vcpu))
> > > > > 
> > > > > Why? There is no architectural guarantee that a counter
> > > > > resets to
> > > > > 0
> > > > > without writing PMCR_EL0.C. And if you want the guest to
> > > > > continue
> > > > > counting where it left off, resetting the counter is at best
> > > > > counter-productive.
> > > > 
> > > > Without this we would not be resetting PMU which is required
> > > > for
> > > > creating host perf events. With the patch that you suggested we
> > > > are
> > > > restoring PMCR_EL0 properly but still missing recreation of
> > > > host
> > > > perf
> > > > events.
> > > 
> > > How? The request that gets set on the first vcpu run will call
> > > kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
> > > kvm_pmu_create_perf_event(). What are we missing?
> > > 
> > 
> > I found out what I was missing. I was working with an older kernel
> > which was missing this upstream patch:
> > 
> > https://lore.kernel.org/lkml/20200124142535.29386-3-eric.auger@redhat.com/
> 
> :-(
> 
> Please test whatever you send with an upstream kernel. Actually,
> please *develop* on an upstream kernel. This will avoid this kind of
> discussion where we talk past each other, and make it plain that your
> production kernel is lacking all sorts of fixes.
> 
> Now, can you please state whether or not this patch fixes it for you
> *on an upstream kernel*? I have no interest in results from a
> production kernel.
> 
>         M.
> 

Really sorry for the noise and I can confirm that your suggested patch
fixes the problem for the upstream kernel i.e., if I live migrate a
guest which is actively using perf events then the guest can continue
using them even after live migration without interruption.

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



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
@ 2021-06-08  8:24             ` Jain, Jinank
  0 siblings, 0 replies; 26+ messages in thread
From: Jain, Jinank @ 2021-06-08  8:24 UTC (permalink / raw)
  To: maz
  Cc: james.morse, kvmarm, suzuki.poulose, linux-kernel,
	alexandru.elisei, linux-arm-kernel, will, catalin.marinas,
	Graf (AWS),
	Alexander

On Tue, 2021-06-08 at 09:18 +0100, Marc Zyngier wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> On Mon, 07 Jun 2021 19:34:08 +0100,
> "Jain, Jinank" <jinankj@amazon.de> wrote:
> > Hi Marc.
> > 
> > On Mon, 2021-06-07 at 17:35 +0100, Marc Zyngier wrote:
> > > CAUTION: This email originated from outside of the organization.
> > > Do
> > > not click links or open attachments unless you can confirm the
> > > sender
> > > and know the content is safe.
> > > 
> > > 
> > > 
> > > On Mon, 07 Jun 2021 17:05:01 +0100,
> > > "Jain, Jinank" <jinankj@amazon.de> wrote:
> > > > On Thu, 2021-06-03 at 17:03 +0100, Marc Zyngier wrote:
> > > > > Hi Jinank,
> > > > > 
> > > > > On Thu, 03 Jun 2021 12:05:54 +0100,
> > > > > Jinank Jain <jinankj@amazon.de> wrote:
> > > > > > Currently if a guest is live-migrated while it is actively
> > > > > > using
> > > > > > perf
> > > > > > counters, then after live-migrate it will notice that all
> > > > > > counters
> > > > > > would
> > > > > > suddenly start reporting 0s. This is due to the fact we are
> > > > > > not
> > > > > > re-creating the relevant perf events inside the kernel.
> > > > > > 
> > > > > > Usually on live-migration guest state is restored using
> > > > > > KVM_SET_ONE_REG
> > > > > > ioctl interface, which simply restores the value of PMU
> > > > > > registers
> > > > > > values but does not re-program the perf events so that the
> > > > > > guest
> > > > > > can seamlessly
> > > > > > use these counters even after live-migration like it was
> > > > > > doing
> > > > > > before
> > > > > > live-migration.
> > > > > > 
> > > > > > Instead there are two completely different code path
> > > > > > between
> > > > > > guest
> > > > > > accessing PMU registers and VMM restoring counters on
> > > > > > live-migration.
> > > > > > 
> > > > > > In case of KVM_SET_ONE_REG:
> > > > > > 
> > > > > > kvm_arm_set_reg()
> > > > > > ...... kvm_arm_sys_reg_set_reg()
> > > > > > ........... reg_from_user()
> > > > > > 
> > > > > > but in case when guest tries to access these counters:
> > > > > > 
> > > > > > handle_exit()
> > > > > > ..... kvm_handle_sys_reg()
> > > > > > ..........perform_access()
> > > > > > ...............access_pmu_evcntr()
> > > > > > ...................kvm_pmu_set_counter_value()
> > > > > > .......................kvm_pmu_create_perf_event()
> > > > > > 
> > > > > > The drawback of using the KVM_SET_ONE_REG interface is that
> > > > > > the
> > > > > > host pmu
> > > > > > events which were registered for the source instance and
> > > > > > not
> > > > > > present for
> > > > > > the destination instance.
> > > > > 
> > > > > I can't parse this sentence. Do you mean "are not present"?
> > > > > 
> > > > > > Thus passively restoring PMCR_EL0 using
> > > > > > KVM_SET_ONE_REG interface would not create the necessary
> > > > > > host
> > > > > > pmu
> > > > > > events
> > > > > > which are crucial for seamless guest experience across live
> > > > > > migration.
> > > > > > 
> > > > > > In ordet to fix the situation, on first vcpu load we should
> > > > > > restore
> > > > > > PMCR_EL0 in the same exact way like the guest was trying to
> > > > > > access
> > > > > > these counters. And then we will also recreate the relevant
> > > > > > host
> > > > > > pmu
> > > > > > events.
> > > > > > 
> > > > > > Signed-off-by: Jinank Jain <jinankj@amazon.de>
> > > > > > Cc: Alexander Graf (AWS) <graf@amazon.de>
> > > > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > > > Cc: James Morse <james.morse@arm.com>
> > > > > > Cc: Alexandru Elisei <alexandru.elisei@arm.com>
> > > > > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > > Cc: Will Deacon <will@kernel.org>
> > > > > > ---
> > > > > >  arch/arm64/include/asm/kvm_host.h |  1 +
> > > > > >  arch/arm64/kvm/arm.c              |  1 +
> > > > > >  arch/arm64/kvm/pmu-emul.c         | 10 ++++++++--
> > > > > >  arch/arm64/kvm/pmu.c              | 15 +++++++++++++++
> > > > > >  include/kvm/arm_pmu.h             |  3 +++
> > > > > >  5 files changed, 28 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > > > > b/arch/arm64/include/asm/kvm_host.h
> > > > > > index 7cd7d5c8c4bc..2376ad3c2fc2 100644
> > > > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > > > @@ -745,6 +745,7 @@ static inline int
> > > > > > kvm_arch_vcpu_run_pid_change(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_restore(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 e720148232a0..c66f6d16ec06 100644
> > > > > > --- a/arch/arm64/kvm/arm.c
> > > > > > +++ b/arch/arm64/kvm/arm.c
> > > > > > @@ -408,6 +408,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(vcpu);
> > > > > 
> > > > > If this only needs to be run once per vcpu, why not trigger
> > > > > it
> > > > > from
> > > > > kvm_arm_pmu_v3_enable(), which is also called once per vcpu?
> > > > > 
> > > > > This can done on the back of a request, saving most of the
> > > > > overhead
> > > > > and not requiring any extra field. Essentially, something
> > > > > like
> > > > > the
> > > > > (untested) patch below.
> > > > > 
> > > > > >       kvm_vcpu_pmu_restore_guest(vcpu);
> > > > > >       if (kvm_arm_is_pvtime_enabled(&vcpu->arch))
> > > > > >               kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu);
> > > > > > diff --git a/arch/arm64/kvm/pmu-emul.c
> > > > > > b/arch/arm64/kvm/pmu-
> > > > > > emul.c
> > > > > > index fd167d4f4215..12a40f4b5f0d 100644
> > > > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > > > @@ -574,10 +574,16 @@ void kvm_pmu_handle_pmcr(struct
> > > > > > kvm_vcpu
> > > > > > *vcpu, u64 val)
> > > > > >               kvm_pmu_disable_counter_mask(vcpu, mask);
> > > > > >       }
> > > > > > 
> > > > > > -     if (val & ARMV8_PMU_PMCR_C)
> > > > > > +     /*
> > > > > > +      * Cycle counter needs to reset in case of first vcpu
> > > > > > load.
> > > > > > +      */
> > > > > > +     if (val & ARMV8_PMU_PMCR_C ||
> > > > > > !kvm_arm_pmu_v3_restored(vcpu))
> > > > > 
> > > > > Why? There is no architectural guarantee that a counter
> > > > > resets to
> > > > > 0
> > > > > without writing PMCR_EL0.C. And if you want the guest to
> > > > > continue
> > > > > counting where it left off, resetting the counter is at best
> > > > > counter-productive.
> > > > 
> > > > Without this we would not be resetting PMU which is required
> > > > for
> > > > creating host perf events. With the patch that you suggested we
> > > > are
> > > > restoring PMCR_EL0 properly but still missing recreation of
> > > > host
> > > > perf
> > > > events.
> > > 
> > > How? The request that gets set on the first vcpu run will call
> > > kvm_pmu_handle_pmcr() -> kvm_pmu_enable_counter_mask() ->
> > > kvm_pmu_create_perf_event(). What are we missing?
> > > 
> > 
> > I found out what I was missing. I was working with an older kernel
> > which was missing this upstream patch:
> > 
> > https://lore.kernel.org/lkml/20200124142535.29386-3-eric.auger@redhat.com/
> 
> :-(
> 
> Please test whatever you send with an upstream kernel. Actually,
> please *develop* on an upstream kernel. This will avoid this kind of
> discussion where we talk past each other, and make it plain that your
> production kernel is lacking all sorts of fixes.
> 
> Now, can you please state whether or not this patch fixes it for you
> *on an upstream kernel*? I have no interest in results from a
> production kernel.
> 
>         M.
> 

Really sorry for the noise and I can confirm that your suggested patch
fixes the problem for the upstream kernel i.e., if I live migrate a
guest which is actively using perf events then the guest can continue
using them even after live migration without interruption.

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



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-06-08 13:05 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-03 11:05 [PATCH] KVM: arm64: Properly restore PMU state during live-migration Jinank Jain
2021-06-03 11:05 ` Jinank Jain
2021-06-03 11:05 ` Jinank Jain
2021-06-03 15:20 ` kernel test robot
2021-06-03 15:20   ` kernel test robot
2021-06-03 15:20   ` kernel test robot
2021-06-03 16:03 ` Marc Zyngier
2021-06-03 16:03   ` Marc Zyngier
2021-06-03 16:03   ` Marc Zyngier
2021-06-07 16:05   ` Jain, Jinank
2021-06-07 16:05     ` Jain, Jinank
2021-06-07 16:05     ` Jain, Jinank
2021-06-07 16:35     ` Marc Zyngier
2021-06-07 16:35       ` Marc Zyngier
2021-06-07 18:34       ` Jain, Jinank
2021-06-07 18:34         ` Jain, Jinank
2021-06-07 18:34         ` Jain, Jinank
2021-06-08  8:18         ` Marc Zyngier
2021-06-08  8:18           ` Marc Zyngier
2021-06-08  8:18           ` Marc Zyngier
2021-06-08  8:24           ` Jain, Jinank
2021-06-08  8:24             ` Jain, Jinank
2021-06-08  8:24             ` Jain, Jinank
2021-06-07 18:58 ` [PATCH v2] " Jinank Jain
2021-06-07 18:58   ` Jinank Jain
2021-06-07 18:58   ` Jinank Jain

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.