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

[Julien Grall <julien.grall@xxxxxxx>]

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
> processor.
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:
>
> http://marc.info/?l=xen-devel&m=148237289620471
>
> 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?
Cheers,

--
Julien Grall

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