| Message ID | 20210319161711.24972-8-yoan.picchi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: add more event counters for kvm_stat | expand |
On Fri, 19 Mar 2021 16:17:11 +0000, Yoan Picchi <yoan.picchi@arm.com> wrote: > > Add a counter for interrupt injections. That is when kvm relay an > interrupt to the guest (for instance a timer, or a device interrupt > like from a network card) > > Signed-off-by: Yoan Picchi <yoan.picchi@arm.com> > --- > arch/arm64/include/asm/kvm_host.h | 2 ++ > arch/arm64/kvm/arm.c | 2 ++ > arch/arm64/kvm/guest.c | 2 ++ > arch/arm64/kvm/vgic/vgic.c | 2 ++ > 4 files changed, 8 insertions(+) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index fa59b669c..253acb8c2 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -551,6 +551,7 @@ struct kvm_vm_stat { > ulong memory_slot_unmaped; > ulong stage2_unmap_vm; > ulong cached_page_invalidated; > + ulong irq_inject; > }; > > struct kvm_vcpu_stat { > @@ -567,6 +568,7 @@ struct kvm_vcpu_stat { > u64 mmio_exit_kernel; > u64 regular_page_mapped; > u64 huge_page_mapped; > + u64 irq_inject; > u64 exits; > }; > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index fc4c95dd2..841551f14 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -895,6 +895,8 @@ static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level) > bool set; > unsigned long *hcr; > > + vcpu->stat.irq_inject++; > + > if (number == KVM_ARM_IRQ_CPU_IRQ) > bit_index = __ffs(HCR_VI); > else /* KVM_ARM_IRQ_CPU_FIQ */ > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index 129c0d53d..f663b03ae 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -45,6 +45,8 @@ struct kvm_stats_debugfs_item debugfs_entries[] = { > VM_STAT("memory_slot_unmaped", memory_slot_unmaped), > VM_STAT("stage2_unmap_vm", stage2_unmap_vm), > VM_STAT("cached_page_invalidated", cached_page_invalidated), > + VM_STAT("irq_inject", irq_inject), > + VCPU_STAT("irq_inject", irq_inject), > VCPU_STAT("exits", exits), > VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns), > VCPU_STAT("halt_poll_fail_ns", halt_poll_fail_ns), > diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c > index 1c597c988..9e504243b 100644 > --- a/arch/arm64/kvm/vgic/vgic.c > +++ b/arch/arm64/kvm/vgic/vgic.c > @@ -458,6 +458,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid, > > raw_spin_lock_irqsave(&irq->irq_lock, flags); > > + kvm->stat.irq_inject++; > + > if (!vgic_validate_injection(irq, level, owner)) { So even if the injection failed, you report an injection? And what about injection that occur via the MMIO interface? What about direct injection? What about a level interrupt that is forever high? M.
Hi Mark. Thanks for all the reviews. I am a beginner and you gave me a lot to learn about. I will reply to the other patch progressively once I understand better the issues. On 23/03/2021 17:37, Marc Zyngier wrote: > On Fri, 19 Mar 2021 16:17:11 +0000, > Yoan Picchi <yoan.picchi@arm.com> wrote: >> Add a counter for interrupt injections. That is when kvm relay an >> interrupt to the guest (for instance a timer, or a device interrupt >> like from a network card) >> >> Signed-off-by: Yoan Picchi <yoan.picchi@arm.com> >> --- >> arch/arm64/include/asm/kvm_host.h | 2 ++ >> arch/arm64/kvm/arm.c | 2 ++ >> arch/arm64/kvm/guest.c | 2 ++ >> arch/arm64/kvm/vgic/vgic.c | 2 ++ >> 4 files changed, 8 insertions(+) >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index fa59b669c..253acb8c2 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -551,6 +551,7 @@ struct kvm_vm_stat { >> ulong memory_slot_unmaped; >> ulong stage2_unmap_vm; >> ulong cached_page_invalidated; >> + ulong irq_inject; >> }; >> >> struct kvm_vcpu_stat { >> @@ -567,6 +568,7 @@ struct kvm_vcpu_stat { >> u64 mmio_exit_kernel; >> u64 regular_page_mapped; >> u64 huge_page_mapped; >> + u64 irq_inject; >> u64 exits; >> }; >> >> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c >> index fc4c95dd2..841551f14 100644 >> --- a/arch/arm64/kvm/arm.c >> +++ b/arch/arm64/kvm/arm.c >> @@ -895,6 +895,8 @@ static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level) >> bool set; >> unsigned long *hcr; >> >> + vcpu->stat.irq_inject++; >> + >> if (number == KVM_ARM_IRQ_CPU_IRQ) >> bit_index = __ffs(HCR_VI); >> else /* KVM_ARM_IRQ_CPU_FIQ */ >> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c >> index 129c0d53d..f663b03ae 100644 >> --- a/arch/arm64/kvm/guest.c >> +++ b/arch/arm64/kvm/guest.c >> @@ -45,6 +45,8 @@ struct kvm_stats_debugfs_item debugfs_entries[] = { >> VM_STAT("memory_slot_unmaped", memory_slot_unmaped), >> VM_STAT("stage2_unmap_vm", stage2_unmap_vm), >> VM_STAT("cached_page_invalidated", cached_page_invalidated), >> + VM_STAT("irq_inject", irq_inject), >> + VCPU_STAT("irq_inject", irq_inject), >> VCPU_STAT("exits", exits), >> VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns), >> VCPU_STAT("halt_poll_fail_ns", halt_poll_fail_ns), >> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c >> index 1c597c988..9e504243b 100644 >> --- a/arch/arm64/kvm/vgic/vgic.c >> +++ b/arch/arm64/kvm/vgic/vgic.c >> @@ -458,6 +458,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid, >> >> raw_spin_lock_irqsave(&irq->irq_lock, flags); >> >> + kvm->stat.irq_inject++; >> + >> if (!vgic_validate_injection(irq, level, owner)) { > So even if the injection failed, you report an injection? And what > about injection that occur via the MMIO interface? What about direct > injection? What about a level interrupt that is forever high? > > M. > This one I actually started to fix this afternoon by moving the counter into vgic_queue_irq_unlock(). This way it is only incremented when the interrupt is inserted into a vcpu, and it also takes care of the vgic_mmio injections. I also fixed the issue with the interrupt line so it only increment when the line change of level. I'm not sure about what you mean by direct injection yet though. Kindly, Yoan
On Tue, 23 Mar 2021 17:53:42 +0000, Yoan Picchi <yoan.picchi@arm.com> wrote: > > Hi Mark. s/k/c/, please! > > Thanks for all the reviews. I am a beginner and you gave me a lot to > learn about. I will reply to the other patch progressively once I > understand better the issues. I think you should consider what I said in my reply to the cover letter before going all out on every counter you have introduced in this series. [...] > >> diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c > >> index 1c597c988..9e504243b 100644 > >> --- a/arch/arm64/kvm/vgic/vgic.c > >> +++ b/arch/arm64/kvm/vgic/vgic.c > >> @@ -458,6 +458,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid, > >> raw_spin_lock_irqsave(&irq->irq_lock, flags); > >> + kvm->stat.irq_inject++; > >> + > >> if (!vgic_validate_injection(irq, level, owner)) { > > So even if the injection failed, you report an injection? And what > > about injection that occur via the MMIO interface? What about direct > > injection? What about a level interrupt that is forever high? > > > > M. > > > This one I actually started to fix this afternoon by moving the > counter into vgic_queue_irq_unlock(). This way it is only > incremented when the interrupt is inserted into a vcpu, and it also > takes care of the vgic_mmio injections. I also fixed the issue with > the interrupt line so it only increment when the line change of > level. But if you do that, you start counting interrupts the guest itself generates. What is the exact semantic of this counter? userspace injected interrupts? Acked interrupts? Any interrupt? Take my level interrupt example. The interrupt will be forever pending, the guest will take as many interrupt as it can process, and yet your counter will have been incremented *once*. What does your counter mean then? > I'm not sure about what you mean by direct injection yet though. GICv4.{0,1}, where an interrupt gets directly delivered to the guest without (too much) SW intervention. With this, directly injected LPIs will never result in the counter being incremented, and yet can pin the guest to the ground under interrupt load. Again, defining the exact behaviour of the counter would avoid me ranting away... M.
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index fa59b669c..253acb8c2 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -551,6 +551,7 @@ struct kvm_vm_stat { ulong memory_slot_unmaped; ulong stage2_unmap_vm; ulong cached_page_invalidated; + ulong irq_inject; }; struct kvm_vcpu_stat { @@ -567,6 +568,7 @@ struct kvm_vcpu_stat { u64 mmio_exit_kernel; u64 regular_page_mapped; u64 huge_page_mapped; + u64 irq_inject; u64 exits; }; diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index fc4c95dd2..841551f14 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -895,6 +895,8 @@ static int vcpu_interrupt_line(struct kvm_vcpu *vcpu, int number, bool level) bool set; unsigned long *hcr; + vcpu->stat.irq_inject++; + if (number == KVM_ARM_IRQ_CPU_IRQ) bit_index = __ffs(HCR_VI); else /* KVM_ARM_IRQ_CPU_FIQ */ diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index 129c0d53d..f663b03ae 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -45,6 +45,8 @@ struct kvm_stats_debugfs_item debugfs_entries[] = { VM_STAT("memory_slot_unmaped", memory_slot_unmaped), VM_STAT("stage2_unmap_vm", stage2_unmap_vm), VM_STAT("cached_page_invalidated", cached_page_invalidated), + VM_STAT("irq_inject", irq_inject), + VCPU_STAT("irq_inject", irq_inject), VCPU_STAT("exits", exits), VCPU_STAT("halt_poll_success_ns", halt_poll_success_ns), VCPU_STAT("halt_poll_fail_ns", halt_poll_fail_ns), diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c index 1c597c988..9e504243b 100644 --- a/arch/arm64/kvm/vgic/vgic.c +++ b/arch/arm64/kvm/vgic/vgic.c @@ -458,6 +458,8 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid, raw_spin_lock_irqsave(&irq->irq_lock, flags); + kvm->stat.irq_inject++; + if (!vgic_validate_injection(irq, level, owner)) { /* Nothing to see here, move along... */ raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
Add a counter for interrupt injections. That is when kvm relay an interrupt to the guest (for instance a timer, or a device interrupt like from a network card) Signed-off-by: Yoan Picchi <yoan.picchi@arm.com> --- arch/arm64/include/asm/kvm_host.h | 2 ++ arch/arm64/kvm/arm.c | 2 ++ arch/arm64/kvm/guest.c | 2 ++ arch/arm64/kvm/vgic/vgic.c | 2 ++ 4 files changed, 8 insertions(+)