Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
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" 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" 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 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 > > > > > > Cc: Alexander Graf (AWS) > > > > > > Cc: Marc Zyngier > > > > > > Cc: James Morse > > > > > > Cc: Alexandru Elisei > > > > > > Cc: Suzuki K Poulose > > > > > > Cc: Catalin Marinas > > > > > > Cc: Will Deacon > > > > > > --- > > > > > > 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
Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
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" 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 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 > > > > Cc: Alexander Graf (AWS) > > > > Cc: Marc Zyngier > > > > Cc: James Morse > > > > Cc: Alexandru Elisei > > > > Cc: Suzuki K Poulose > > > > Cc: Catalin Marinas > > > > Cc: Will Deacon > > > > --- > > > > 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(>arch)) > > > > kvm_make_request(KVM_REQ_RECORD_STEAL, vcpu); > > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu- > >
Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
On Mon, 07 Jun 2021 19:34:08 +0100, "Jain, Jinank" 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" 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 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 > > > > > Cc: Alexander Graf (AWS) > > > > > Cc: Marc Zyngier > > > > > Cc: James Morse > > > > > Cc: Alexandru Elisei > > > > > Cc: Suzuki K Poulose > > > > > Cc: Catalin Marinas > > > > > Cc: Will Deacon > > > > > --- > > > > > 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
Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
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 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 > > Cc: Alexander Graf (AWS) > > Cc: Marc Zyngier > > Cc: James Morse > > Cc: Alexandru Elisei > > Cc: Suzuki K Poulose > > Cc: Catalin Marinas > > Cc: Will Deacon > > --- > > 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(>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
Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
On Mon, 07 Jun 2021 17:05:01 +0100, "Jain, Jinank" 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 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 > > > Cc: Alexander Graf (AWS) > > > Cc: Marc Zyngier > > > Cc: James Morse > > > Cc: Alexandru Elisei > > > Cc: Suzuki K Poulose > > > Cc: Catalin Marinas > > > Cc: Will Deacon > > > --- > > > 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(>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
Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
Hi Jinank,
On Thu, 03 Jun 2021 12:05:54 +0100,
Jinank Jain 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
> Cc: Alexander Graf (AWS)
> Cc: Marc Zyngier
> Cc: James Morse
> Cc: Alexandru Elisei
> Cc: Suzuki K Poulose
> Cc: Catalin Marinas
> Cc: Will Deacon
> ---
> 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(>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, , 32)
> kvm_pmu_set_counter_value(vcpu, i, 0);
> }
The rest of the changes
Re: [PATCH] KVM: arm64: Properly restore PMU state during live-migration
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
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-...@lists.01.org
.config.gz
Description: application/gzip
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
