Hello, this is trying to make the point that having NULL pointer entries in the radix tree and letting Xen handle that is a valid case. To some degree this serves as a tag (LPI has been unmapped), so should semantically come close to what we are discussing. It has the big advantage of being able to immediately clean up the pending_irq and free it right away.
So I checked all call sites of irq_to_pending (see below). A number of them is *not* used for LPIs (but only for SPIs, for instance). An LPI number will never make it to those places. A number of calls is already protected in patch 10/34 in the v7, and I just added one case. Interestingly all those calls use the retrieved pending_irq pointer either already under the VCPU VGIC lock or can be easily moved under it without changing any semantics. Since irq_to_pending right now was just a wrapper around some static array lookup, so far we just didn't bother with taking the lock before or after the irq_to_pending call. Changing this now doesn't change anything for the existing code. So this means that the pending_irq pointer is only retrieved and used under the lock - which gives us the guarantee that it's always valid to use. So as long as we hold the VGIC VCPU lock while we replace the radix tree entry we can't never have a use-after-free race. So this is the summary based on "git grep irq_to_pending": xen/arch/arm/gic.c: gic_route_irq_to_guest(): only called for SPIs gic_remove_irq_from_guest(): only called for SPIs gic_remove_from_queues(): PROTECTED, VCPU VGIC lock gic_raise_inflight_irq(): PROTECTED, VCPU VGIC lock gic_raise_guest_irq(): TO BE PROTECTED, VCPU VGIC lock gic_update_one_lr(): PROTECTED, VCPU VGIC lock xen/arch/arm/vgic.c: vgic_migrate_irq(): not called for LPIs (not for virtual IRQs) arch_move_irqs(): not iterating over LPIs vgic_disable_irqs(): not called for LPIs vgic_enable_irqs(): not called for LPIs vgic_vcpu_inject_irq(): PROTECTED, moved under VCPU VGIC lock xen/include/asm-arm/event.h: local_events_need_delivery_nomask(): only called for a PPI xen/include/asm-arm/vgic.h: (prototype) So here is the suggestion (yet another one, I know): - DISCARD translates the DevID/EvID pair to a vCPU/vLPI pair. This is done under the vcmd lock and ITS lock. We then lock the respective vCPU VGIC lock, take the radix tree lock and remove the pending_irq from the radix tree (making it read NULL), drop the radix tree lock and vCPU VGIC lock again. We clean up the pending_irq (removing from lists) under the lock still. Any VGIC code either sees a valid pending_irq pointer (and uses it only while holding the vCPU lock!) or a NULL pointer, and due to patch 10/34 can deal with this. A VCPU returning will just clear the LR when it reads NULL (because there is nothing to handle). - MAPD(V=0) just does the above in a loop with every mapped event. At the end it can safely free the pend_irq array. I implemented that along with addressing the other comments and pushed this to http://www.linux-arm.org/git?p=xen-ap.git;a=shortlog;h=refs/heads/its/v8-pre Maybe we can use this as a discussion base? Cheers, Andre. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
