[PATCHv2] evtchn: don't reuse ports that are still "busy"

[PATCHv2] evtchn: don't reuse ports that are still "busy"

[David Vrabel]

When using the FIFO ABI a guest may close an event channel that is
still LINKED.  If this port is reused, subsequent events may be lost
because they may become pending on the wrong queue.

This could be fixed by requiring guests to only close event channels
that are not linked.  This is difficult since: a) irq cleanup in the
guest may be done in a context that cannot wait for the event to be
unlinked; b) the guest may attempt to rebind a PIRQ whose previous
close is still pending; and c) existing guests already have the
problematic behaviour.

Instead, simply check a port is not "busy" (i.e., it's not linked)
before reusing it.

Guests should still drain any queues for VCPUs that are being
offlined, or the port will become unusable until the VCPU is onlined
and starts processing events again.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v2:
- Pass port number to evtchn_port_is_busy().
- No is_busy method => never busy.
---
 xen/common/event_channel.c |  3 ++-
 xen/common/event_fifo.c    | 12 ++++++++++++
 xen/include/xen/event.h    | 12 ++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 5a529a6..638dc5e 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -170,7 +170,8 @@ static int get_free_port(struct domain *d)
     {
         if ( port > d->max_evtchn_port )
             return -ENOSPC;
-        if ( evtchn_from_port(d, port)->state == ECS_FREE )
+        if ( evtchn_from_port(d, port)->state == ECS_FREE
+             && !evtchn_port_is_busy(d, port) )
             return port;
     }
 
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index c9b7884..2ef07fd 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -312,6 +312,17 @@ static bool_t evtchn_fifo_is_masked(struct domain *d,
     return test_bit(EVTCHN_FIFO_MASKED, word);
 }
 
+static bool_t evtchn_fifo_is_busy(struct domain *d, u32 port)
+{
+    event_word_t *word;
+
+    word = evtchn_fifo_word_from_port(d, port);
+    if ( unlikely(!word) )
+        return 0;
+
+    return test_bit(EVTCHN_FIFO_LINKED, word);
+}
+
 static int evtchn_fifo_set_priority(struct domain *d, struct evtchn *evtchn,
                                     unsigned int priority)
 {
@@ -351,6 +362,7 @@ static const struct evtchn_port_ops evtchn_port_ops_fifo =
     .unmask        = evtchn_fifo_unmask,
     .is_pending    = evtchn_fifo_is_pending,
     .is_masked     = evtchn_fifo_is_masked,
+    .is_busy       = evtchn_fifo_is_busy,
     .set_priority  = evtchn_fifo_set_priority,
     .print_state   = evtchn_fifo_print_state,
 };
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index b87924a..7c216f1 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -139,6 +139,11 @@ struct evtchn_port_ops {
     void (*unmask)(struct domain *d, struct evtchn *evtchn);
     bool_t (*is_pending)(struct domain *d, const struct evtchn *evtchn);
     bool_t (*is_masked)(struct domain *d, const struct evtchn *evtchn);
+    /*
+     * Is the port unavailable because it's still being cleaned up
+     * after being closed?
+     */
+    bool_t (*is_busy)(struct domain *d, u32 port);
     int (*set_priority)(struct domain *d, struct evtchn *evtchn,
                         unsigned int priority);
     void (*print_state)(struct domain *d, const struct evtchn *evtchn);
@@ -181,6 +186,13 @@ static inline bool_t evtchn_port_is_masked(struct domain *d,
     return d->evtchn_port_ops->is_masked(d, evtchn);
 }
 
+static inline bool_t evtchn_port_is_busy(struct domain *d, u32 port)
+{
+    if ( d->evtchn_port_ops->is_busy )
+        return d->evtchn_port_ops->is_busy(d, port);
+    return 0;
+}
+
 static inline int evtchn_port_set_priority(struct domain *d,
                                            struct evtchn *evtchn,
                                            unsigned int priority)
-- 
2.1.4

Re: [PATCHv2] evtchn: don't reuse ports that are still "busy"

[Jan Beulich]

>>> On 01.12.15 at 15:59, <david.vrabel@citrix.com> wrote:
> When using the FIFO ABI a guest may close an event channel that is
> still LINKED.  If this port is reused, subsequent events may be lost
> because they may become pending on the wrong queue.
> 
> This could be fixed by requiring guests to only close event channels
> that are not linked.  This is difficult since: a) irq cleanup in the
> guest may be done in a context that cannot wait for the event to be
> unlinked; b) the guest may attempt to rebind a PIRQ whose previous
> close is still pending; and c) existing guests already have the
> problematic behaviour.
> 
> Instead, simply check a port is not "busy" (i.e., it's not linked)
> before reusing it.
> 
> Guests should still drain any queues for VCPUs that are being
> offlined, or the port will become unusable until the VCPU is onlined
> and starts processing events again.
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one remark (no reason to cut another version):

> --- a/xen/include/xen/event.h
> +++ b/xen/include/xen/event.h
> @@ -139,6 +139,11 @@ struct evtchn_port_ops {
>      void (*unmask)(struct domain *d, struct evtchn *evtchn);
>      bool_t (*is_pending)(struct domain *d, const struct evtchn *evtchn);
>      bool_t (*is_masked)(struct domain *d, const struct evtchn *evtchn);
> +    /*
> +     * Is the port unavailable because it's still being cleaned up
> +     * after being closed?
> +     */
> +    bool_t (*is_busy)(struct domain *d, u32 port);

I realize there's a lot of u32-s for port numbers, but I think we
should really get used to using evtchn_port_t for those.

Jan

Re: [PATCHv2] evtchn: don't reuse ports that are still "busy"

[David Vrabel]

On 01/12/15 15:51, Jan Beulich wrote:
> 
> I realize there's a lot of u32-s for port numbers, but I think we
> should really get used to using evtchn_port_t for those.

I checked the type in struct evtchn (which is u32) but evtchn_port_t is
better.

Thanks.

David