[PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[Stefano Stabellini]

xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

Switch virqs and pirqs that don't need EOI (in Xen acktype ==
ACKTYPE_NONE, that means the machine irq is actually edge)
to handle_edge_irq.

Use handle_fasteoi_irq for pirqs that need eoi (they generally
correspond to level triggered irqs), no risk in loosing interrupts
because we have to EOI the irq anyway.

This change has the following benefits:

- it uses the very same handlers that Linux would use on native for the
same irqs;

- it uses these handlers in the same way Linux would use them: it let
Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
the irq;

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 02b5a9c..4f15dde 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -118,6 +118,8 @@ static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
 static struct irq_chip xen_dynamic_chip;
 static struct irq_chip xen_percpu_chip;
 static struct irq_chip xen_pirq_chip;
+static void enable_dynirq(struct irq_data *data);
+static void disable_dynirq(struct irq_data *data);
 
 /* Get info for IRQ */
 static struct irq_info *info_for_irq(unsigned irq)
@@ -473,16 +475,6 @@ static void xen_free_irq(unsigned irq)
 	irq_free_desc(irq);
 }
 
-static void pirq_unmask_notify(int irq)
-{
-	struct physdev_eoi eoi = { .irq = pirq_from_irq(irq) };
-
-	if (unlikely(pirq_needs_eoi(irq))) {
-		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
-		WARN_ON(rc);
-	}
-}
-
 static void pirq_query_unmask(int irq)
 {
 	struct physdev_irq_status_query irq_status;
@@ -506,6 +498,30 @@ static bool probing_irq(int irq)
 	return desc && desc->action == NULL;
 }
 
+static void eoi_pirq(struct irq_data *data)
+{
+	int evtchn = evtchn_from_irq(data->irq);
+ 	struct irq_info *info = info_for_irq(data->irq);
+ 	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
+	int rc = 0;
+
+	move_native_irq(data->irq);
+
+	if (VALID_EVTCHN(evtchn))
+		clear_evtchn(evtchn);
+
+	if (pirq_needs_eoi(data->irq)) {
+		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
+		WARN_ON(rc);
+	}
+}
+
+static void mask_ack_pirq(struct irq_data *data)
+{
+	disable_dynirq(data);
+	eoi_pirq(data);
+}
+ 
 static unsigned int __startup_pirq(unsigned int irq)
 {
 	struct evtchn_bind_pirq bind_pirq;
@@ -539,7 +555,7 @@ static unsigned int __startup_pirq(unsigned int irq)
 
 out:
 	unmask_evtchn(evtchn);
-	pirq_unmask_notify(irq);
+	eoi_pirq(irq_get_irq_data(irq));
 
 	return 0;
 }
@@ -572,27 +588,6 @@ static void shutdown_pirq(struct irq_data *data)
 	info->evtchn = 0;
 }
 
-static void enable_pirq(struct irq_data *data)
-{
-	startup_pirq(data);
-}
-
-static void disable_pirq(struct irq_data *data)
-{
-}
-
-static void ack_pirq(struct irq_data *data)
-{
-	int evtchn = evtchn_from_irq(data->irq);
-
-	move_native_irq(data->irq);
-
-	if (VALID_EVTCHN(evtchn)) {
-		mask_evtchn(evtchn);
-		clear_evtchn(evtchn);
-	}
-}
-
 static int find_irq_by_gsi(unsigned gsi)
 {
 	struct irq_info *info;
@@ -639,9 +634,6 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	if (irq < 0)
 		goto out;
 
-	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
-				      handle_level_irq, name);
-
 	irq_op.irq = irq;
 	irq_op.vector = 0;
 
@@ -658,6 +650,14 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	xen_irq_info_pirq_init(irq, 0, pirq, gsi, irq_op.vector,
 			       shareable ? PIRQ_SHAREABLE : 0);
 
+	pirq_query_unmask(irq);
+	if (pirq_needs_eoi(irq))
+		set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
+				handle_fasteoi_irq, name);
+	else
+		set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
+				handle_edge_irq, name);
+
 out:
 	spin_unlock(&irq_mapping_update_lock);
 
@@ -691,7 +691,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 		goto out;
 
 	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
-				      handle_level_irq, name);
+				      handle_edge_irq, name);
 
 	xen_irq_info_pirq_init(irq, 0, pirq, 0, vector, 0);
 	ret = irq_set_msi_desc(irq, msidesc);
@@ -773,7 +773,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 			goto out;
 
 		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_fasteoi_irq, "event");
+					      handle_edge_irq, "event");
 
 		xen_irq_info_evtchn_init(irq, evtchn);
 	}
@@ -1181,9 +1181,6 @@ static void __xen_evtchn_do_upcall(void)
 				port = (word_idx * BITS_PER_LONG) + bit_idx;
 				irq = evtchn_to_irq[port];
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1339,12 +1336,18 @@ static void ack_dynirq(struct irq_data *data)
 {
 	int evtchn = evtchn_from_irq(data->irq);
 
-	move_masked_irq(data->irq);
+	move_native_irq(data->irq);
 
 	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+		clear_evtchn(evtchn);
 }
 
+static void mask_ack_dynirq(struct irq_data *data)
+{
+	disable_dynirq(data);
+	ack_dynirq(data);
+}
+ 
 static int retrigger_dynirq(struct irq_data *data)
 {
 	int evtchn = evtchn_from_irq(data->irq);
@@ -1533,11 +1536,12 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name			= "xen-dyn",
 
-	.irq_disable		= disable_dynirq,
 	.irq_mask		= disable_dynirq,
 	.irq_unmask		= enable_dynirq,
 
-	.irq_eoi		= ack_dynirq,
+	.irq_ack		= ack_dynirq,
+	.irq_mask_ack		= mask_ack_dynirq,
+
 	.irq_set_affinity	= set_affinity_irq,
 	.irq_retrigger		= retrigger_dynirq,
 };
@@ -1548,13 +1552,12 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.irq_startup		= startup_pirq,
 	.irq_shutdown		= shutdown_pirq,
 
-	.irq_enable		= enable_pirq,
-	.irq_unmask		= enable_pirq,
-
-	.irq_disable		= disable_pirq,
-	.irq_mask		= disable_pirq,
+	.irq_mask		= disable_dynirq,
+	.irq_unmask		= enable_dynirq,
 
-	.irq_ack		= ack_pirq,
+	.irq_ack		= eoi_pirq,
+	.irq_eoi		= eoi_pirq,
+	.irq_mask_ack		= mask_ack_pirq,
 
 	.irq_set_affinity	= set_affinity_irq,
 

Re: [Xen-devel] [PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[Jan Beulich]

>>> On 29.03.11 at 16:15, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> xen: do not clear and mask evtchns in __xen_evtchn_do_upcall
> 
> Switch virqs and pirqs that don't need EOI (in Xen acktype ==
> ACKTYPE_NONE, that means the machine irq is actually edge)
> to handle_edge_irq.
> 
> Use handle_fasteoi_irq for pirqs that need eoi (they generally
> correspond to level triggered irqs), no risk in loosing interrupts
> because we have to EOI the irq anyway.
> 
> This change has the following benefits:
> 
> - it uses the very same handlers that Linux would use on native for the
> same irqs;

