[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen/arm: fix rank/vgic locks inversion bug
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |