Re: [Xen-devel] [PATCH 2/3] evtchn: don't lose pending state if FIFO event array page is missing

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.10.13 at 16:03, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> From: David Vrabel <david.vrabel@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> ---
>  xen/common/event_fifo.c |   47 
> +++++++++++++++++++++++++++++++++++++++++------
>  xen/include/xen/sched.h |    1 +
>  2 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/event_fifo.c b/xen/common/event_fifo.c
> index bec8d87..64dbfff 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_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,22 @@ 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);
> +    }
> +
>      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
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 



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