From 90d00db0949a8e796d7f812134753a54b2fe3d51 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Thu, 20 Nov 2014 14:28:11 -0500 Subject: [PATCH] dpci: Add 'masked' as an gate for hvm_dirq_assist to process. commit f6dd295381f4b6a66acddacf46bca8940586c8d8 "dpci: replace tasklet with softirq" used the 'masked' as an two-bit state mechanism (STATE_SCHED, STATE_RUN) to communicate between 'raise_softirq_for' and 'dpci_softirq' to determine whether the 'struct hvm_pirq_dpci' can be re-scheduled. However it ignored the 'pt_irq_guest_eoi' was not adhering to the proper dialogue and was not using locked cmpxchg or test_bit operations and ended setting 'state' set to zero. That meant 'raise_softirq_for' was free to schedule it while the 'struct hvm_pirq_dpci'' was still on an per-cpu list causing an list corruption. The code would trigger the following path causing list corruption: \-timer_softirq_action pt_irq_time_out calls pt_pirq_softirq_cancel sets state to 0. pirq_dpci is still on dpci_list. \- dpci_sofitrq while (!list_emptry(&our_list)) list_del, but has not yet done 'entry->next = LIST_POISON1;' [interrupt happens] raise_softirq checks state which is zero. Adds pirq_dpci to the dpci_list. [interrupt is done, back to dpci_softirq] finishes the entry->next = LIST_POISON1; .. test STATE_SCHED returns true, so executes the hvm_dirq_assist. ends the loop, exits. \- dpci_softirq while (!list_emtpry) list_del, but ->next already has LIST_POISON1 and we blow up. An alternative solution was proposed (adding STATE_ZOMBIE and making pt_irq_time_out use the cmpxchg protocol on 'state'), which fixed the above issue but had an fatal bug. It would miss interrupts that are to be scheduled! This patch brings back the 'masked' boolean which is used as an communication channel between 'hvm_do_IRQ_dpci', 'hvm_dirq_assist' and 'pt_irq_guest_eoi'. When we have an interrupt we set 'masked'. Anytime 'hvm_dirq_assist' or 'pt_irq_guest_eoi' executes - it clears it. The 'state' is left as a seperate mechanism to provide an mechanism between 'raise_sofitrq' and 'softirq_dpci' to communicate the state of the 'struct hvm_dirq_pirq'. However since we have now two seperate machines we have to deal with an cancellations and outstanding interrupt being serviced: 'pt_irq_destroy_bind' is called while an 'hvm_dirq_assist' is just about to service. The 'pt_irq_destroy_bind' takes the lock first and kills the timer - and the moment it releases the spinlock, 'hvm_dirq_assist' thunders in and calls 'set_timer' hitting an ASSERT. By clearing the 'masked' in the 'pt_irq_destroy_bind' we take care of that scenario by inhibiting 'hvm_dirq_assist' to call the 'set_timer'. In the 'pt_irq_create_bind' - in the error cases we could be seeing an softirq scheduled right away and being serviced (though stuck at the spinlock). The 'pt_irq_create_bind' fails in 'pt_pirq_softirq_reset' to change the 'state' (as the state is in 'STATE_RUN', not 'STATE_SCHED'). 'pt_irq_create_bind' continues on with setting '->flag=0' and unlocks the lock. 'hvm_dirq_assist' grabs the lock and continues one. Since 'flag = 0' and 'digl_list' is empty, it thunders through the 'hvm_dirq_assist' not doing anything until it hits 'set_timer' which is undefined for MSI. Adding in 'masked=0' for the MSI case fixes that. The legacy interrupt one does not need it as there is no chance of do_IRQ being called at that point. Reported-by: Sander Eikelenboom Signed-off-by: Konrad Rzeszutek Wilk v2: Deal with 'masked' in pt_irq_destroy_bind. v3: Deal with 'masked' in pt_irq_create_bind. --- xen/drivers/passthrough/io.c | 12 ++++++++++-- xen/include/xen/hvm/irq.h | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c index efc66dc..ae050df 100644 --- a/xen/drivers/passthrough/io.c +++ b/xen/drivers/passthrough/io.c @@ -129,6 +129,13 @@ static void pt_pirq_softirq_reset(struct hvm_pirq_dpci *pirq_dpci) pirq_dpci->dom = NULL; break; } + /* + * Inhibit 'hvm_dirq_assist' from doing anything useful and at worst + * calling 'set_timer' which will blow up (as we have called kill_timer + * or never initialized it). Note that we hold the lock that + * 'hvm_dirq_assist' could be spinning on. + */ + pirq_dpci->masked = 0; } bool_t pt_irq_need_timer(uint32_t flags) @@ -142,7 +149,7 @@ static int pt_irq_guest_eoi(struct domain *d, struct hvm_pirq_dpci *pirq_dpci, if ( __test_and_clear_bit(_HVM_IRQ_DPCI_EOI_LATCH_SHIFT, &pirq_dpci->flags) ) { - pirq_dpci->state = 0; + pirq_dpci->masked = 0; pirq_dpci->pending = 0; pirq_guest_eoi(dpci_pirq(pirq_dpci)); } @@ -610,6 +617,7 @@ int hvm_do_IRQ_dpci(struct domain *d, struct pirq *pirq) !(pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) ) return 0; + pirq_dpci->masked = 1; raise_softirq_for(pirq_dpci); return 1; } @@ -669,7 +677,7 @@ static void hvm_dirq_assist(struct domain *d, struct hvm_pirq_dpci *pirq_dpci) ASSERT(d->arch.hvm_domain.irq.dpci); spin_lock(&d->event_lock); - if ( pirq_dpci->state ) + if ( test_and_clear_bool(pirq_dpci->masked) ) { struct pirq *pirq = dpci_pirq(pirq_dpci); const struct dev_intx_gsi_link *digl; diff --git a/xen/include/xen/hvm/irq.h b/xen/include/xen/hvm/irq.h index 9709397..3996f1f 100644 --- a/xen/include/xen/hvm/irq.h +++ b/xen/include/xen/hvm/irq.h @@ -94,6 +94,7 @@ struct hvm_irq_dpci { struct hvm_pirq_dpci { uint32_t flags; unsigned int state; + bool_t masked; uint16_t pending; struct list_head digl_list; struct domain *dom; -- 1.9.3