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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.15 at 18:59, <david.vrabel@xxxxxxxxxx> 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


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