Re: [Xen-devel] [PATCH v2 3/4] arm, vgic_migrate_irq: take the right vgic lock

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 03/01/17 23:30, Stefano Stabellini wrote:
> On Wed, 28 Dec 2016, Julien Grall wrote:
> > On 22/12/16 02:15, Stefano Stabellini wrote:
> > > Always take the vgic lock of the old vcpu. When more than one irq
> > > migration is requested before the first one completes, take the vgic
> > > lock of the oldest vcpu.
> > >
> > > Write the new vcpu id into the rank from vgic_migrate_irq, protected by
> > > the oldest vgic vcpu lock.
> > >
> > > Use barriers to ensure proper ordering between clearing inflight and
> > > MIGRATING and setting vcpu to GIC_INVALID_VCPU.
> > The field p->status was designed to be accessed without any lock using *_bit
> > helpers.
> >
> > Currently vgic_migrate_irq is protected by the rank lock (an ASSERT would
> > probably useful for documentation) and can only be called once at the time.
> >
> > Let's take the following example to describe the problem:
> >     1) IRQ has been injected into vCPU A (e.g present in the LR)
> >     2) IRQ is migrated to vCPU B
> >     3) IRQ is migrated to vCPU C
> >     4) IRQ is EOIed by the guest
> >
> > When vgic_irq_migrate_irq is called for the first time (step 2), the vCPU A
> > vgic lock will be taken and GIC_IRQ_GUEST_MIGRATED will be set at the end. The
> > second time (step 3), GIC_IRQ_GUEST_MIGRATING is already set, so the function
> > will return directly no lock needed.
> Right, but the caller, vgic_store_itargetsr, will still write to
> rank->vcpu.
>
>
> > So, I think the vgic lock taken is already correct.
> The problem arises by 3) and 4) running simultaneously, and specifically
> from the rank->vcpu write in vgic_store_itargetsr running concurrently
> with gic_update_one_lr, as described in
> alpine.DEB.2.10.1612191337370.13831@sstabellini-ThinkPad-X260:
>
>   CPU0                                  CPU1
>   <GIC_IRQ_GUEST_MIGRATING is set>      <GIC_IRQ_GUEST_MIGRATING is set>
>   ----------------------------------------------------------------------
>                                         vgic_migrate_irq C (does nothing)
>   clear GIC_IRQ_GUEST_MIGRATING
>   read rank->vcpu B
>                                         set rank->vcpu C
>   irq_set_affinity B
>
>
> Result: the irq physical affinity is B instead of C.
>
> Please note that the new patch
> (1483486167-24607-1-git-send-email-sstabellini@xxxxxxxxxx) doesn't have
> this problem because it removes the call to irq_set_affinity in
> gic_update_one_lr.
Sorry I think there was a bit of confusion with my previous e-mail. I 
was describing the behavior with a change in the code similar to your v3.
Cheers,

--
Julien Grall

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