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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 20 Dec 2016, Stefano Stabellini wrote:
> On Tue, 20 Dec 2016, Julien Grall wrote:
> > Hi Stefano,
> > 
> > On 20/12/2016 00:22, Stefano Stabellini wrote:
> > > On Mon, 19 Dec 2016, Julien Grall wrote:
> > > > Hi Stefano,
> > > > 
> > > > On 19/12/2016 23:30, Stefano Stabellini wrote:
> > > > > On Mon, 19 Dec 2016, Julien Grall wrote:
> > > > > > > > 2) We run gic_update_one_lr and vgic_store_itargetsr in parallel
> > > > > > > > safely
> > > > > > > > and locklessly. There might be a way to do it, but it is not 
> > > > > > > > easy
> > > > > > > > I
> > > > > > > > haven't found it yet.
> > > > > > 
> > > > > > Correct me if I am wrong. There are no restriction to write into
> > > > > > IROUTER/ITARGETSR while an IRQ is pending. So the irq_set_affinity
> > > > > > could
> > > > > > be
> > > > > > called once at the beginning of vgic_irq_migrate.
> > > > > > 
> > > > > > We may receive the interrupt on the wrong physical CPU at the
> > > > > > beginning.
> > > > > > But
> > > > > > it would be the same when writing into IROUTER/ITARGETSR.
> > > > > > 
> > > > > > This would remove the need to get the rank lock in 
> > > > > > gic_update_one_lr.
> > > > > > 
> > > > > > Did I miss anything?
> > > > > 
> > > > > I am not sure if we can do that: the virtual interrupt might not be
> > > > > EOI'd yet at that time. The guest EOI is used to deactivate the
> > > > > corresponding physical interrupt. Migrating the physical IRQ at that
> > > > > time, could have unintended consequences? I am not sure what the spec
> > > > > says about this, but even if it is correct on paper, I would prefer 
> > > > > not
> > > > > to put it to the test: I bet that not many interrupt controllers have
> > > > > been heavily tested in this scenario. What do you think?
> > > > 
> > > > I don't think this is an issue. An interrupt could have the priority 
> > > > drop
> > > > happening on pCPU A and be deactivated on pCPU B if the vCPU A is been
> > > > migrated when the interrupt is inflight. So if it is fine here, why 
> > > > would
> > > > not
> > > > it be when the guest is specifically requesting the routing?
> > > 
> > > That is true, but this is not exactly the same.
> > 
> > My example was to show you that an IRQ can have its priority dropped in 
> > pCPU A
> > and been deactivated to pCPU B. Another example is when only the IRQ is been
> > migrated. The spec does not promise you to receive the next interrupt on the
> > CPU you asked because it may take time to update the GIC state. So the
> > priority drop and deactivation could be done on separate physical CPU here
> > too.
> > 
> > > This is changing the
> > > physical irq affinity while both physical and virtual irqs are still
> > > active.
> > 
> > Physical IRQ state and virtual IRQ state are completely dissociated in the
> > GIC. The only interaction possible is the virtual interface to send a
> > deactivate request to the distributor when the virtual interrupt has been
> > deactivated and correspond to a hardware interrupt.
> > 
> > > As I wrote, usually operating systems only change affinity after
> > > deactivating an irq, so I thought it would be wise in Xen to wait at
> > > least for the EOI.
> > 
> > I looked at the Linux code and did not see a such requirement when setting 
> > the
> > affinity (see irq_set_affinity) of an IRQ.
> > 
> > > If we tried to inject the same virtual interrupt on a
> > > different vcpu, while the interrupt is still inflight, we could get in
> > > troubles. But we could avoid that by testing for GIC_IRQ_GUEST_MIGRATING
> > > in vgic_vcpu_inject_irq. Maybe I am worrying about nothing.
> > 
> > The only interrupt that can be routed to a guest in Xen are SPI which are
> > shared between all CPUs. The active bit is handled by the distributor. It is
> > not possible to receive the same SPI until it has been deactivated.
> 
> True, but keep in mind that we don't get any interruptions when the vcpu
> issues an EOI. We lazily clean the data structures the first time we get
> back to Xen. So there is a window, where the interrupt has already been
> EOI'ed on the first vcpu, but struct pending_irq still shows the
> interrupt as inflight and would mess up today's checks in
> vgic_vcpu_inject_irq on other vcpus. Also they wouldn't be protected by
> the right vgic lock either. Maybe this is the real reason why I didn't
> go for this route originally. Sorry I didn't bring this up earlier, the
> irq migration stuff is extremely difficult to get right.

I couldn't figure out a good way to solve the problem going down this
route. Thus, I wrote a small patch series to solve the lock inversion
problem using the vgic lock technique (same as the previous patch).

However, if you can come up with a better approach, I'll be happy to
rewrite my patches.

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