Re: [Xen-devel] [PATCH v9 20/28] ARM: GICv3: handle unmapped LPIs

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 19 May 2017, Stefano Stabellini wrote:
> On Thu, 11 May 2017, Andre Przywara wrote:
> > When LPIs get unmapped by a guest, they might still be in some LR of
> > some VCPU. Nevertheless we remove the corresponding pending_irq
> > (possibly freeing it), and detect this case (irq_to_pending() returns
> > NULL) when the LR gets cleaned up later.
> > However a *new* LPI may get mapped with the same number while the old
> > LPI is *still* in some LR. To avoid getting the wrong state, we mark
> > every newly mapped LPI as PRISTINE, which means: has never been in an
> > LR before. If we detect the LPI in an LR anyway, it must have been an
> > older one, which we can simply retire.
> > Before inserting such a PRISTINE LPI into an LR, we must make sure that
> > it's not already in another LR, as the architecture forbids two
> > interrupts with the same virtual IRQ number on one CPU.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > ---
> >  xen/arch/arm/gic.c         | 55 
> > +++++++++++++++++++++++++++++++++++++++++-----
> >  xen/include/asm-arm/vgic.h |  6 +++++
> >  2 files changed, 56 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index fd3fa05..8bf0578 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -375,6 +375,8 @@ static inline void gic_set_lr(int lr, struct 
> > pending_irq *p,
> >  {
> >      ASSERT(!local_irq_is_enabled());
> >  
> > +    clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status);
> > +
> >      gic_hw_ops->update_lr(lr, p, state);
> >  
> >      set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > @@ -442,12 +444,41 @@ void gic_raise_inflight_irq(struct vcpu *v, unsigned 
> > int virtual_irq)
> >  #endif
> >  }
> >  
> > +/*
> > + * Find an unused LR to insert an IRQ into. If this new interrupt is a
> > + * PRISTINE LPI, scan the other LRs to avoid inserting the same IRQ twice.
> > + */
> > +static int gic_find_unused_lr(struct vcpu *v, struct pending_irq *p, int 
> > lr)
> > +{
> > +    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
> > +    unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask);
> > +    struct gic_lr lr_val;
> > +
> > +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> > +
> > +    if ( test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
> 
> Maybe we should add an "unlikely".
> 
> I can see how this would be OKish at runtime, but at boot time there
> might be a bunch of PRISTINE_LPIs, but no MAPDs have been issued yet,
> right?
> 
> I have a suggestion, I'll leave it to you and Julien if you want to do
> this now, or maybe consider it as a TODO item. I am OK either way (I
> don't want to delay the ITS any longer).
> 
> I am thinking we should do this scanning only after at least one MAPD
> has been issued for a given cpu at least once. I would resurrect the
> idea of a DISCARD flag, but not on the pending_irq, that I believe it's
> difficult to handle, but a single global DISCARD flag per struct vcpu.
> 
> On MAPD, we set DISCARD for the target vcpu of the LPI we are dropping.
> Next time we want to inject a PRISTINE_IRQ on that cpu interface, we
> scan all LRs for interrupts with a NULL pending_irq. We remove those
> from LRs, then we remove the DISCARD flag.
> 
> Do you think it would work?

This would need to be done not only on MAPD but also on DISCARD (and on
any other commands that would cause a pending_irq - vLPI mapping to be
dropped).


> > +    {
> > +        int used_lr = 0;
> > +
> > +        while ( (used_lr = find_next_bit(lr_mask, nr_lrs, used_lr)) < 
> > nr_lrs )
> > +        {
> > +            gic_hw_ops->read_lr(used_lr, &lr_val);
> > +            if ( lr_val.virq == p->irq )
> > +                return used_lr;
> > +        }
> > +    }
> > +
> > +    lr = find_next_zero_bit(lr_mask, nr_lrs, lr);
> > +
> > +    return lr;
> > +}
> > +
> >  void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> >          unsigned int priority)
> >  {
> > -    int i;
> > -    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
> >      struct pending_irq *p = irq_to_pending(v, virtual_irq);
> > +    unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
> > +    int i = nr_lrs;
> >  
> >      ASSERT(spin_is_locked(&v->arch.vgic.lock));
> >  
> > @@ -457,7 +488,8 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int 
> > virtual_irq,
> >  
> >      if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
> >      {
> > -        i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
> > +        i = gic_find_unused_lr(v, p, 0);
> > +
> >          if (i < nr_lrs) {
> >              set_bit(i, &this_cpu(lr_mask));
> >              gic_set_lr(i, p, GICH_LR_PENDING);
> > @@ -509,7 +541,17 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> >      }
> >      else if ( lr_val.state & GICH_LR_PENDING )
> >      {
> > -        int q __attribute__ ((unused)) = 
> > test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> > +        int q __attribute__ ((unused));
> > +
> > +        if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
> > +        {
> > +            gic_hw_ops->clear_lr(i);
> > +            clear_bit(i, &this_cpu(lr_mask));
> > +
> > +            return;
> > +        }
> > +
> > +        q = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> >  #ifdef GIC_DEBUG
> >          if ( q )
> >              gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, 
> > when it is already pending in LR%d\n",
> > @@ -521,6 +563,9 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> >          gic_hw_ops->clear_lr(i);
> >          clear_bit(i, &this_cpu(lr_mask));
> >  
> > +        if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
> > +            return;
> >          if ( p->desc != NULL )
> >              clear_bit(_IRQ_INPROGRESS, &p->desc->status);
> >          clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > @@ -591,7 +636,7 @@ static void gic_restore_pending_irqs(struct vcpu *v)
> >      inflight_r = &v->arch.vgic.inflight_irqs;
> >      list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> >      {
> > -        lr = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, lr);
> > +        lr = gic_find_unused_lr(v, p, lr);
> >          if ( lr >= nr_lrs )
> >          {
> >              /* No more free LRs: find a lower priority irq to evict */
> > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> > index 02732db..3fc4ceb 100644
> > --- a/xen/include/asm-arm/vgic.h
> > +++ b/xen/include/asm-arm/vgic.h
> > @@ -60,12 +60,18 @@ struct pending_irq
> >       * vcpu while it is still inflight and on an GICH_LR register on the
> >       * old vcpu.
> >       *
> > +     * GIC_IRQ_GUEST_PRISTINE_LPI: the IRQ is a newly mapped LPI, which
> > +     * has never been in an LR before. This means that any trace of an
> > +     * LPI with the same number in an LR must be from an older LPI, which
> > +     * has been unmapped before.
> > +     *
> >       */
> >  #define GIC_IRQ_GUEST_QUEUED   0
> >  #define GIC_IRQ_GUEST_ACTIVE   1
> >  #define GIC_IRQ_GUEST_VISIBLE  2
> >  #define GIC_IRQ_GUEST_ENABLED  3
> >  #define GIC_IRQ_GUEST_MIGRATING   4
> > +#define GIC_IRQ_GUEST_PRISTINE_LPI  5
> >      unsigned long status;
> >      struct irq_desc *desc; /* only set it the irq corresponds to a 
> > physical irq */
> >      unsigned int irq;
> > -- 
> > 2.9.0
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx
> > https://lists.xen.org/xen-devel
> > 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel