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

[Andre Przywara <andre.przywara@arm.com>]

Hi Stefano,

On 20/05/17 02:25, 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@arm.com>
>> ---
>>  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,

What is your concern here, performance?
Let's put this into perspective:
- The PRISTINE bit gets set upon MAPTI, which Linux usually does *once*
when the driver gets loaded. It gets cleared after the first injection.
- If that happens, we scan all LRs. Most implementations have 4(!) of
them (ARM GIC implementations, for instance), also the algorithm only
scans *used* LRs, so normally just one or two.
- Reading the LR is a *local* system register *read*, not an MMIO
access, and not propagated to other cores. Yes, this may be "costly"
(compared to other instructions), but it's probably still cheaper than a
page table walk (TLB miss) or L2 cache miss.

So to summarize: this is rare, iterates over only a very small number of
registers and is not hugely expensive.
At this point in time I would refrain from any kind of performance
optimization, at least unless we have solved all the other issues and
have done some benchmarking/profiling (on different hardware platforms).

> but no MAPDs have been issued yet, right?

As Julien already mentioned, this gets set after a MAPTI, which requires
a MAPD before.

Cheers,
Andre.

> 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@lists.xen.org
>> https://lists.xen.org/xen-devel
>>

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