For IO-APIC IRQs this is true, but I don't think it is for MSI.

> - it uses these handlers in the same way Linux would use them: it let
> Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
> the irq;

Same here, plus with this you give the change the appearance as
if Linux now suddenly had control over when the ACK happens,
which I don't think is the case (and would be wrong in the DomU
case at least).

Jan

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 02b5a9c..4f15dde 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -118,6 +118,8 @@ static DEFINE_PER_CPU(unsigned long 
> [NR_EVENT_CHANNELS/BITS_PER_LONG],
>  static struct irq_chip xen_dynamic_chip;
>  static struct irq_chip xen_percpu_chip;
>  static struct irq_chip xen_pirq_chip;
> +static void enable_dynirq(struct irq_data *data);
> +static void disable_dynirq(struct irq_data *data);
>  
>  /* Get info for IRQ */
>  static struct irq_info *info_for_irq(unsigned irq)
> @@ -473,16 +475,6 @@ static void xen_free_irq(unsigned irq)
>  	irq_free_desc(irq);
>  }
>  
> -static void pirq_unmask_notify(int irq)
> -{
> -	struct physdev_eoi eoi = { .irq = pirq_from_irq(irq) };
> -
> -	if (unlikely(pirq_needs_eoi(irq))) {
> -		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
> -		WARN_ON(rc);
> -	}
> -}
> -
>  static void pirq_query_unmask(int irq)
>  {
>  	struct physdev_irq_status_query irq_status;
> @@ -506,6 +498,30 @@ static bool probing_irq(int irq)
>  	return desc && desc->action == NULL;
>  }
>  
> +static void eoi_pirq(struct irq_data *data)
> +{
> +	int evtchn = evtchn_from_irq(data->irq);
> + 	struct irq_info *info = info_for_irq(data->irq);
> + 	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
> +	int rc = 0;
> +
> +	move_native_irq(data->irq);
> +
> +	if (VALID_EVTCHN(evtchn))
> +		clear_evtchn(evtchn);
> +
> +	if (pirq_needs_eoi(data->irq)) {
> +		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
> +		WARN_ON(rc);
> +	}
> +}
> +
> +static void mask_ack_pirq(struct irq_data *data)
> +{
> +	disable_dynirq(data);
> +	eoi_pirq(data);
> +}
> + 
>  static unsigned int __startup_pirq(unsigned int irq)
>  {
>  	struct evtchn_bind_pirq bind_pirq;
> @@ -539,7 +555,7 @@ static unsigned int __startup_pirq(unsigned int irq)
>  
>  out:
>  	unmask_evtchn(evtchn);
> -	pirq_unmask_notify(irq);
> +	eoi_pirq(irq_get_irq_data(irq));
>  
>  	return 0;
>  }
> @@ -572,27 +588,6 @@ static void shutdown_pirq(struct irq_data *data)
>  	info->evtchn = 0;
>  }
>  
> -static void enable_pirq(struct irq_data *data)
> -{
> -	startup_pirq(data);
> -}
> -
> -static void disable_pirq(struct irq_data *data)
> -{
> -}
> -
> -static void ack_pirq(struct irq_data *data)
> -{
> -	int evtchn = evtchn_from_irq(data->irq);
> -
> -	move_native_irq(data->irq);
> -
> -	if (VALID_EVTCHN(evtchn)) {
> -		mask_evtchn(evtchn);
> -		clear_evtchn(evtchn);
> -	}
> -}
> -
>  static int find_irq_by_gsi(unsigned gsi)
>  {
>  	struct irq_info *info;
> @@ -639,9 +634,6 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
>  	if (irq < 0)
>  		goto out;
>  
> -	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> -				      handle_level_irq, name);
> -
>  	irq_op.irq = irq;
>  	irq_op.vector = 0;
>  
> @@ -658,6 +650,14 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
>  	xen_irq_info_pirq_init(irq, 0, pirq, gsi, irq_op.vector,
>  			       shareable ? PIRQ_SHAREABLE : 0);
>  
> +	pirq_query_unmask(irq);
> +	if (pirq_needs_eoi(irq))
> +		set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> +				handle_fasteoi_irq, name);
> +	else
> +		set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> +				handle_edge_irq, name);
> +
>  out:
>  	spin_unlock(&irq_mapping_update_lock);
>  
> @@ -691,7 +691,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct 
> msi_desc *msidesc,
>  		goto out;
>  
>  	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> -				      handle_level_irq, name);
> +				      handle_edge_irq, name);
>  
>  	xen_irq_info_pirq_init(irq, 0, pirq, 0, vector, 0);
>  	ret = irq_set_msi_desc(irq, msidesc);
> @@ -773,7 +773,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
>  			goto out;
>  
>  		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
> -					      handle_fasteoi_irq, "event");
> +					      handle_edge_irq, "event");
>  
>  		xen_irq_info_evtchn_init(irq, evtchn);
>  	}
> @@ -1181,9 +1181,6 @@ static void __xen_evtchn_do_upcall(void)
>  				port = (word_idx * BITS_PER_LONG) + bit_idx;
>  				irq = evtchn_to_irq[port];
>  
> -				mask_evtchn(port);
> -				clear_evtchn(port);
> -
>  				if (irq != -1) {
>  					desc = irq_to_desc(irq);
>  					if (desc)
> @@ -1339,12 +1336,18 @@ static void ack_dynirq(struct irq_data *data)
>  {
>  	int evtchn = evtchn_from_irq(data->irq);
>  
> -	move_masked_irq(data->irq);
> +	move_native_irq(data->irq);
>  
>  	if (VALID_EVTCHN(evtchn))
> -		unmask_evtchn(evtchn);
> +		clear_evtchn(evtchn);
>  }
>  
> +static void mask_ack_dynirq(struct irq_data *data)
> +{
> +	disable_dynirq(data);
> +	ack_dynirq(data);
> +}
> + 
>  static int retrigger_dynirq(struct irq_data *data)
>  {
>  	int evtchn = evtchn_from_irq(data->irq);
> @@ -1533,11 +1536,12 @@ void xen_irq_resume(void)
>  static struct irq_chip xen_dynamic_chip __read_mostly = {
>  	.name			= "xen-dyn",
>  
> -	.irq_disable		= disable_dynirq,
>  	.irq_mask		= disable_dynirq,
>  	.irq_unmask		= enable_dynirq,
>  
> -	.irq_eoi		= ack_dynirq,
> +	.irq_ack		= ack_dynirq,
> +	.irq_mask_ack		= mask_ack_dynirq,
> +
>  	.irq_set_affinity	= set_affinity_irq,
>  	.irq_retrigger		= retrigger_dynirq,
>  };
> @@ -1548,13 +1552,12 @@ static struct irq_chip xen_pirq_chip __read_mostly = 
> {
>  	.irq_startup		= startup_pirq,
>  	.irq_shutdown		= shutdown_pirq,
>  
> -	.irq_enable		= enable_pirq,
> -	.irq_unmask		= enable_pirq,
> -
> -	.irq_disable		= disable_pirq,
> -	.irq_mask		= disable_pirq,
> +	.irq_mask		= disable_dynirq,
> +	.irq_unmask		= enable_dynirq,
>  
> -	.irq_ack		= ack_pirq,
> +	.irq_ack		= eoi_pirq,
> +	.irq_eoi		= eoi_pirq,
> +	.irq_mask_ack		= mask_ack_pirq,
>  
>  	.irq_set_affinity	= set_affinity_irq,
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 

Re: [Xen-devel] [PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[Stefano Stabellini]

On Tue, 29 Mar 2011, Jan Beulich wrote:
> >>> On 29.03.11 at 16:15, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > xen: do not clear and mask evtchns in __xen_evtchn_do_upcall
> > 
> > Switch virqs and pirqs that don't need EOI (in Xen acktype ==
> > ACKTYPE_NONE, that means the machine irq is actually edge)
> > to handle_edge_irq.
> > 
> > Use handle_fasteoi_irq for pirqs that need eoi (they generally
> > correspond to level triggered irqs), no risk in loosing interrupts
> > because we have to EOI the irq anyway.
> > 
> > This change has the following benefits:
> > 
> > - it uses the very same handlers that Linux would use on native for the
> > same irqs;
> 
> For IO-APIC IRQs this is true, but I don't think it is for MSI.

Yes it is:

static int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc,
        int irq)
...
    irq_set_chip_and_handler_name(irq, chip, handle_edge_irq, "edge");


> > - it uses these handlers in the same way Linux would use them: it let
> > Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
> > the irq;
> 
> Same here, plus with this you give the change the appearance as
> if Linux now suddenly had control over when the ACK happens,
> which I don't think is the case (and would be wrong in the DomU
> case at least).

>From the evtchn perspective, the ack is clearing the pending bit in the
shared_info page, so I think we are giving Linux control on when the ack
happen.
>From the pirq perspective, we are giving Linux control on when the eoi
happen.

Re: [PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[Konrad Rzeszutek Wilk]

On Tue, Mar 29, 2011 at 03:15:07PM +0100, Stefano Stabellini wrote:
> xen: do not clear and mask evtchns in __xen_evtchn_do_upcall
> 
> Switch virqs and pirqs that don't need EOI (in Xen acktype ==
   ^^                   ^^
Swap                     irq sub-handlers? 
> ACKTYPE_NONE, that means the machine irq is actually edge)

And where is the ACKTYPE_NONE? Is that a Xen hypervisor thing?
Ah yes. It looks like soo. Perhaps you should rephrase that
and explain what ACKTYPE_NONE is.

I think for these patches the more copious the comments are
the better - in case we want to revisit this in next quarter.
> to handle_edge_irq.
> 
> Use handle_fasteoi_irq for pirqs that need eoi (they generally
> correspond to level triggered irqs), no risk in loosing interrupts
> because we have to EOI the irq anyway.

Can you include an URL to explain what EOI/level/edge interrupts
are? Maybe the correct page in the Intel manual?

I wish there was a table explaining when/or what type of eoi
you need per different type of interrupts. B/c it looks like
you can do EOI for edge-type interrupts (ISA), but not for
MSI/MSI-X ones.
> 
> This change has the following benefits:
> 
> - it uses the very same handlers that Linux would use on native for the
> same irqs;

Can you expand? As in for the GSI? Or for the MSI/MSI-X?

> 
> - it uses these handlers in the same way Linux would use them: it let
> Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
> the irq;
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 02b5a9c..4f15dde 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -118,6 +118,8 @@ static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
>  static struct irq_chip xen_dynamic_chip;
>  static struct irq_chip xen_percpu_chip;
>  static struct irq_chip xen_pirq_chip;
> +static void enable_dynirq(struct irq_data *data);
> +static void disable_dynirq(struct irq_data *data);
>  
>  /* Get info for IRQ */
>  static struct irq_info *info_for_irq(unsigned irq)
> @@ -473,16 +475,6 @@ static void xen_free_irq(unsigned irq)
>  	irq_free_desc(irq);
>  }
>  
> -static void pirq_unmask_notify(int irq)
> -{
> -	struct physdev_eoi eoi = { .irq = pirq_from_irq(irq) };
> -
> -	if (unlikely(pirq_needs_eoi(irq))) {
> -		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
> -		WARN_ON(rc);
> -	}
> -}
> -
>  static void pirq_query_unmask(int irq)
>  {
>  	struct physdev_irq_status_query irq_status;
> @@ -506,6 +498,30 @@ static bool probing_irq(int irq)
>  	return desc && desc->action == NULL;
>  }
>  
> +static void eoi_pirq(struct irq_data *data)
> +{
> +	int evtchn = evtchn_from_irq(data->irq);
> + 	struct irq_info *info = info_for_irq(data->irq);
> + 	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };

