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

[PATCHv1] 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 permanently unusable.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/event_2l.c      |  7 +++++++
 xen/common/event_channel.c |  3 ++-
 xen/common/event_fifo.c    | 13 +++++++++++++
 xen/include/xen/event.h    | 11 +++++++++++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/xen/common/event_2l.c b/xen/common/event_2l.c
index 5837ae8..0ccc59f 100644
--- a/xen/common/event_2l.c
+++ b/xen/common/event_2l.c
@@ -74,6 +74,12 @@ static bool_t evtchn_2l_is_masked(struct domain *d,
     return test_bit(evtchn->port, &shared_info(d, evtchn_mask));
 }
 
+static bool_t evtchn_2l_is_busy(struct domain *d,
+                                const struct evtchn *evtchn)
+{
+    return 0;
+}
+
 static void evtchn_2l_print_state(struct domain *d,
                                   const struct evtchn *evtchn)
 {
@@ -90,6 +96,7 @@ static const struct evtchn_port_ops evtchn_port_ops_2l =
     .unmask        = evtchn_2l_unmask,
     .is_pending    = evtchn_2l_is_pending,
     .is_masked     = evtchn_2l_is_masked,
+    .is_busy       = evtchn_2l_is_busy,
     .print_state   = evtchn_2l_print_state,
 };
 
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 5a529a6..b1d4e47 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 )
+        chn = evtchn_from_port(d, port);
+        if ( chn->state == ECS_FREE && !evtchn_port_is_busy(d, chn) )
             return port;
     }
 
diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index c9b7884..4157318 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -312,6 +312,18 @@ 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,
+                                  const struct evtchn *evtchn)
+{
+    event_word_t *word;
+
+    word = evtchn_fifo_word_from_port(d, evtchn->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 +363,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..b754f08 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 its still being cleaned up
+     * after being closed?
+     */
+    bool_t (*is_busy)(struct domain *d, const struct evtchn *evtchn);
     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,12 @@ 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,
+                                         const struct evtchn *evtchn)
+{
+    return d->evtchn_port_ops->is_busy(d, evtchn);
+}
+
 static inline int evtchn_port_set_priority(struct domain *d,
                                            struct evtchn *evtchn,
                                            unsigned int priority)
-- 
2.1.4

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

[Jan Beulich]

>>> On 30.11.15 at 18: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.

I agree with the reasoning, but see below.

> --- a/xen/common/event_2l.c
> +++ b/xen/common/event_2l.c
> @@ -74,6 +74,12 @@ static bool_t evtchn_2l_is_masked(struct domain *d,
>      return test_bit(evtchn->port, &shared_info(d, evtchn_mask));
>  }
>  
> +static bool_t evtchn_2l_is_busy(struct domain *d,
> +                                const struct evtchn *evtchn)
> +{
> +    return 0;
> +}
> +
>  static void evtchn_2l_print_state(struct domain *d,
>                                    const struct evtchn *evtchn)
>  {
> @@ -90,6 +96,7 @@ static const struct evtchn_port_ops evtchn_port_ops_2l =
>      .unmask        = evtchn_2l_unmask,
>      .is_pending    = evtchn_2l_is_pending,
>      .is_masked     = evtchn_2l_is_masked,
> +    .is_busy       = evtchn_2l_is_busy,
>      .print_state   = evtchn_2l_print_state,
>  };

Perhaps better to avoid introduction of the function by having the
wrapper check for NULL?

> --- 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 )
> +        chn = evtchn_from_port(d, port);
> +        if ( chn->state == ECS_FREE && !evtchn_port_is_busy(d, chn) )

Despite the reasonable arguments you give this looks very wrong:
How can a free port still be busy? Could we have a new ECS_* and
require guests to notify the hypervisor when they unlinked an
already closed port (while "close" would transition busy ports into
that new state)?

Jan

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

[David Vrabel]

On 01/12/15 12:49, Jan Beulich wrote:
>>>> On 30.11.15 at 18:59, <david.vrabel@citrix.com> wrote:
>> --- 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 )
>> +        chn = evtchn_from_port(d, port);
>> +        if ( chn->state == ECS_FREE && !evtchn_port_is_busy(d, chn) )
> 
> Despite the reasonable arguments you give this looks very wrong:
> How can a free port still be busy? Could we have a new ECS_* and
> require guests to notify the hypervisor when they unlinked an
> already closed port (while "close" would transition busy ports into
> that new state)?

I would look at it as: The channel object is free, but the corresponding
ABI specific port object is busy.  So it doesn't seem unreasonable to
check the state of both objects.

What you suggest (adding an additional call) would break all existing
guests that would not make the unlinked call, leaving the event channel
in a state where it cannot be reused.

David

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

[Jan Beulich]

>>> On 01.12.15 at 15:04, <david.vrabel@citrix.com> wrote:
> On 01/12/15 12:49, Jan Beulich wrote:
>>>>> On 30.11.15 at 18:59, <david.vrabel@citrix.com> wrote:
>>> --- 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 )
>>> +        chn = evtchn_from_port(d, port);
>>> +        if ( chn->state == ECS_FREE && !evtchn_port_is_busy(d, chn) )
>> 
>> Despite the reasonable arguments you give this looks very wrong:
>> How can a free port still be busy? Could we have a new ECS_* and
>> require guests to notify the hypervisor when they unlinked an
>> already closed port (while "close" would transition busy ports into
>> that new state)?
> 
> I would look at it as: The channel object is free, but the corresponding
> ABI specific port object is busy.  So it doesn't seem unreasonable to
> check the state of both objects.

While I think they're really tied together (namely due to how
get_free_port() works), yes - that's a way to view it, taking the
tying together as an implementation detail.

However, in that case it seems wrong to pass the channel pointer
to the is_busy() hook.

> What you suggest (adding an additional call) would break all existing
> guests that would not make the unlinked call, leaving the event channel
> in a state where it cannot be reused.

True.

Jan