Xen devel - Oct 2013 - [PATCH] evtchn: don't lose pending state if FIFO event array page is missing

From: David Vrabel <david.vrabel@citrix.com>

When the FIFO-based ABI is in use, if an event is bound when the
corresponding event array page is missing any attempt to set the event
pending will lose the event (because there is nowhere to write the
pending state).

This wasn''t initially considered an issue because guests were expected
to only bind events once they had expanded the event array, however:

1. A domain may start with events already bound (by the toolstack).

2. The guest does not know what the port number will be until the
   event is bound (it doesn''t know how many already bound events there
   are), so it does not know how many event array pages are required.
   This makes it difficult to expand in advanced (the current Linux
   implementation expands after binding for example).

To prevent pending events from being lost because there is no array
page, temporarily store the pending state in evtchn->pending.  When an
array page is added, use this state to set the port as pending.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/event_fifo.c |   50 +++++++++++++++++++++++++++++++++++++++++-----
 xen/include/xen/sched.h |    1 +
 2 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
index bec8d87..42ae423 100644
--- a/xen/common/event_fifo.c
+++ b/xen/common/event_fifo.c
@@ -61,8 +61,16 @@ static void evtchn_fifo_set_pending(struct vcpu *v, struct
evtchn *evtchn)
 
     port = evtchn->port;
     word = evtchn_fifo_word_from_port(d, port);
+
+    /*
+     * Event array page may not exist yet, save the pending state for
+     * when the page is added.
+     */
     if ( unlikely(!word) )
+    {
+        evtchn->pending = 1;
         return;
+    }
 
     /*
      * No locking around getting the queue. This may race with
@@ -322,16 +330,29 @@ static void cleanup_event_array(struct domain *d)
     xfree(d->evtchn_fifo);
 }
 
-static void set_priority_all(struct domain *d, unsigned int priority)
+static void setup_ports(struct domain *d)
 {
     unsigned int port;
 
+    /*
+     * For each port that is already bound:
+     *
+     * - save its pending state.
+     * - set default priority.
+     */
     for ( port = 1; port < d->max_evtchns; port++ )
     {
+        struct evtchn *evtchn;
+
         if ( !port_is_valid(d, port) )
             break;
 
-        evtchn_port_set_priority(d, evtchn_from_port(d, port), priority);
+        evtchn = evtchn_from_port(d, port);
+
+        if ( test_and_clear_bit(port, &shared_info(d, evtchn_pending)) )
+            evtchn->pending = 1;
+
+        evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
     }
 }
 
@@ -369,9 +390,6 @@ int evtchn_fifo_init_control(struct evtchn_init_control
*init_control)
     /*
      * If this is the first control block, setup an empty event array
      * and switch to the fifo port ops.
-     *
-     * Any ports currently bound will have their priority set to the
-     * default.
      */
     if ( rc == 0 && !d->evtchn_fifo )
     {
@@ -382,7 +400,7 @@ int evtchn_fifo_init_control(struct evtchn_init_control
*init_control)
         {
             d->evtchn_port_ops = &evtchn_port_ops_fifo;
             d->max_evtchns = EVTCHN_FIFO_NR_CHANNELS;
-            set_priority_all(d, EVTCHN_FIFO_PRIORITY_DEFAULT);
+            setup_ports(d);
         }
     }
 
@@ -395,6 +413,7 @@ static int add_page_to_event_array(struct domain *d,
unsigned long gfn)
 {
     void *virt;
     unsigned int slot;
+    unsigned int port = d->evtchn_fifo->num_evtchns;
     int rc;
 
     slot = d->evtchn_fifo->num_evtchns /
EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
@@ -408,6 +427,25 @@ static int add_page_to_event_array(struct domain *d,
unsigned long gfn)
     d->evtchn_fifo->event_array[slot] = virt;
     d->evtchn_fifo->num_evtchns += EVTCHN_FIFO_EVENT_WORDS_PER_PAGE;
 
+    /*
+     * Re-raise any events that were pending while this array page was
+     * missing.
+     */
+    for ( ; port < d->evtchn_fifo->num_evtchns; port++ )
+    {
+        struct evtchn *evtchn;
+
+        if ( !port_is_valid(d, port) )
+            break;
+
+        evtchn = evtchn_from_port(d, port);
+        if ( evtchn->pending )
+        {
+            evtchn_fifo_set_pending(d->vcpu[evtchn->notify_vcpu_id],
evtchn);
+            evtchn->pending = 0;
+        }
+    }
+
     return 0;
 }
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 25bf637..624ea15 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -97,6 +97,7 @@ struct evtchn
         u16 virq;      /* state == ECS_VIRQ */
     } u;
     u8 priority;
+    u8 pending:1;
 #ifdef FLASK_ENABLE
     void *ssid;
 #endif
-- 
1.7.2.5