No pirq?
> +	int rc = 0;
> +
> +	move_native_irq(data->irq);
> +
> +	if (VALID_EVTCHN(evtchn))
> +		clear_evtchn(evtchn);
> +
> +	if (pirq_needs_eoi(data->irq)) {

Hm, you are using the Linux IRQ here, not the PIRQ value.

> +		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
> +		WARN_ON(rc);
> +	}
> +}
> +
> +static void mask_ack_pirq(struct irq_data *data)
> +{
> +	disable_dynirq(data);
> +	eoi_pirq(data);
> +}
> + 
>  static unsigned int __startup_pirq(unsigned int irq)
>  {
>  	struct evtchn_bind_pirq bind_pirq;
> @@ -539,7 +555,7 @@ static unsigned int __startup_pirq(unsigned int irq)
>  
>  out:
>  	unmask_evtchn(evtchn);
> -	pirq_unmask_notify(irq);
> +	eoi_pirq(irq_get_irq_data(irq));
>  
>  	return 0;
>  }
> @@ -572,27 +588,6 @@ static void shutdown_pirq(struct irq_data *data)
>  	info->evtchn = 0;
>  }
>  
> -static void enable_pirq(struct irq_data *data)
> -{
> -	startup_pirq(data);
> -}
> -
> -static void disable_pirq(struct irq_data *data)
> -{
> -}
> -
> -static void ack_pirq(struct irq_data *data)
> -{
> -	int evtchn = evtchn_from_irq(data->irq);
> -
> -	move_native_irq(data->irq);
> -
> -	if (VALID_EVTCHN(evtchn)) {
> -		mask_evtchn(evtchn);
> -		clear_evtchn(evtchn);
> -	}
> -}
> -
>  static int find_irq_by_gsi(unsigned gsi)
>  {
>  	struct irq_info *info;
> @@ -639,9 +634,6 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
>  	if (irq < 0)
>  		goto out;
>  
> -	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> -				      handle_level_irq, name);
> -
>  	irq_op.irq = irq;
>  	irq_op.vector = 0;
>  
> @@ -658,6 +650,14 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
>  	xen_irq_info_pirq_init(irq, 0, pirq, gsi, irq_op.vector,
>  			       shareable ? PIRQ_SHAREABLE : 0);
>  
> +	pirq_query_unmask(irq);
> +	if (pirq_needs_eoi(irq))
> +		set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> +				handle_fasteoi_irq, name);
> +	else
> +		set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> +				handle_edge_irq, name);

