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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?


> +    {
> +        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