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

[Keir Fraser <keir.xen@xxxxxxxxx>]

On 17/09/2011 05:50, "Jeremy Fitzhardinge" <jeremy@xxxxxxxx> wrote:

> 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.

The 'event channels' returned by /dev/evtchn are really 32-bit opaque
tokens, and can be whatever you want, as long as all kernel drivers are
updated to treat them consistently.

 -- Keir

> What's the downside of sending the event after the event channel has closed?
> 
> That said:
> 
>> 
>> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> ---
>>  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);
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel