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