Re: [Xen-devel] [for-xen-4.5 PATCH v2 2/2] dpci: Add ZOMBIE state to allow the softirq to finish with the dpci_pirq.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Thu, Nov 20, 2014 at 11:55:43AM +0100, Jan Beulich wrote:
> >>> On 19.11.14 at 23:21, <konrad.wilk@xxxxxxxxxx> wrote:
> 
> Leaving aside the question of whether this is the right approach, in
> case it is a couple of comments:
> 
> > @@ -85,7 +91,7 @@ static void raise_softirq_for(struct hvm_pirq_dpci 
> > *pirq_dpci)
> >   */
> >  bool_t pt_pirq_softirq_active(struct hvm_pirq_dpci *pirq_dpci)
> >  {
> > -    if ( pirq_dpci->state & ((1 << STATE_RUN) | (1 << STATE_SCHED)) )
> > +    if ( pirq_dpci->state & ((1 << STATE_RUN) | (1 << STATE_SCHED) | (1 << 
> > STATE_ZOMBIE) ) )
> 
> This is becoming unwieldy - perhaps better just "if ( pirq_dpci->state )"?
> 
> > @@ -122,6 +128,7 @@ static void pt_pirq_softirq_cancel(struct hvm_pirq_dpci 
> > *pirq_dpci,
> >          /* fallthrough. */
> >      case (1 << STATE_RUN):
> >      case (1 << STATE_RUN) | (1 << STATE_SCHED):
> > +    case (1 << STATE_RUN) | (1 << STATE_SCHED) | (1 << STATE_ZOMBIE):
> 
> How would we get into this state? Afaict STATE_ZOMBIE can't be set
> at the same time as STATE_SCHED.
> 
> > @@ -786,6 +793,7 @@ unlock:
> >  static void dpci_softirq(void)
> >  {
> >      unsigned int cpu = smp_processor_id();
> > +    unsigned int reset = 0;
> 
> bool_t (and would better be inside the loop, avoiding the pointless
> re-init on the last iteration).
> 
> But taking it as a whole - aren't we now at risk of losing an interrupt
> instance the guest expects, due to raise_softirq_for() bailing on
> zombie entries, and dpci_softirq() being the only place where the
> zombie state gets cleared?

Ah crud.

So a simple fix could be to seperate the 'state' to only deal with the
raise_softirq and softirq_dpci. And then add a new (old) 'masked' to
deal between hvm_dirq_assist, pt_irq_guest_eoi and hvm_do_IRQ_dpci.


From 94a98e20a8ab721a58788919f92e3524a6c6e25c Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
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'.

Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
---
 xen/drivers/passthrough/io.c | 5 +++--
 xen/include/xen/hvm/irq.h    | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index efc66dc..4d8c02f 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -142,7 +142,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 +610,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 +670,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

> 
> Jan
> 

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