WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

On Tue, Oct 18, 2011 at 05:04:10PM -0400, 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.

You should probably also remove the comment in "xen_bind_pirq_gsi_to_irq"
which talks about refcount (as the comment would now apply to this code and
might confuse people reading the code).

There are two scenarios I am concerned about:

 1). Xen pciback allocates/setups an physical IRQ on behalf of a guest. Lets
     concentrate on MSI as that is more interesting. The PV guests sends
     XEN_PCI_OP_enable_msi, dom0 calls pci_enable_msi(), MSI libs end up calling
     xen_initdom_setup_msi_irqs, which calls xen_bind_pirq_msi_to_irq and 
     irq->refcnt==2.

     Guest dies without calling XEN_PCI_OP_disable_msi, so we end up in
     xen_pcibk_reset_device which calls pci_disable_msi().. which calls 
xen_free_irq().
     And all of that sets refcnt==1.. OK, and if we do call 
xen_pcibk_reset_device()
     again it is smart enough _not_ to call pci_disable_msi() twice.

     So I guess that case is actually OK, but if there was a driver that 
decided to
     call pci_disable_msi (or pci_disable_irq) we could hit the BUG_ON(). 
Perhaps
     that should be altered to WARN_ON.

 2). Grantdev holding the refcnt forever. That is probably the easiest as it 
would
     be a bug in the code.

Hmm, I  think I've talked myself out of actually finding any cases where this 
would
be problematic from a design perspective. The only issue I can see is exposing 
bugs
in the users of event channel API - which there might be. So definitly needs 
some
heavy duty testing.

> 
> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> ---
>  drivers/xen/events.c |   34 ++++++++++++++++++++++++++++++++++
>  include/xen/events.h |    6 ++++++
>  2 files changed, 40 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 7523719..36d3390 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -88,6 +88,7 @@ enum xen_irq_type {
>  struct irq_info
>  {
>       struct list_head list;
> +     atomic_t refcount;

refcnt

>       enum xen_irq_type type; /* type */
>       unsigned irq;
>       unsigned short evtchn;  /* event channel */
> @@ -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;
> +     atomic_set(&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(atomic_read(&info->refcount) > 1);
> +
>       kfree(info);
>  
>       /* Legacy IRQ descriptors are managed by the arch. */
> @@ -912,6 +916,10 @@ 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);
> +
> +     if (!atomic_dec_and_test(&info->refcount))
> +             return;
>  
>       mutex_lock(&irq_mapping_update_lock);
>  
> @@ -1038,6 +1046,32 @@ void unbind_from_irqhandler(unsigned int irq, void 
> *dev_id)
>  }
>  EXPORT_SYMBOL_GPL(unbind_from_irqhandler);
>  
> +int evtchn_get(unsigned int evtchn)
> +{
> +     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;
> +
> +     atomic_inc(&info->refcount);
> +
> +     return 0;
> +}
> +EXPORT_SYMBOL_GPL(evtchn_get);
> +
> +void evtchn_put(unsigned int evtchn)

The decleration for 'evtchn' is 'unsigned short' so that can be
used instead of 'unsigned int'.

> +{
> +     int irq = evtchn_to_irq[evtchn];

Not checking if the irq is valid? Or if the evtchn is valid?

> +     unbind_from_irq(irq);
> +}
> +EXPORT_SYMBOL_GPL(evtchn_put);
> +
>  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..a459cca 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 evtchn_get(unsigned int evtchn);
> +void evtchn_put(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);
> -- 
> 1.7.6.4

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