>>> On 18.10.13 at 16:06, David Vrabel <david.vrabel@citrix.com>
wrote:
> From: David Vrabel <david.vrabel@citrix.com>
> 
> When the FIFO-based ABI is in use, if an event is bound when the
> corresponding event array page is missing any attempt to set the event
> pending will lose the event (because there is nowhere to write the
> pending state).
> 
> This wasn''t initially considered an issue because guests were
expected
> to only bind events once they had expanded the event array, however:
> 
> 1. A domain may start with events already bound (by the toolstack).
But a domain wouldn''t start in FIFO mode, would it?

Jan
> 2. The guest does not know what the port number will be until the
>    event is bound (it doesn''t know how many already bound events
there
>    are), so it does not know how many event array pages are required.
>    This makes it difficult to expand in advanced (the current Linux
>    implementation expands after binding for example).
> 
> To prevent pending events from being lost because there is no array
> page, temporarily store the pending state in evtchn->pending.  When an
> array page is added, use this state to set the port as pending.

On 18/10/13 15:41, Jan Beulich wrote:
>>>> On 18.10.13 at 16:06, David Vrabel
<david.vrabel@citrix.com> wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> When the FIFO-based ABI is in use, if an event is bound when the
>> corresponding event array page is missing any attempt to set the event
>> pending will lose the event (because there is nowhere to write the
>> pending state).
>>
>> This wasn''t initially considered an issue because guests were
expected
>> to only bind events once they had expanded the event array, however:
>>
>> 1. A domain may start with events already bound (by the toolstack).
> 
> But a domain wouldn''t start in FIFO mode, would it?
Yes, guests always start in 2-level mode, but if the guest doesn''t
unmask the events they should still be pending after switching to FIFO
mode so must copy the pending bits from the 2-level array and set them
pending in the into the FIFO event array (when the pages turn up).

David
>> 2. The guest does not know what the port number will be until the
>>    event is bound (it doesn''t know how many already bound
events there
>>    are), so it does not know how many event array pages are required.
>>    This makes it difficult to expand in advanced (the current Linux
>>    implementation expands after binding for example).
>>
>> To prevent pending events from being lost because there is no array
>> page, temporarily store the pending state in evtchn->pending.  When
an
>> array page is added, use this state to set the port as pending.

>>> On 18.10.13 at 16:06, David Vrabel <david.vrabel@citrix.com>
wrote:
> @@ -322,16 +330,29 @@ static void cleanup_event_array(struct domain *d)
>      xfree(d->evtchn_fifo);
>  }
>  
> -static void set_priority_all(struct domain *d, unsigned int priority)
> +static void setup_ports(struct domain *d)
>  {
>      unsigned int port;
>  
> +    /*
> +     * For each port that is already bound:
> +     *
> +     * - save its pending state.
> +     * - set default priority.
> +     */
>      for ( port = 1; port < d->max_evtchns; port++ )
>      {
> +        struct evtchn *evtchn;
> +
>          if ( !port_is_valid(d, port) )
>              break;
>  
> -        evtchn_port_set_priority(d, evtchn_from_port(d, port), priority);
> +        evtchn = evtchn_from_port(d, port);
> +
> +        if ( test_and_clear_bit(port, &shared_info(d, evtchn_pending))
)
So why test_and_clear rather than just test?

Jan
> +            evtchn->pending = 1;
> +
> +        evtchn_fifo_set_priority(d, evtchn, EVTCHN_FIFO_PRIORITY_DEFAULT);
>      }
>  }
>

On 18/10/13 16:06, Jan Beulich wrote:
>>>> On 18.10.13 at 16:06, David Vrabel
<david.vrabel@citrix.com> wrote:
>> @@ -322,16 +330,29 @@ static void cleanup_event_array(struct domain *d)
>>      xfree(d->evtchn_fifo);
>>  }
>>  
>> -static void set_priority_all(struct domain *d, unsigned int priority)
>> +static void setup_ports(struct domain *d)
>>  {
>>      unsigned int port;
>>  
>> +    /*
>> +     * For each port that is already bound:
>> +     *
>> +     * - save its pending state.
>> +     * - set default priority.
>> +     */
>>      for ( port = 1; port < d->max_evtchns; port++ )
>>      {
>> +        struct evtchn *evtchn;
>> +
>>          if ( !port_is_valid(d, port) )
>>              break;
>>  
>> -        evtchn_port_set_priority(d, evtchn_from_port(d, port),
priority);
>> +        evtchn = evtchn_from_port(d, port);
>> +
>> +        if ( test_and_clear_bit(port, &shared_info(d,
evtchn_pending)) )
> 
> So why test_and_clear rather than just test?
I thought of it as moving the bit, but it''s not necessary so
I''ll
replace it with a plain test_bit().

Similarly, the evtchn->pending = 0 isn''t needed either.

David