OK. You need a big comment about this in the code. Explain
why this is happening. B/c if you look at this from code
it seems like wrong thing to do for gsi's (as in you would
think handle_level_irq would the right choice).

> +
>  out:
>  	spin_unlock(&irq_mapping_update_lock);
>  
> @@ -691,7 +691,7 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
>  		goto out;
>  
>  	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> -				      handle_level_irq, name);
> +				      handle_edge_irq, name);
>  
>  	xen_irq_info_pirq_init(irq, 0, pirq, 0, vector, 0);
>  	ret = irq_set_msi_desc(irq, msidesc);
> @@ -773,7 +773,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
>  			goto out;
>  
>  		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
> -					      handle_fasteoi_irq, "event");
> +					      handle_edge_irq, "event");
>  
>  		xen_irq_info_evtchn_init(irq, evtchn);
>  	}
> @@ -1181,9 +1181,6 @@ static void __xen_evtchn_do_upcall(void)
>  				port = (word_idx * BITS_PER_LONG) + bit_idx;
>  				irq = evtchn_to_irq[port];
>  
> -				mask_evtchn(port);
> -				clear_evtchn(port);
> -
>  				if (irq != -1) {
>  					desc = irq_to_desc(irq);
>  					if (desc)
> @@ -1339,12 +1336,18 @@ static void ack_dynirq(struct irq_data *data)
>  {
>  	int evtchn = evtchn_from_irq(data->irq);
>  
> -	move_masked_irq(data->irq);
> +	move_native_irq(data->irq);
>  
>  	if (VALID_EVTCHN(evtchn))
> -		unmask_evtchn(evtchn);
> +		clear_evtchn(evtchn);
>  }
>  
> +static void mask_ack_dynirq(struct irq_data *data)
> +{
> +	disable_dynirq(data);
> +	ack_dynirq(data);
> +}
> + 
>  static int retrigger_dynirq(struct irq_data *data)
>  {
>  	int evtchn = evtchn_from_irq(data->irq);
> @@ -1533,11 +1536,12 @@ void xen_irq_resume(void)
>  static struct irq_chip xen_dynamic_chip __read_mostly = {
>  	.name			= "xen-dyn",
>  
> -	.irq_disable		= disable_dynirq,
>  	.irq_mask		= disable_dynirq,
>  	.irq_unmask		= enable_dynirq,
>  
> -	.irq_eoi		= ack_dynirq,
> +	.irq_ack		= ack_dynirq,
> +	.irq_mask_ack		= mask_ack_dynirq,
> +

Do we need to this for percpu handler? 
>  	.irq_set_affinity	= set_affinity_irq,
>  	.irq_retrigger		= retrigger_dynirq,
>  };
> @@ -1548,13 +1552,12 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
>  	.irq_startup		= startup_pirq,
>  	.irq_shutdown		= shutdown_pirq,
>  
> -	.irq_enable		= enable_pirq,
> -	.irq_unmask		= enable_pirq,
> -
> -	.irq_disable		= disable_pirq,
> -	.irq_mask		= disable_pirq,
> +	.irq_mask		= disable_dynirq,
> +	.irq_unmask		= enable_dynirq,
>  
> -	.irq_ack		= ack_pirq,
> +	.irq_ack		= eoi_pirq,
> +	.irq_eoi		= eoi_pirq,
> +	.irq_mask_ack		= mask_ack_pirq,
>  
>  	.irq_set_affinity	= set_affinity_irq,
>  

Re: [PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[Stefano Stabellini]

On Wed, 30 Mar 2011, Konrad Rzeszutek Wilk wrote:
> On Tue, Mar 29, 2011 at 03:15:07PM +0100, Stefano Stabellini wrote:
> > xen: do not clear and mask evtchns in __xen_evtchn_do_upcall
> > 
> > Switch virqs and pirqs that don't need EOI (in Xen acktype ==
>    ^^                   ^^
> Swap                     irq sub-handlers? 
> > ACKTYPE_NONE, that means the machine irq is actually edge)
> 
> And where is the ACKTYPE_NONE? Is that a Xen hypervisor thing?
> Ah yes. It looks like soo. Perhaps you should rephrase that
> and explain what ACKTYPE_NONE is.
> 
> I think for these patches the more copious the comments are
> the better - in case we want to revisit this in next quarter.

I reworded this part of the commit message, see below updated patch.


> > to handle_edge_irq.
> > 
> > Use handle_fasteoi_irq for pirqs that need eoi (they generally
> > correspond to level triggered irqs), no risk in loosing interrupts
> > because we have to EOI the irq anyway.
> 
> Can you include an URL to explain what EOI/level/edge interrupts
> are? Maybe the correct page in the Intel manual?

I think the best docs we have are the genericirq docbook docs by tglx.


> I wish there was a table explaining when/or what type of eoi
> you need per different type of interrupts. B/c it looks like
> you can do EOI for edge-type interrupts (ISA), but not for
> MSI/MSI-X ones.

Indeed.


> > 
> > This change has the following benefits:
> > 
> > - it uses the very same handlers that Linux would use on native for the
> > same irqs;
> 
> Can you expand? As in for the GSI? Or for the MSI/MSI-X?

Linux on native would use handle_edge_irq for edge irqs and msis, and
handle_fasteoi_irq for everything else.
With this patch we are doing the same thing.


> > - it uses these handlers in the same way Linux would use them: it let
> > Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
> > the irq;
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> > index 02b5a9c..4f15dde 100644
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -118,6 +118,8 @@ static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
> >  static struct irq_chip xen_dynamic_chip;
> >  static struct irq_chip xen_percpu_chip;
> >  static struct irq_chip xen_pirq_chip;
> > +static void enable_dynirq(struct irq_data *data);
> > +static void disable_dynirq(struct irq_data *data);
> >  
> >  /* Get info for IRQ */
> >  static struct irq_info *info_for_irq(unsigned irq)
> > @@ -473,16 +475,6 @@ static void xen_free_irq(unsigned irq)
> >  	irq_free_desc(irq);
> >  }
> >  
> > -static void pirq_unmask_notify(int irq)
> > -{
> > -	struct physdev_eoi eoi = { .irq = pirq_from_irq(irq) };
> > -
> > -	if (unlikely(pirq_needs_eoi(irq))) {
> > -		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
> > -		WARN_ON(rc);
> > -	}
> > -}
> > -
> >  static void pirq_query_unmask(int irq)
> >  {
> >  	struct physdev_irq_status_query irq_status;
> > @@ -506,6 +498,30 @@ static bool probing_irq(int irq)
> >  	return desc && desc->action == NULL;
> >  }
> >  
> > +static void eoi_pirq(struct irq_data *data)
> > +{
> > +	int evtchn = evtchn_from_irq(data->irq);
> > + 	struct irq_info *info = info_for_irq(data->irq);
> > + 	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
> 
> No pirq?

great catch, thanks!


> > +	int rc = 0;
> > +
> > +	move_native_irq(data->irq);
> > +
> > +	if (VALID_EVTCHN(evtchn))
> > +		clear_evtchn(evtchn);
> > +
> > +	if (pirq_needs_eoi(data->irq)) {
> 
> Hm, you are using the Linux IRQ here, not the PIRQ value.

this one is correct, I believe


> > +		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
> > +		WARN_ON(rc);
> > +	}
> > +}
> > +
> > +static void mask_ack_pirq(struct irq_data *data)
> > +{
> > +	disable_dynirq(data);
> > +	eoi_pirq(data);
> > +}
> > + 
> >  static unsigned int __startup_pirq(unsigned int irq)
> >  {
> >  	struct evtchn_bind_pirq bind_pirq;
> > @@ -539,7 +555,7 @@ static unsigned int __startup_pirq(unsigned int irq)
> >  
> >  out:
> >  	unmask_evtchn(evtchn);
> > -	pirq_unmask_notify(irq);
> > +	eoi_pirq(irq_get_irq_data(irq));
> >  
> >  	return 0;
> >  }
> > @@ -572,27 +588,6 @@ static void shutdown_pirq(struct irq_data *data)
> >  	info->evtchn = 0;
> >  }
> >  
> > -static void enable_pirq(struct irq_data *data)
> > -{
> > -	startup_pirq(data);
> > -}
> > -
> > -static void disable_pirq(struct irq_data *data)
> > -{
> > -}
> > -
> > -static void ack_pirq(struct irq_data *data)
> > -{
> > -	int evtchn = evtchn_from_irq(data->irq);
> > -
> > -	move_native_irq(data->irq);
> > -
> > -	if (VALID_EVTCHN(evtchn)) {
> > -		mask_evtchn(evtchn);
> > -		clear_evtchn(evtchn);
> > -	}
> > -}
> > -
> >  static int find_irq_by_gsi(unsigned gsi)
> >  {
> >  	struct irq_info *info;
> > @@ -639,9 +634,6 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
> >  	if (irq < 0)
> >  		goto out;
> >  
> > -	set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> > -				      handle_level_irq, name);
> > -
> >  	irq_op.irq = irq;
> >  	irq_op.vector = 0;
> >  
> > @@ -658,6 +650,14 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
> >  	xen_irq_info_pirq_init(irq, 0, pirq, gsi, irq_op.vector,
> >  			       shareable ? PIRQ_SHAREABLE : 0);
> >  
> > +	pirq_query_unmask(irq);
> > +	if (pirq_needs_eoi(irq))
> > +		set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> > +				handle_fasteoi_irq, name);
> > +	else
> > +		set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
> > +				handle_edge_irq, name);
> 
> OK. You need a big comment about this in the code. Explain
> why this is happening. B/c if you look at this from code
> it seems like wrong thing to do for gsi's (as in you would
> think handle_level_irq would the right choice).

