[Xen-devel,v11,03/10] xen/arm: inflight irqs during migration

On Fri, 8 Aug 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 08/08/2014 06:13 PM, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index 63d4f65..39d8272 100644
> > --- a/xen/arch/arm/vgic-v2.c
> > +++ b/xen/arch/arm/vgic-v2.c
> > @@ -356,34 +356,60 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >          goto write_ignore;
> >  
> >      case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
> > +    {
> > +        /* unsigned long needed for find_next_bit */
> > +        unsigned long target;
> > +        int i;
> >          if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
> >          rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
> >          if ( rank == NULL) goto write_ignore;
> >          /* 8-bit vcpu mask for this domain */
> >          BUG_ON(v->domain->max_vcpus > 8);
> > -        tr = (1 << v->domain->max_vcpus) - 1;
> > +        target = (1 << v->domain->max_vcpus) - 1;
> >          if ( dabt.size == 2 )
> > -            tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> > +            target = target | (target << 8) | (target << 16) | (target << 24);
> >          else
> > -            tr = (tr << (8 * (gicd_reg & 0x3)));
> > -        tr &= *r;
> > +            target = (target << (8 * (gicd_reg & 0x3)));
> > +        target &= *r;
> 
> Renaming tr in target in this patch while most of the code has been
> introduced in patch #1 is very odd. Actually it make the code harder to
> read.
> 
> Anyway, I think it's fine a V11. I don't want to delay the merge just
> for that.
> 
> > +void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
> > +{
> > +    unsigned long flags;
> > +    struct pending_irq *p = irq_to_pending(old, irq);
> > +
> > +    /* nothing to do for virtual interrupts */
> > +    if ( p->desc == NULL )
> > +        return;
> > +
> > +    /* migration already in progress, no need to do anything */
> > +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> > +        return;
> > +
> > +    spin_lock_irqsave(&old->arch.vgic.lock, flags);
> > +
> > +    if ( list_empty(&p->inflight) )
> > +    {
> > +        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> > +        return;
> > +    }
> 
> NIT: I would create a label unlock below and jump to it. It would avoid
> at least one of the 3 call to spin_unlock.

I would rather avoid making this kind of code style changes at this
stage.


> > +    /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
> > +    if ( !list_empty(&p->lr_queue) )
> > +    {
> > +        list_del_init(&p->lr_queue);
> > +        list_del_init(&p->inflight);
> > +        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> > +        vgic_vcpu_inject_irq(new, irq);
> 
> Shouldn't we also clear the p->status? At least for consistency?

We should probably clear GIC_IRQ_GUEST_QUEUED, but in practice it
doesn't make any difference because vgic_vcpu_inject_irq immediately
sets it again.

This is the updated patch with the GIC_IRQ_GUEST_QUEUED clear.

---

xen/arm: inflight irqs during migration

We need to take special care when migrating irqs that are already
inflight from one vcpu to another. See "The effect of changes to an
GICD_ITARGETSR", part of chapter 4.3.12 of the ARM Generic Interrupt
Controller Architecture Specification, IHI 0048B.

The main issue from the Xen point of view is that the lr_pending and
inflight lists are per-vcpu. The lock we take to protect them is also
per-vcpu.

In order to avoid issues, if the irq is still lr_pending, we can
immediately move it to the new vcpu for injection.

Otherwise if it is in a GICH_LR register, set a new flag
GIC_IRQ_GUEST_MIGRATING.  If GIC_IRQ_GUEST_MIGRATING is set we'll change
the affinity of the physical irq when clearing the LR (in a following
commit). Therefore if the irq is inflight in an LR when the guest writes
to itarget, we wait until the LR is cleared before changing physical
irq affinity. This guarantees that the old vcpu is going to be
interrupted and clear the LR, either because it has been descheduled
after EOIing the interrupt or because it is interrupted by the second
physical irq coming.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v12:
- clear GIC_IRQ_GUEST_QUEUED in vgic_migrate_irq.

Changes in v11:
- fix irq calculation in vgic_v2_distr_mmio_write.

Changes in v9:
- simplify the code: rely on "physical irq follow virtual irq" to ensure
that physical irqs are injected into the right pcpu.

Changes in v8:
- rebase on ab78724fc5628318b172b4344f7280621a151e1b;
- rename target to new_target to avoid conflicts.

Changes in v7:
- move the _VPF_down check before setting GIC_IRQ_GUEST_QUEUED;
- fix comments;
- rename trl to target;
- introduce vcpu_migrate_from;
- do not kick new vcpu on MIGRATING, kick the old vcpu instead;
- separate moving GIC_IRQ_GUEST_QUEUED earlier into a different patch.

Changes in v6:
- remove unnecessary casts to (const unsigned long *) to the arguments
of find_first_bit and find_next_bit;
- instroduce a corresponding smb_rmb call;
- deal with migrating an irq that is inflight and still lr_pending;
- replace the dsb with smb_wmb and smb_rmb, use them to ensure the order
of accesses to GIC_IRQ_GUEST_QUEUED and GIC_IRQ_GUEST_MIGRATING;

Changes in v5:
- pass unsigned long to find_next_bit for alignment on aarch64;
- call vgic_get_target_vcpu instead of gic_add_to_lr_pending to add the
irq in the right inflight queue;
- add barrier and bit tests to make sure that vgic_migrate_irq and
gic_update_one_lr can run simultaneously on different cpus without
issues;
- rework the loop to identify the new vcpu when ITARGETSR is written;
- use find_first_bit instead of find_next_bit.
---
 xen/arch/arm/gic.c         |    8 ++++++--
 xen/arch/arm/vgic-v2.c     |   44 +++++++++++++++++++++++++++++++++++---------
 xen/arch/arm/vgic.c        |   38 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vgic.h |    6 ++++++
 4 files changed, 85 insertions(+), 11 deletions(-)

Hi Stefano,

On 08/11/2014 04:23 PM, Stefano Stabellini wrote:
> On Fri, 8 Aug 2014, Julien Grall wrote:
>>> +void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
>>> +{
>>> +    unsigned long flags;
>>> +    struct pending_irq *p = irq_to_pending(old, irq);
>>> +
>>> +    /* nothing to do for virtual interrupts */
>>> +    if ( p->desc == NULL )
>>> +        return;
>>> +
>>> +    /* migration already in progress, no need to do anything */
>>> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
>>> +        return;
>>> +
>>> +    spin_lock_irqsave(&old->arch.vgic.lock, flags);
>>> +
>>> +    if ( list_empty(&p->inflight) )
>>> +    {
>>> +        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>>> +        return;
>>> +    }
>>
>> NIT: I would create a label unlock below and jump to it. It would avoid
>> at least one of the 3 call to spin_unlock.
> 
> I would rather avoid making this kind of code style changes at this
> stage.

I'm fine with that.

>>> +    /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
>>> +    if ( !list_empty(&p->lr_queue) )
>>> +    {
>>> +        list_del_init(&p->lr_queue);
>>> +        list_del_init(&p->inflight);
>>> +        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>>> +        vgic_vcpu_inject_irq(new, irq);
>>
>> Shouldn't we also clear the p->status? At least for consistency?
> 
> We should probably clear GIC_IRQ_GUEST_QUEUED, but in practice it
> doesn't make any difference because vgic_vcpu_inject_irq immediately
> sets it again.
> 
> This is the updated patch with the GIC_IRQ_GUEST_QUEUED clear.

Thanks! I think we should take this one. It may avoid issue later if the
code is changing, such as checking GUEST_QUEUED in different place.

For the updated patch:

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,