Re: [PATCH 1/2] xen/event: Add reference counting to event channel

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 09/16/2011 02:14 PM, Daniel De Graaf wrote:
> Event channels exposed to userspace by the evtchn module may be used by
> other modules in an asynchronous manner, which requires that reference
> counting be used to prevent the event channel from being closed before
> the signals are delivered.

Could you use the refcounting at the irq level?  I was quite pleased to
have removed the event channel refcounting (and the use of naked event
channels).

Oh, is it that userspace allocates an event channel with /dev/evtchn,
then passes that event channel to the gntalloc/gntdev drivers so they
can use it to pass events between the two.  That's a bit unfortunate; it
might have been better to expose those event channels as file
descriptors so you could use fd refcounting to manage the lifetimes.

What's the downside of sending the event after the event channel has closed?

That said:

>
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/events.c |   38 ++++++++++++++++++++++++++++++++++++++
>  include/xen/events.h |    6 ++++++
>  2 files changed, 44 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index da70f5c..c9343b9 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -89,6 +89,7 @@ struct irq_info
>  {
>  	struct list_head list;
>  	enum xen_irq_type type;	/* type */
> +	unsigned short refcount;

Is short large enough?  Is this something that untrusted userspace could
end up wrapping?  If short is sufficient, you should pack it next to the
other short fields to avoid a gap.

>  	unsigned irq;
>  	unsigned short evtchn;	/* event channel */
>  	unsigned short cpu;	/* cpu bound */
> @@ -407,6 +408,7 @@ static void xen_irq_init(unsigned irq)
>  		panic("Unable to allocate metadata for IRQ%d\n", irq);
>  
>  	info->type = IRQT_UNBOUND;
> +	info->refcount = 1;
>  
>  	irq_set_handler_data(irq, info);
>  
> @@ -469,6 +471,8 @@ static void xen_free_irq(unsigned irq)
>  
>  	irq_set_handler_data(irq, NULL);
>  
> +	BUG_ON(info->refcount > 1);
> +
>  	kfree(info);
>  
>  	/* Legacy IRQ descriptors are managed by the arch. */
> @@ -912,9 +916,14 @@ static void unbind_from_irq(unsigned int irq)
>  {
>  	struct evtchn_close close;
>  	int evtchn = evtchn_from_irq(irq);
> +	struct irq_info *info = irq_get_handler_data(irq);
>  
>  	spin_lock(&irq_mapping_update_lock);
>  
> +	info->refcount--;
> +	if (info->refcount > 0)
> +		goto out_unlock;
> +
>  	if (VALID_EVTCHN(evtchn)) {
>  		close.port = evtchn;
>  		if (HYPERVISOR_event_channel_op(EVTCHNOP_close, &close) != 0)
> @@ -943,6 +952,7 @@ static void unbind_from_irq(unsigned int irq)
>  
>  	xen_free_irq(irq);
>  
> + out_unlock:
>  	spin_unlock(&irq_mapping_update_lock);
>  }
>  
> @@ -1038,6 +1048,34 @@ void unbind_from_irqhandler(unsigned int irq, void *dev_id)
>  }
>  EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
>  
> +int get_evtchn_reservation(unsigned int evtchn)
"reservation"?  I think just evtchn_get/put would be more consistent
with kernel naming conventions.

> +{
> +	int irq = evtchn_to_irq[evtchn];
> +	struct irq_info *info;
> +
> +	if (irq == -1)
> +		return -ENOENT;
> +
> +	info = irq_get_handler_data(irq);
> +
> +	if (!info)
> +		return -ENOENT;
> +
> +	spin_lock(&irq_mapping_update_lock);
> +	info->refcount++;
> +	spin_unlock(&irq_mapping_update_lock);

What is this spinlock protecting against?  The non-atomicity of ++, or
something larger scale?  If its just an atomicity thing, should it be an
atomic_t?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(get_evtchn_reservation);
> +
> +void put_evtchn_reservation(unsigned int evtchn)
> +{
> +	int irq = evtchn_to_irq[evtchn];
> +	unbind_from_irq(irq);

Hm.

> +}
> +EXPORT_SYMBOL_GPL(put_evtchn_reservation);
> +
>  void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector)
>  {
>  	int irq = per_cpu(ipi_to_irq, cpu)[vector];
> diff --git a/include/xen/events.h b/include/xen/events.h
> index d287997..23bd5fd 100644
> --- a/include/xen/events.h
> +++ b/include/xen/events.h
> @@ -37,6 +37,12 @@ int bind_interdomain_evtchn_to_irqhandler(unsigned int remote_domain,
>   */
>  void unbind_from_irqhandler(unsigned int irq, void *dev_id);
>  
> +/*
> + * Allow extra references to event channels exposed to userspace by evtchn
> + */
> +int get_evtchn_reservation(unsigned int evtchn);
> +void put_evtchn_reservation(unsigned int evtchn);
> +
>  void xen_send_IPI_one(unsigned int cpu, enum ipi_vector vector);
>  int resend_irq_on_evtchn(unsigned int irq);
>  void rebind_evtchn_irq(int evtchn, int irq);