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

Re: [Xen-devel] [PATCH v3] xen/arm: fix rank/vgic lock inversion bug

Hi Stefano,

On 01/02/17 23:23, Stefano Stabellini wrote:
On Wed, 1 Feb 2017, Julien Grall wrote:
On 31/01/2017 23:49, Stefano Stabellini wrote:
On Fri, 27 Jan 2017, Julien Grall wrote:
On 03/01/17 23:29, Stefano Stabellini wrote:
So we have to worry about SPIs and LPIs (thought they are not yet supported).
From the note in 3.2.3:
"For any processor, if an interrupt is active and pending, the GIC does not
signal an interrupt exception request
for the interrupt to any processor until the active status is cleared."

Given that SPIs have an activate state and will be shared, this scenario
cannot happen.

Yes, but just above:

  A GICv1 implementation might ensure that only one processor can make a
  1-N interrupt active, removing the requirement for a lock on the ISR.
  This is not required by the architecture, and generic GIC code must not
  rely on this behavior.

That's for a GICv1 implementation. However, only GICv2 and onwards support virtualization.

then, the spec also says:

  The GIC maintains a state machine for each supported interrupt on each
  CPU interface.

So I am thinking that the statement you quoted wouldn't apply, because
the second interrupt wouldn't mark the state of the interrupt as "active
and pending" because, the interrupt being targeted to another processor,
it would simply mark it as "pending" in the CPU interface of the other

I don't have the proper quote in the spec, but I can confirm the interrupt can only be active on a single CPU at the time. It is also implied in the description of the 1-N model (see 1.4.3): " Only one processor handles this interrupt".

For LPIs, there is no activate state. So as soon as they are EOIed, they might
come up again. Depending on how will we handle irq migration, your scenario
will become true. I am not sure if we should take into account LPIs right now.

To be honest, I don't much like the idea of kicking the other vCPU. But I
don't have a better idea in order to clear the LRs.

Me neither, that's why I was proposing a different solution instead. We
still have the option to take the right lock in vgic_migrate_irq:


The code is more complex, but I think it's safe in all cases.

It is not only complex but also really confusing as we would have a variable protected by two locks, both lock does not need to be taken at the same time.

I may have an idea to avoid completely the lock in vgic_get_target_vcpu. The lock is only here to read the target vcpu in the rank, the rest does not need a lock, right? So could not we read the target vcpu atomically instead?


Julien Grall

Xen-devel mailing list



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