Except handle_level_irq is not used anymore anywhere in the kernel, give
a look at arch/x86/kernel/apic/io_apic.c:ioapic_register_intr.

Updated patch, rebased on 2.6.39-rc1 follows:

---


commit 6978531913b45abf3aff048475a2174a2cdaf288
Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Date:   Tue Mar 29 14:15:06 2011 +0000

    xen: do not clear and mask evtchns in __xen_evtchn_do_upcall
    
    Change the irq handler of virqs and pirqs that don't need EOI (pirqs
    that correspond to physical edge interrupts) to handle_edge_irq.
    
    Use handle_fasteoi_irq for pirqs that need eoi (they generally
    correspond to level triggered irqs), no risk in loosing interrupts
    because we have to EOI the irq anyway.
    
    This change has the following benefits:
    
    - it uses the very same handlers that Linux would use on native for the
    same irqs (handle_edge_irq for edge irqs and msis, and
    handle_fasteoi_irq for everything else);
    
    - it uses these handlers in the same way Linux would use them: it let
    Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
    the irq;
    
    See genericirq in the kernel docbook docs for more informations.
    
    Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 036343b..186eb1e 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -118,6 +118,8 @@ static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
 static struct irq_chip xen_dynamic_chip;
 static struct irq_chip xen_percpu_chip;
 static struct irq_chip xen_pirq_chip;
+static void enable_dynirq(struct irq_data *data);
+static void disable_dynirq(struct irq_data *data);
 
 /* Get info for IRQ */
 static struct irq_info *info_for_irq(unsigned irq)
@@ -473,16 +475,6 @@ static void xen_free_irq(unsigned irq)
 	irq_free_desc(irq);
 }
 
-static void pirq_unmask_notify(int irq)
-{
-	struct physdev_eoi eoi = { .irq = pirq_from_irq(irq) };
-
-	if (unlikely(pirq_needs_eoi(irq))) {
-		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
-		WARN_ON(rc);
-	}
-}
-
 static void pirq_query_unmask(int irq)
 {
 	struct physdev_irq_status_query irq_status;
@@ -506,6 +498,29 @@ static bool probing_irq(int irq)
 	return desc && desc->action == NULL;
 }
 
+static void eoi_pirq(struct irq_data *data)
+{
+	int evtchn = evtchn_from_irq(data->irq);
+ 	struct physdev_eoi eoi = { .irq = pirq_from_irq(data->irq) };
+	int rc = 0;
+
+	irq_move_irq(data);
+
+	if (VALID_EVTCHN(evtchn))
+		clear_evtchn(evtchn);
+
+	if (pirq_needs_eoi(data->irq)) {
+		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
+		WARN_ON(rc);
+	}
+}
+
+static void mask_ack_pirq(struct irq_data *data)
+{
+	disable_dynirq(data);
+	eoi_pirq(data);
+}
+ 
 static unsigned int __startup_pirq(unsigned int irq)
 {
 	struct evtchn_bind_pirq bind_pirq;
@@ -539,7 +554,7 @@ static unsigned int __startup_pirq(unsigned int irq)
 
 out:
 	unmask_evtchn(evtchn);
-	pirq_unmask_notify(irq);
+	eoi_pirq(irq_get_irq_data(irq));
 
 	return 0;
 }
@@ -572,27 +587,6 @@ static void shutdown_pirq(struct irq_data *data)
 	info->evtchn = 0;
 }
 
