[PATCH] xen: use the trigger info we already have to choose the irq handler

[PATCH] xen: use the trigger info we already have to choose the irq handler

[Stefano Stabellini]

Do not use pirq_needs_eoi to decide which irq handler to use because Xen
always returns true if the guest does not support pirq_eoi_map.
Use the trigger information we already have from MP-tables and ACPI.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reported-by: Thomas Goetz <tcgoetz@gmail.com>
Tested-by: Thomas Goetz <tcgoetz@gmail.com>

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 7f676f8..8418398 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -627,6 +627,9 @@ int xen_allocate_pirq_gsi(unsigned gsi)
  *
  * Note: We don't assign an event channel until the irq actually started
  * up.  Return an existing irq if we've already got one for the gsi.
+ *
+ * Shareable implies level triggered, not shareable implies edge
+ * triggered here.
  */
 int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 			     unsigned pirq, int shareable, char *name)
@@ -665,16 +668,13 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 
 	pirq_query_unmask(irq);
 	/* We try to use the handler with the appropriate semantic for the
-	 * type of interrupt: if the interrupt doesn't need an eoi
-	 * (pirq_needs_eoi returns false), we treat it like an edge
-	 * triggered interrupt so we use handle_edge_irq.
-	 * As a matter of fact this only happens when the corresponding
-	 * physical interrupt is edge triggered or an msi.
+	 * type of interrupt: if the interrupt is an edge triggered
+	 * interrupt we use handle_edge_irq.
 	 *
-	 * On the other hand if the interrupt needs an eoi (pirq_needs_eoi
-	 * returns true) we treat it like a level triggered interrupt so we
-	 * use handle_fasteoi_irq like the native code does for this kind of
+	 * On the other hand if the interrupt is level triggered we use
+	 * handle_fasteoi_irq like the native code does for this kind of
 	 * interrupts.
+	 *
 	 * Depending on the Xen version, pirq_needs_eoi might return true
 	 * not only for level triggered interrupts but for edge triggered
 	 * interrupts too. In any case Xen always honors the eoi mechanism,
@@ -682,7 +682,7 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	 * hasn't received an eoi yet. Therefore using the fasteoi handler
 	 * is the right choice either way.
 	 */
-	if (pirq_needs_eoi(irq))
+	if (shareable)
 		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
 				handle_fasteoi_irq, name);
 	else

[PATCH] xen: use the trigger info we already have to choose the irq handler

[Stefano Stabellini]

Do not use pirq_needs_eoi to decide which irq handler to use because Xen
always returns true if the guest does not support pirq_eoi_map.
Use the trigger information we already have from MP-tables and ACPI.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reported-by: Thomas Goetz <tcgoetz@gmail.com>
Tested-by: Thomas Goetz <tcgoetz@gmail.com>

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 7f676f8..8418398 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -627,6 +627,9 @@ int xen_allocate_pirq_gsi(unsigned gsi)
  *
  * Note: We don't assign an event channel until the irq actually started
  * up.  Return an existing irq if we've already got one for the gsi.
+ *
+ * Shareable implies level triggered, not shareable implies edge
+ * triggered here.
  */
 int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 			     unsigned pirq, int shareable, char *name)
@@ -665,16 +668,13 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 
 	pirq_query_unmask(irq);
 	/* We try to use the handler with the appropriate semantic for the
-	 * type of interrupt: if the interrupt doesn't need an eoi
-	 * (pirq_needs_eoi returns false), we treat it like an edge
-	 * triggered interrupt so we use handle_edge_irq.
-	 * As a matter of fact this only happens when the corresponding
-	 * physical interrupt is edge triggered or an msi.
+	 * type of interrupt: if the interrupt is an edge triggered
+	 * interrupt we use handle_edge_irq.
 	 *
-	 * On the other hand if the interrupt needs an eoi (pirq_needs_eoi
-	 * returns true) we treat it like a level triggered interrupt so we
-	 * use handle_fasteoi_irq like the native code does for this kind of
+	 * On the other hand if the interrupt is level triggered we use
+	 * handle_fasteoi_irq like the native code does for this kind of
 	 * interrupts.
+	 *
 	 * Depending on the Xen version, pirq_needs_eoi might return true
 	 * not only for level triggered interrupts but for edge triggered
 	 * interrupts too. In any case Xen always honors the eoi mechanism,
@@ -682,7 +682,7 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	 * hasn't received an eoi yet. Therefore using the fasteoi handler
 	 * is the right choice either way.
 	 */
-	if (pirq_needs_eoi(irq))
+	if (shareable)
 		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
 				handle_fasteoi_irq, name);
 	else

Re: [PATCH] xen: use the trigger info we already have to choose the irq handler

[Konrad Rzeszutek Wilk]

On Wed, May 25, 2011 at 12:33:23PM +0100, Stefano Stabellini wrote:
> Do not use pirq_needs_eoi to decide which irq handler to use because Xen
> always returns true if the guest does not support pirq_eoi_map.
> Use the trigger information we already have from MP-tables and ACPI.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Reported-by: Thomas Goetz <tcgoetz@gmail.com>
> Tested-by: Thomas Goetz <tcgoetz@gmail.com>

Tom,

Do you prefer those email addresses or tom.goetz@virtualcomputer.com ?
..
> -	if (pirq_needs_eoi(irq))
> +	if (shareable)

Ok, so for dom0, the sharable flag is determined by the ACPI parsing.. good.
And for MSI/MSI-X we use xen_bind_pirq_msi_to_irq which sets it to use handle_edge_irq.

The only hazard which you asked me on IRC is for PV guests with Xen PCI front.
In those cases, we have no idea whether the "GSI" IRQ is edge or level and we
just assume that anything under 16 is edge. Which is 99% right, except if you
are to pass in the ACPI IRQ to the guest, or some other weird creature. The MSI/MSI-X
part of the PCI front are OK too - they use the xen_bind_pirq_msi_to_irq call.

So, putting on the todo list an extension to the PCI PV protocol to pass information
about the GSI's - whether they are edge or level.

Patch is on the 2.6.40-rc1 train.

Re: [PATCH] xen: use the trigger info we already have to choose the irq handler

[Konrad Rzeszutek Wilk]

On Wed, May 25, 2011 at 12:33:23PM +0100, Stefano Stabellini wrote:
> Do not use pirq_needs_eoi to decide which irq handler to use because Xen
> always returns true if the guest does not support pirq_eoi_map.
> Use the trigger information we already have from MP-tables and ACPI.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Reported-by: Thomas Goetz <tcgoetz@gmail.com>
> Tested-by: Thomas Goetz <tcgoetz@gmail.com>

Tom,

Do you prefer those email addresses or tom.goetz@virtualcomputer.com ?
..
> -	if (pirq_needs_eoi(irq))
> +	if (shareable)

Ok, so for dom0, the sharable flag is determined by the ACPI parsing.. good.
And for MSI/MSI-X we use xen_bind_pirq_msi_to_irq which sets it to use handle_edge_irq.

The only hazard which you asked me on IRC is for PV guests with Xen PCI front.
In those cases, we have no idea whether the "GSI" IRQ is edge or level and we
just assume that anything under 16 is edge. Which is 99% right, except if you
are to pass in the ACPI IRQ to the guest, or some other weird creature. The MSI/MSI-X
part of the PCI front are OK too - they use the xen_bind_pirq_msi_to_irq call.

So, putting on the todo list an extension to the PCI PV protocol to pass information
about the GSI's - whether they are edge or level.

Patch is on the 2.6.40-rc1 train.

Re: [PATCH] xen: use the trigger info we already have to choose the irq handler

[Tom Goetz]

On May 25, 2011, at 10:25 AM, Konrad Rzeszutek Wilk wrote:

> On Wed, May 25, 2011 at 12:33:23PM +0100, Stefano Stabellini wrote:
>> Do not use pirq_needs_eoi to decide which irq handler to use because Xen
>> always returns true if the guest does not support pirq_eoi_map.
>> Use the trigger information we already have from MP-tables and ACPI.
>> 
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Reported-by: Thomas Goetz <tcgoetz@gmail.com>
>> Tested-by: Thomas Goetz <tcgoetz@gmail.com>
> 
> Tom,
> 
> Do you prefer those email addresses or tom.goetz@virtualcomputer.com ?

It doesn't matter to me. I'm signed up to Xen devel with the gmail address because I signed up before we had a domain for the company and never bothered to change it.


> ..
>> -	if (pirq_needs_eoi(irq))
>> +	if (shareable)
> 
> Ok, so for dom0, the sharable flag is determined by the ACPI parsing.. good.
> And for MSI/MSI-X we use xen_bind_pirq_msi_to_irq which sets it to use handle_edge_irq.
> 
> The only hazard which you asked me on IRC is for PV guests with Xen PCI front.
> In those cases, we have no idea whether the "GSI" IRQ is edge or level and we
> just assume that anything under 16 is edge. Which is 99% right, except if you
> are to pass in the ACPI IRQ to the guest, or some other weird creature. The MSI/MSI-X
> part of the PCI front are OK too - they use the xen_bind_pirq_msi_to_irq call.
> 
> So, putting on the todo list an extension to the PCI PV protocol to pass information
> about the GSI's - whether they are edge or level.
> 
> Patch is on the 2.6.40-rc1 train.