From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751953AbeCXKPd (ORCPT ); Sat, 24 Mar 2018 06:15:33 -0400 Received: from foss.arm.com ([217.140.101.70]:58084 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751755AbeCXKPb (ORCPT ); Sat, 24 Mar 2018 06:15:31 -0400 Date: Sat, 24 Mar 2018 10:15:26 +0000 Message-ID: <86muyx230h.wl-marc.zyngier@arm.com> From: Marc Zyngier To: Cc: , , , Subject: Re: [PATCH v3] KVM: arm/arm64 : add lpi info in vgic-debug In-Reply-To: <201803241008543904320@zte.com.cn> References: <201803241008543904320@zte.com.cn> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL/10.8 EasyPG/1.0.0 Emacs/25.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Organization: ARM Ltd MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 24 Mar 2018 02:08:54 +0000, peng hao wrote: > > [1 ] > [1.1 ] > >On 24/03/18 00:42, Peng Hao wrote: > >> Add lpi debug info to vgic-stat. > >> The printed info like this: > >> SPI 287 0 000001 0 0 0 160 -1 > >> LPI 8192 2 000100 0 0 0 160 -1 > >> > >> Signed-off-by: Peng Hao > >> --- > >> virt/kvm/arm/vgic/vgic-debug.c | 59 ++++++++++++++++++++++++++++++++++++++---- > >> virt/kvm/arm/vgic/vgic-its.c | 16 ++++++------ > >> virt/kvm/arm/vgic/vgic.h | 1 + > >> 3 files changed, 63 insertions(+), 13 deletions(-) > >> > ..... > >> + for (i = 0; i < irq_count; i++) { > >> + irq = vgic_get_irq(kvm, NULL, intids[i]); > >> + if (!irq) > >> + continue; > >> + lpi_irqs[iter->nr_lpis++] = irq; > >> + } > >> + iter->lpi_irqs = lpi_irqs; > >> + kfree(intids); > > >You are still completely missing the point. Why are you allocating this > >array of pointers while you have a perfectly sensible array of intids, > >allowing you do treat all the irqs uniformly? > > >> } > >> > >> static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter, > >> @@ -64,6 +100,8 @@ static void iter_init(struct kvm *kvm, struct vgic_state_iter *iter, > >> iter->nr_cpus = nr_cpus; > >> iter->nr_spis = kvm->arch.vgic.nr_spis; > >> > > + if (vgic_supports_direct_msis(kvm) && !pos) > >> + vgic_debug_get_lpis(kvm, iter); > > >Again: What is the point of this? > > >> /* Fast forward to the right position if needed */ > >> while (pos--) > >> iter_next(iter); > >> @@ -73,7 +111,9 @@ static bool end_of_vgic(struct vgic_state_iter *iter) > >> { > >> return iter->dist_id > 0 && > >> iter->vcpu_id == iter->nr_cpus && > >> - (iter->intid - VGIC_NR_PRIVATE_IRQS) == iter->nr_spis; > >> + (iter->intid - VGIC_NR_PRIVATE_IRQS) >= iter->nr_spis && > >> + ((iter->nr_lpis == 0) || > >> + (iter->lpi_print_count == iter->nr_lpis + 1)); > >> } > >> > >> static void *vgic_debug_start(struct seq_file *s, loff_t *pos) > >> @@ -130,6 +170,7 @@ static void vgic_debug_stop(struct seq_file *s, void *v) > >> > >> mutex_lock(&kvm->lock); > >> iter = kvm->arch.vgic.iter; > >> + kfree(iter->lpi_irqs); > >> kfree(iter); > >> kvm->arch.vgic.iter = NULL; > >> mutex_unlock(&kvm->lock); > >> @@ -154,7 +195,7 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq, > >> struct kvm_vcpu *vcpu) > >> { > >> int id = 0; > >> - char *hdr = "SPI "; > >> + char *hdr = "Global"; > >> > >> if (vcpu) { > >> hdr = "VCPU"; > >> @@ -162,7 +203,10 @@ static void print_header(struct seq_file *s, struct vgic_irq *irq, > >> } > >> > >> seq_printf(s, "\n"); > .... > >> print_irq_state(s, irq, vcpu); > >> spin_unlock(&irq->irq_lock); > >> + vgic_put_irq(kvm, irq); > > >Doesn't it shock you that you're doing a "put" on something you haven't > >done a "get" on? > > >[...] > > >Here's what I mean[1]. No double allocation, uniform access to the irq > >pointer, no imbalance in reference management. > Thanks for your help. > By the way, I want to know which device you use for testing vgic-v4 function. > I passthrough one VF to VM,but it just says "timeout". I use both a software model (FastModel) and a HiSilicon D05 system. M. -- Jazz is not dead, it just smell funny.