-static void enable_pirq(struct irq_data *data)
-{
-	startup_pirq(data);
-}
-
-static void disable_pirq(struct irq_data *data)
-{
-}
-
-static void ack_pirq(struct irq_data *data)
-{
-	int evtchn = evtchn_from_irq(data->irq);
-
-	irq_move_irq(data);
-
-	if (VALID_EVTCHN(evtchn)) {
-		mask_evtchn(evtchn);
-		clear_evtchn(evtchn);
-	}
-}
-
 static int find_irq_by_gsi(unsigned gsi)
 {
 	struct irq_info *info;
@@ -639,9 +633,6 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	if (irq < 0)
 		goto out;
 
-	irq_set_chip_and_handler_name(irq, &xen_pirq_chip, handle_level_irq,
-				      name);
-
 	irq_op.irq = irq;
 	irq_op.vector = 0;
 
@@ -658,6 +649,19 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
 	xen_irq_info_pirq_init(irq, 0, pirq, gsi, irq_op.vector,
 			       shareable ? PIRQ_SHAREABLE : 0);
 
+	pirq_query_unmask(irq);
+	/* we try to follow the same convention as Linux on native:
+	 * handle_edge_irq for edge irqs and handle_fasteoi_irq for level
+	 * irqs, see ioapic_register_intr (handle_level_irq is not used
+	 * anymore).
+	 */
+	if (pirq_needs_eoi(irq))
+		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
+				handle_fasteoi_irq, name);
+	else
+		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
+				handle_edge_irq, name);
+
 out:
 	spin_unlock(&irq_mapping_update_lock);
 
@@ -690,8 +694,8 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
 	if (irq == -1)
 		goto out;
 
-	irq_set_chip_and_handler_name(irq, &xen_pirq_chip, handle_level_irq,
-				      name);
+	irq_set_chip_and_handler_name(irq, &xen_pirq_chip, handle_edge_irq,
+			name);
 
 	xen_irq_info_pirq_init(irq, 0, pirq, 0, vector, 0);
 	ret = irq_set_msi_desc(irq, msidesc);
@@ -773,7 +777,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 			goto out;
 
 		irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_fasteoi_irq, "event");
+					      handle_edge_irq, "event");
 
 		xen_irq_info_evtchn_init(irq, evtchn);
 	}
@@ -1181,9 +1185,6 @@ static void __xen_evtchn_do_upcall(void)
 				port = (word_idx * BITS_PER_LONG) + bit_idx;
 				irq = evtchn_to_irq[port];
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1339,12 +1340,18 @@ static void ack_dynirq(struct irq_data *data)
 {
 	int evtchn = evtchn_from_irq(data->irq);
 
-	irq_move_masked_irq(data);
+	irq_move_irq(data);
 
 	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+		clear_evtchn(evtchn);
 }
 
+static void mask_ack_dynirq(struct irq_data *data)
+{
+	disable_dynirq(data);
+	ack_dynirq(data);
+}
+ 
 static int retrigger_dynirq(struct irq_data *data)
 {
 	int evtchn = evtchn_from_irq(data->irq);
@@ -1533,11 +1540,12 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name			= "xen-dyn",
 
-	.irq_disable		= disable_dynirq,
 	.irq_mask		= disable_dynirq,
 	.irq_unmask		= enable_dynirq,
 
-	.irq_eoi		= ack_dynirq,
+	.irq_ack		= ack_dynirq,
+	.irq_mask_ack		= mask_ack_dynirq,
+
 	.irq_set_affinity	= set_affinity_irq,
 	.irq_retrigger		= retrigger_dynirq,
 };
@@ -1548,13 +1556,12 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.irq_startup		= startup_pirq,
 	.irq_shutdown		= shutdown_pirq,
 
-	.irq_enable		= enable_pirq,
-	.irq_unmask		= enable_pirq,
-
-	.irq_disable		= disable_pirq,
-	.irq_mask		= disable_pirq,
+	.irq_mask		= disable_dynirq,
+	.irq_unmask		= enable_dynirq,
 
-	.irq_ack		= ack_pirq,
+	.irq_ack		= eoi_pirq,
+	.irq_eoi		= eoi_pirq,
+	.irq_mask_ack		= mask_ack_pirq,
 
 	.irq_set_affinity	= set_affinity_irq,
 

Re: [PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[Konrad Rzeszutek Wilk]

> I reworded this part of the commit message, see below updated patch.
> 

[looking]
.. snip..
> > Can you expand? As in for the GSI? Or for the MSI/MSI-X?
> 
> Linux on native would use handle_edge_irq for edge irqs and msis, and
> handle_fasteoi_irq for everything else.

What about per_cpu one?

.. snip ..

> > 
> > OK. You need a big comment about this in the code. Explain
> > why this is happening. B/c if you look at this from code
> > it seems like wrong thing to do for gsi's (as in you would
> > think handle_level_irq would the right choice).
> 
> Except handle_level_irq is not used anymore anywhere in the kernel, give
> a look at arch/x86/kernel/apic/io_apic.c:ioapic_register_intr.

'make_8259A_irq' ? But yeah, I don't think we will hit machines with
that architecture anymore.

> 
> Updated patch, rebased on 2.6.39-rc1 follows:
> 
> ---
> 
> 
> commit 6978531913b45abf3aff048475a2174a2cdaf288
> Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Date:   Tue Mar 29 14:15:06 2011 +0000
> 
>     xen: do not clear and mask evtchns in __xen_evtchn_do_upcall
>     
>     Change the irq handler of virqs and pirqs that don't need EOI (pirqs
>     that correspond to physical edge interrupts) to handle_edge_irq.
>     
>     Use handle_fasteoi_irq for pirqs that need eoi (they generally
>     correspond to level triggered irqs), no risk in loosing interrupts
>     because we have to EOI the irq anyway.
>     
>     This change has the following benefits:
>     
>     - it uses the very same handlers that Linux would use on native for the
>     same irqs (handle_edge_irq for edge irqs and msis, and
                                                   ^^^-'MSIs/MSI-Xs'
>     handle_fasteoi_irq for everything else);
>     
>     - it uses these handlers in the same way Linux would use them: it let
                                                                           ^- 's'

>     Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
                                                       ^- 's'
>     the irq;
>     
>     See genericirq in the kernel docbook docs for more informations.

Say 'Documentation/DocBook/genericirq.tmpl'

[edit: The patch looks OK to me, but let me think about this some more over this
week and sketch out the flow].

