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