Re: [PATCH v3] KVM: arm/arm64 : add lpi info in vgic-debug

[Marc Zyngier <marc.zyngier@arm.com>]

On Sat, 24 Mar 2018 02:08:54 +0000,
peng hao wrote:
> 
> [1  <multipart/alternative (7bit)>]
> [1.1  <text/plain; UTF-8 (base64)>]
> >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 <peng.hao2@zte.com.cn>
> >> ---
> >>  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.