>     
>     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 036343b..186eb1e 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -118,6 +118,8 @@ static DEFINE_PER_CPU(unsigned long [NR_EVENT_CHANNELS/BITS_PER_LONG],
>  static struct irq_chip xen_dynamic_chip;
>  static struct irq_chip xen_percpu_chip;
>  static struct irq_chip xen_pirq_chip;
> +static void enable_dynirq(struct irq_data *data);
> +static void disable_dynirq(struct irq_data *data);
>  
>  /* Get info for IRQ */
>  static struct irq_info *info_for_irq(unsigned irq)
> @@ -473,16 +475,6 @@ static void xen_free_irq(unsigned irq)
>  	irq_free_desc(irq);
>  }
>  
> -static void pirq_unmask_notify(int irq)
> -{
> -	struct physdev_eoi eoi = { .irq = pirq_from_irq(irq) };
> -
> -	if (unlikely(pirq_needs_eoi(irq))) {
> -		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
> -		WARN_ON(rc);
> -	}
> -}
> -
>  static void pirq_query_unmask(int irq)
>  {
>  	struct physdev_irq_status_query irq_status;
> @@ -506,6 +498,29 @@ static bool probing_irq(int irq)
>  	return desc && desc->action == NULL;
>  }
>  
> +static void eoi_pirq(struct irq_data *data)
> +{
> +	int evtchn = evtchn_from_irq(data->irq);
> + 	struct physdev_eoi eoi = { .irq = pirq_from_irq(data->irq) };
> +	int rc = 0;
> +
> +	irq_move_irq(data);
> +
> +	if (VALID_EVTCHN(evtchn))
> +		clear_evtchn(evtchn);
> +
> +	if (pirq_needs_eoi(data->irq)) {
> +		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
> +		WARN_ON(rc);
> +	}
> +}
> +
> +static void mask_ack_pirq(struct irq_data *data)
> +{
> +	disable_dynirq(data);
> +	eoi_pirq(data);
> +}
> + 
>  static unsigned int __startup_pirq(unsigned int irq)
>  {
>  	struct evtchn_bind_pirq bind_pirq;
> @@ -539,7 +554,7 @@ static unsigned int __startup_pirq(unsigned int irq)
>  
>  out:
>  	unmask_evtchn(evtchn);
> -	pirq_unmask_notify(irq);
> +	eoi_pirq(irq_get_irq_data(irq));
>  
>  	return 0;
>  }
> @@ -572,27 +587,6 @@ static void shutdown_pirq(struct irq_data *data)
>  	info->evtchn = 0;
>  }
>  
> -static void enable_pirq(struct irq_data *data)
> -{
> -	startup_pirq(data);
> -}
> -
> -static void disable_pirq(struct irq_data *data)
> -{
> -}
> -
> -static void ack_pirq(struct irq_data *data)
> -{
> -	int evtchn = evtchn_from_irq(data->irq);
> -
> -	irq_move_irq(data);
> -
> -	if (VALID_EVTCHN(evtchn)) {
> -		mask_evtchn(evtchn);
> -		clear_evtchn(evtchn);
> -	}
> -}
> -
>  static int find_irq_by_gsi(unsigned gsi)
>  {
>  	struct irq_info *info;
> @@ -639,9 +633,6 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
>  	if (irq < 0)
>  		goto out;
>  
> -	irq_set_chip_and_handler_name(irq, &xen_pirq_chip, handle_level_irq,
> -				      name);
> -
>  	irq_op.irq = irq;
>  	irq_op.vector = 0;
>  
> @@ -658,6 +649,19 @@ int xen_bind_pirq_gsi_to_irq(unsigned gsi,
>  	xen_irq_info_pirq_init(irq, 0, pirq, gsi, irq_op.vector,
>  			       shareable ? PIRQ_SHAREABLE : 0);
>  
> +	pirq_query_unmask(irq);
> +	/* we try to follow the same convention as Linux on native:
> +	 * handle_edge_irq for edge irqs and handle_fasteoi_irq for level
> +	 * irqs, see ioapic_register_intr (handle_level_irq is not used
> +	 * anymore).
> +	 */
> +	if (pirq_needs_eoi(irq))
> +		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
> +				handle_fasteoi_irq, name);
> +	else
> +		irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
> +				handle_edge_irq, name);
> +
>  out:
>  	spin_unlock(&irq_mapping_update_lock);
>  
> @@ -690,8 +694,8 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
>  	if (irq == -1)
>  		goto out;
>  
> -	irq_set_chip_and_handler_name(irq, &xen_pirq_chip, handle_level_irq,
> -				      name);
> +	irq_set_chip_and_handler_name(irq, &xen_pirq_chip, handle_edge_irq,
> +			name);
>  
>  	xen_irq_info_pirq_init(irq, 0, pirq, 0, vector, 0);
>  	ret = irq_set_msi_desc(irq, msidesc);
> @@ -773,7 +777,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
>  			goto out;
>  
>  		irq_set_chip_and_handler_name(irq, &xen_dynamic_chip,
> -					      handle_fasteoi_irq, "event");
> +					      handle_edge_irq, "event");
>  
>  		xen_irq_info_evtchn_init(irq, evtchn);
>  	}
> @@ -1181,9 +1185,6 @@ static void __xen_evtchn_do_upcall(void)
>  				port = (word_idx * BITS_PER_LONG) + bit_idx;
>  				irq = evtchn_to_irq[port];
>  
> -				mask_evtchn(port);
> -				clear_evtchn(port);
> -
>  				if (irq != -1) {
>  					desc = irq_to_desc(irq);
>  					if (desc)
> @@ -1339,12 +1340,18 @@ static void ack_dynirq(struct irq_data *data)
>  {
>  	int evtchn = evtchn_from_irq(data->irq);
>  
> -	irq_move_masked_irq(data);
> +	irq_move_irq(data);
>  
>  	if (VALID_EVTCHN(evtchn))
> -		unmask_evtchn(evtchn);
> +		clear_evtchn(evtchn);
>  }
>  
> +static void mask_ack_dynirq(struct irq_data *data)
> +{
> +	disable_dynirq(data);
> +	ack_dynirq(data);
> +}
> + 
>  static int retrigger_dynirq(struct irq_data *data)
>  {
>  	int evtchn = evtchn_from_irq(data->irq);
> @@ -1533,11 +1540,12 @@ void xen_irq_resume(void)
>  static struct irq_chip xen_dynamic_chip __read_mostly = {
>  	.name			= "xen-dyn",
>  
> -	.irq_disable		= disable_dynirq,
>  	.irq_mask		= disable_dynirq,
>  	.irq_unmask		= enable_dynirq,
>  
> -	.irq_eoi		= ack_dynirq,
> +	.irq_ack		= ack_dynirq,
> +	.irq_mask_ack		= mask_ack_dynirq,
> +
>  	.irq_set_affinity	= set_affinity_irq,
>  	.irq_retrigger		= retrigger_dynirq,
>  };
> @@ -1548,13 +1556,12 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
>  	.irq_startup		= startup_pirq,
>  	.irq_shutdown		= shutdown_pirq,
>  
> -	.irq_enable		= enable_pirq,
> -	.irq_unmask		= enable_pirq,
> -
> -	.irq_disable		= disable_pirq,
> -	.irq_mask		= disable_pirq,
> +	.irq_mask		= disable_dynirq,
> +	.irq_unmask		= enable_dynirq,
>  
> -	.irq_ack		= ack_pirq,
> +	.irq_ack		= eoi_pirq,
> +	.irq_eoi		= eoi_pirq,
> +	.irq_mask_ack		= mask_ack_pirq,
>  
>  	.irq_set_affinity	= set_affinity_irq,
>  

Re: [Xen-devel] Re: [PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[Ian Campbell]

Since this patch revolves around the interaction with the irq core I
think it would be worth running it by tglx as well as xen-devel for the
next iteration.

Does something about the switch of handler type fix the issue with
drivers which call disable_irq() in their interrupt handler and leave it
disabled until a later time? (e.g. the userspace evtchn driver). I think
I know why this is the case but it would be useful to mention in the
commit message.

On Mon, 2011-04-04 at 17:46 +0100, Stefano Stabellini wrote:
> 
>     - it uses these handlers in the same way Linux would use them: it let
>     Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
>     the irq;

This code is in Linux, perhaps you mean "the same way native code would
use them" or "with the semantics which the generic code defines" or
something along those lines.

> +       pirq_query_unmask(irq);
> +       /* we try to follow the same convention as Linux on native:

That's basically a coincidence, isn't it?

What we are actually trying to do is use the handler with the
appropriate semantics for the type of interrupt, e.g. event channels are
naturally edge triggered but some event channels can be bound to pirqs
with configurations that require an EOI and therefore have a level of
levelness about them.

> +        * handle_edge_irq for edge irqs and handle_fasteoi_irq for level
> +        * irqs

should probably mention the relationship between pirq_needs_eoi=>level
triggering.

Is there any relationship between pirq_needs_eoi and the trigger value
passed to PHYSDEVOP_setup_gsi at setup time?

> , see ioapic_register_intr (handle_level_irq is not used
> +        * anymore).
> +        */
> +       if (pirq_needs_eoi(irq))
> +               irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
> +                               handle_fasteoi_irq, name);
> +       else
> +               irq_set_chip_and_handler_name(irq, &xen_pirq_chip,
> +                               handle_edge_irq, name);
> +
>  out:
>         spin_unlock(&irq_mapping_update_lock);

Ian.

Re: [PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[Stefano Stabellini]

On Mon, 4 Apr 2011, Konrad Rzeszutek Wilk wrote:
> > I reworded this part of the commit message, see below updated patch.
> > 
> [looking]
> .. snip..
> > > Can you expand? As in for the GSI? Or for the MSI/MSI-X?
> > 
> > Linux on native would use handle_edge_irq for edge irqs and msis, and
> > handle_fasteoi_irq for everything else.
> 
> What about per_cpu one?
> 

That is the simplest case, we are already using the same semantic as in
the native code.


> > > OK. You need a big comment about this in the code. Explain
> > > why this is happening. B/c if you look at this from code
> > > it seems like wrong thing to do for gsi's (as in you would
> > > think handle_level_irq would the right choice).
> > 
> > Except handle_level_irq is not used anymore anywhere in the kernel, give
> > a look at arch/x86/kernel/apic/io_apic.c:ioapic_register_intr.
> 
> 'make_8259A_irq' ? But yeah, I don't think we will hit machines with
> that architecture anymore.

Actually I was looking at:

setup_IO_APIC_irqs -> __io_apic_setup_irqs -> io_apic_setup_irq_pin



> > Updated patch, rebased on 2.6.39-rc1 follows:
> > 
> > ---
> > 
> > 
> > commit 6978531913b45abf3aff048475a2174a2cdaf288
> > Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Date:   Tue Mar 29 14:15:06 2011 +0000
> > 
> >     xen: do not clear and mask evtchns in __xen_evtchn_do_upcall
> >     
> >     Change the irq handler of virqs and pirqs that don't need EOI (pirqs
> >     that correspond to physical edge interrupts) to handle_edge_irq.
> >     
> >     Use handle_fasteoi_irq for pirqs that need eoi (they generally
> >     correspond to level triggered irqs), no risk in loosing interrupts
> >     because we have to EOI the irq anyway.
> >     
> >     This change has the following benefits:
> >     
> >     - it uses the very same handlers that Linux would use on native for the
> >     same irqs (handle_edge_irq for edge irqs and msis, and
>                                                    ^^^-'MSIs/MSI-Xs'
> >     handle_fasteoi_irq for everything else);
> >     
> >     - it uses these handlers in the same way Linux would use them: it let
>                                                                            ^- 's'
> 
> >     Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
>                                                        ^- 's'
> >     the irq;
> >     
> >     See genericirq in the kernel docbook docs for more informations.
> 
> Say 'Documentation/DocBook/genericirq.tmpl'
> 
> [edit: The patch looks OK to me, but let me think about this some more over this
> week and sketch out the flow].

done

Re: [Xen-devel] Re: [PATCH] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

[Stefano Stabellini]

On Tue, 5 Apr 2011, Ian Campbell wrote:
> Since this patch revolves around the interaction with the irq core I
> think it would be worth running it by tglx as well as xen-devel for the
> next iteration.
> 

Yes, I'll do that.


> Does something about the switch of handler type fix the issue with
> drivers which call disable_irq() in their interrupt handler and leave it
> disabled until a later time? (e.g. the userspace evtchn driver). I think
> I know why this is the case but it would be useful to mention in the
> commit message.

Yep, I added a note in the commit message:

"it fixes a problem occurring when a driver calls disable_irq() in its
handler: the old code was unconditionally unmasking the evtchn even if
the irq is disabled when irq_eoi was called"


> On Mon, 2011-04-04 at 17:46 +0100, Stefano Stabellini wrote:
> > 
> >     - it uses these handlers in the same way Linux would use them: it let
> >     Linux mask\unmask and ack the irq when Linux want to mask\unmask and ack
> >     the irq;
> 
> This code is in Linux, perhaps you mean "the same way native code would
> use them" or "with the semantics which the generic code defines" or
> something along those lines.

good point


> > +       pirq_query_unmask(irq);
> > +       /* we try to follow the same convention as Linux on native:
> 
> That's basically a coincidence, isn't it?
> 
> What we are actually trying to do is use the handler with the
> appropriate semantics for the type of interrupt, e.g. event channels are
> naturally edge triggered but some event channels can be bound to pirqs
> with configurations that require an EOI and therefore have a level of
> levelness about them.

I completely rewrote the paragraph, adding many more details, including
the Xen side of the interface.

> 
> > +        * handle_edge_irq for edge irqs and handle_fasteoi_irq for level
> > +        * irqs
> 
> should probably mention the relationship between pirq_needs_eoi=>level
> triggering.

done.


> Is there any relationship between pirq_needs_eoi and the trigger value
> passed to PHYSDEVOP_setup_gsi at setup time?

PHYSDEVOP_setup_gsi tells Xen the trigger value of the physical
interrupt, depending on this value Xen will handle the interrupt
differently. Besides Xen used to set the need_eoi flag of the
corresponding irq depending on the real trigger value of the underlying
physical interrupt, but it doesn't anymore.