|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 20/28] ARM: GICv3: handle unmapped LPIs
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@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,
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@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |