[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.