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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 28 Dec 2016, Julien Grall wrote:
> Hi Stefano,
> 
> 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.

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