[PATCH] xen: do not unmask disabled IRQ on eoi.

[PATCH] xen: do not unmask disabled IRQ on eoi.

[Ian Campbell]

This prevents a guest from being able to trigger an interrupt storm in
dom0 rendering it inoperable. In particular this effects event
channels delivered to userspace by the /dev/xen/evtchn driver which
uses disable_irq in its interupt handler to prevent further interrupts
firing until the interrupt has been handled by the userspace
application and it has requested an unmask. Without this patch the
event channel is re-enabled immediately after the interrupt handler
completes.

The issue was introduced by 0672fb44a111 "xen/events: change to using
fasteoi".

In the specific instance I saw a domU would spin sending console event
channel notifications to dom0 because its console ring was full (this
behaviour has since been tempered by f3483182666a "xen/hvc: only
notify if we actually sent something") but xenconsoled would be unable
to run due to the storm and hence the ring would never be drained.

I'm not convinced this is the right way to go about this -- there does
not seem to be much precedent and I would have expected some sort of
generic handling but I cannot see any.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/events.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 1496ba5..a9b0637 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -1215,6 +1215,17 @@ int resend_irq_on_evtchn(unsigned int irq)
 	return 1;
 }
 
+static void eoi_dynirq(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+	int evtchn = evtchn_from_irq(irq);
+
+	move_masked_irq(irq);
+
+	if (VALID_EVTCHN(evtchn) & !(desc->status & IRQ_DISABLED))
+		unmask_evtchn(evtchn);
+}
+
 static void ack_dynirq(unsigned int irq)
 {
 	int evtchn = evtchn_from_irq(irq);
@@ -1408,7 +1419,7 @@ static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
 
-	.eoi		= ack_dynirq,
+	.eoi		= eoi_dynirq,
 	.set_affinity	= set_affinity_irq,
 	.retrigger	= retrigger_irq,
 };
-- 
1.5.6.5

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

On Fri, 15 Oct 2010, Ian Campbell wrote:
> This prevents a guest from being able to trigger an interrupt storm in
> dom0 rendering it inoperable. In particular this effects event
> channels delivered to userspace by the /dev/xen/evtchn driver which
> uses disable_irq in its interupt handler to prevent further interrupts
> firing until the interrupt has been handled by the userspace
> application and it has requested an unmask. Without this patch the
> event channel is re-enabled immediately after the interrupt handler
> completes.
> 
> The issue was introduced by 0672fb44a111 "xen/events: change to using
> fasteoi".
> 
> In the specific instance I saw a domU would spin sending console event
> channel notifications to dom0 because its console ring was full (this
> behaviour has since been tempered by f3483182666a "xen/hvc: only
> notify if we actually sent something") but xenconsoled would be unable
> to run due to the storm and hence the ring would never be drained.
> 
> I'm not convinced this is the right way to go about this -- there does
> not seem to be much precedent and I would have expected some sort of
> generic handling but I cannot see any.
> 


Reading a little bit more about the fasteoi handler, it seems to me that
a better solution would be not to mask_evtchn and clear_evtchn in
__xen_evtchn_do_upcall, but just clear_evtchn in the eoi handler.
This would also be much more similar to the way the fasteoi handler is
used by the ioapic chip.
The appended patch has been only smoked tested.


diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 175e931..6de1528 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -1074,9 +1074,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 				int irq = evtchn_to_irq[port];
 				struct irq_desc *desc;
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1202,7 +1199,7 @@ static void ack_dynirq(unsigned int irq)
 	move_masked_irq(irq);
 
 	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+		clear_evtchn(evtchn);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1384,7 +1381,6 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Ian Campbell]

On Fri, 2010-10-15 at 17:12 +0100, Stefano Stabellini wrote:
> Reading a little bit more about the fasteoi handler, it seems to me that
> a better solution would be not to mask_evtchn and clear_evtchn in
> __xen_evtchn_do_upcall, but just clear_evtchn in the eoi handler.
> This would also be much more similar to the way the fasteoi handler is
> used by the ioapic chip.
> The appended patch has been only smoked tested.

Fails to boot dom0 for me.
        
        irq 20: nobody cared (try booting with the "irqpoll" option)
        Pid: 0, comm: swapper Not tainted 2.6.32.24-x86_32p-xen0-01025-g5238c00-dirty #205
        Call Trace:
         [<c1377034>] ? printk+0x18/0x1c
         [<c106f5c7>] __report_bad_irq+0x27/0x90
         [<c106f78e>] note_interrupt+0x15e/0x1a0
         [<c1169669>] ? _raw_spin_unlock+0x89/0xa0
         [<c106febc>] handle_fasteoi_irq+0xac/0xd0
         [<c11a6a5f>] __xen_evtchn_do_upcall+0x1cf/0x1f0
         [<c11a6ab8>] xen_evtchn_do_upcall+0x28/0x40
         [<c100b767>] xen_do_upcall+0x7/0xc
         [<c10013a7>] ? hypercall_page+0x3a7/0x1010
         [<c1007472>] ? xen_safe_halt+0x12/0x30
         [<c100397f>] xen_idle+0x5f/0x80
         [<c1009c49>] cpu_idle+0x49/0xa0
         [<c1007c40>] ? xen_save_fl_direct+0x0/0xd
         [<c135d913>] rest_init+0x53/0x60
         [<c1543975>] start_kernel+0x382/0x39f
         [<c15433be>] ? unknown_bootoption+0x0/0x1e4
         [<c154308f>] i386_start_kernel+0x7e/0xa8
         [<c1546471>] xen_start_kernel+0x561/0x5d5
        handlers:
        [<c1223500>] (ata_sff_interrupt+0x0/0xd0)
        [<c128b2b0>] (usb_hcd_irq+0x0/0xe0)
        Disabling IRQ #20
        
I think because you missed the pirq case.

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 175e931..1144489 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -444,8 +444,8 @@ static void pirq_eoi(unsigned int irq)
 
 	need_eoi = pirq_needs_eoi(irq);
 
-	if (!need_eoi || !pirq_eoi_does_unmask)
-		unmask_evtchn(info->evtchn);
+	if (need_eoi && pirq_eoi_does_unmask)
+		mask_evtchn(info->evtchn);
+
+	clear_evtchn(info->evtchn);
 
 	if (need_eoi) {
 		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
@@ -1052,6 +1056,7 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 
 	do {
 		unsigned long pending_words;
+		int irq;
 
 		vcpu_info->evtchn_upcall_pending = 0;
 
@@ -1062,6 +1067,24 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 		/* Clear master flag /before/ clearing selector flag. */
 		wmb();
 #endif
+
+		/*
+		 * Handle timer interrupts before all others, so that all
+		 * hardirq handlers see an up-to-date system time even if we
+		 * have just woken from a long idle period.
+		 */
+		irq = percpu_read(virq_to_irq[VIRQ_TIMER]);
+		if (irq != -1) {
+			int word_idx;
+			int bit_idx;
+			int port = evtchn_from_irq(irq);
+			word_idx = port / BITS_PER_LONG;
+			bit_idx = port % BITS_PER_LONG;
+			if (VALID_EVTCHN(port) &&
+			    (active_evtchns(cpu, s, word_idx) & (1UL<<bit_idx)))
+				(void)handle_irq(irq, regs);
+		}
+
 		pending_words = xchg(&vcpu_info->evtchn_pending_sel, 0);
 		while (pending_words != 0) {
 			unsigned long pending_bits;
@@ -1071,11 +1094,9 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 			while ((pending_bits = active_evtchns(cpu, s, word_idx)) != 0) {
 				int bit_idx = __ffs(pending_bits);
 				int port = (word_idx * BITS_PER_LONG) + bit_idx;
-				int irq = evtchn_to_irq[port];
 				struct irq_desc *desc;
 
-				mask_evtchn(port);
-				clear_evtchn(port);
+				irq = evtchn_to_irq[port];
 
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
@@ -1202,7 +1223,7 @@ static void ack_dynirq(unsigned int irq)
 	move_masked_irq(irq);
 
 	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+		clear_evtchn(evtchn);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1384,7 +1405,6 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
 
@@ -1412,7 +1432,7 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.enable		= pirq_eoi,
 	.unmask		= unmask_irq,
 
-	.disable	= mask_irq,
+	//.disable	= mask_irq,
 	.mask		= mask_irq,
 
 	.eoi		= ack_pirq,

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Ian Campbell]

On Fri, 2010-10-15 at 17:32 +0100, Ian Campbell wrote:
> On Fri, 2010-10-15 at 17:12 +0100, Stefano Stabellini wrote:
> > Reading a little bit more about the fasteoi handler, it seems to me that
> > a better solution would be not to mask_evtchn and clear_evtchn in
> > __xen_evtchn_do_upcall, but just clear_evtchn in the eoi handler.
> > This would also be much more similar to the way the fasteoi handler is
> > used by the ioapic chip.
> > The appended patch has been only smoked tested.
> 
> Fails to boot dom0 for me.
>         
>         irq 20: nobody cared (try booting with the "irqpoll" option)
>         Pid: 0, comm: swapper Not tainted 2.6.32.24-x86_32p-xen0-01025-g5238c00-dirty #205
>         Call Trace:
>          [<c1377034>] ? printk+0x18/0x1c
>          [<c106f5c7>] __report_bad_irq+0x27/0x90
>          [<c106f78e>] note_interrupt+0x15e/0x1a0
>          [<c1169669>] ? _raw_spin_unlock+0x89/0xa0
>          [<c106febc>] handle_fasteoi_irq+0xac/0xd0
>          [<c11a6a5f>] __xen_evtchn_do_upcall+0x1cf/0x1f0
>          [<c11a6ab8>] xen_evtchn_do_upcall+0x28/0x40
>          [<c100b767>] xen_do_upcall+0x7/0xc
>          [<c10013a7>] ? hypercall_page+0x3a7/0x1010
>          [<c1007472>] ? xen_safe_halt+0x12/0x30
>          [<c100397f>] xen_idle+0x5f/0x80
>          [<c1009c49>] cpu_idle+0x49/0xa0
>          [<c1007c40>] ? xen_save_fl_direct+0x0/0xd
>          [<c135d913>] rest_init+0x53/0x60
>          [<c1543975>] start_kernel+0x382/0x39f
>          [<c15433be>] ? unknown_bootoption+0x0/0x1e4
>          [<c154308f>] i386_start_kernel+0x7e/0xa8
>          [<c1546471>] xen_start_kernel+0x561/0x5d5
>         handlers:
>         [<c1223500>] (ata_sff_interrupt+0x0/0xd0)
>         [<c128b2b0>] (usb_hcd_irq+0x0/0xe0)
>         Disabling IRQ #20
>         
> I think because you missed the pirq case.

Mixed the patch up with some other patch. Lets try that again...

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 716c8ca..16a1e77 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -444,8 +444,10 @@ static void pirq_eoi(unsigned int irq)
 
 	need_eoi = pirq_needs_eoi(irq);
 
-	if (!need_eoi || !pirq_eoi_does_unmask)
-		unmask_evtchn(info->evtchn);
+	if (need_eoi && pirq_eoi_does_unmask)
+		mask_evtchn(info->evtchn);
+
+	clear_evtchn(info->evtchn);
 
 	if (need_eoi) {
 		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
@@ -1094,8 +1096,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 
 				irq = evtchn_to_irq[port];
 
-				mask_evtchn(port);
-				clear_evtchn(port);
 
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
@@ -1222,7 +1222,7 @@ static void ack_dynirq(unsigned int irq)
 	move_masked_irq(irq);
 
 	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+		clear_evtchn(evtchn);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1404,7 +1404,6 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
 
@@ -1432,7 +1431,6 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.enable		= pirq_eoi,
 	.unmask		= unmask_irq,
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 
 	.eoi		= ack_pirq,

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

On Fri, 15 Oct 2010, Ian Campbell wrote:
> On Fri, 2010-10-15 at 17:32 +0100, Ian Campbell wrote:
> > On Fri, 2010-10-15 at 17:12 +0100, Stefano Stabellini wrote:
> > > Reading a little bit more about the fasteoi handler, it seems to me that
> > > a better solution would be not to mask_evtchn and clear_evtchn in
> > > __xen_evtchn_do_upcall, but just clear_evtchn in the eoi handler.
> > > This would also be much more similar to the way the fasteoi handler is
> > > used by the ioapic chip.
> > > The appended patch has been only smoked tested.
> > 
> > Fails to boot dom0 for me.
> >         
> >         irq 20: nobody cared (try booting with the "irqpoll" option)
> >         Pid: 0, comm: swapper Not tainted 2.6.32.24-x86_32p-xen0-01025-g5238c00-dirty #205
> >         Call Trace:
> >          [<c1377034>] ? printk+0x18/0x1c
> >          [<c106f5c7>] __report_bad_irq+0x27/0x90
> >          [<c106f78e>] note_interrupt+0x15e/0x1a0
> >          [<c1169669>] ? _raw_spin_unlock+0x89/0xa0
> >          [<c106febc>] handle_fasteoi_irq+0xac/0xd0
> >          [<c11a6a5f>] __xen_evtchn_do_upcall+0x1cf/0x1f0
> >          [<c11a6ab8>] xen_evtchn_do_upcall+0x28/0x40
> >          [<c100b767>] xen_do_upcall+0x7/0xc
> >          [<c10013a7>] ? hypercall_page+0x3a7/0x1010
> >          [<c1007472>] ? xen_safe_halt+0x12/0x30
> >          [<c100397f>] xen_idle+0x5f/0x80
> >          [<c1009c49>] cpu_idle+0x49/0xa0
> >          [<c1007c40>] ? xen_save_fl_direct+0x0/0xd
> >          [<c135d913>] rest_init+0x53/0x60
> >          [<c1543975>] start_kernel+0x382/0x39f
> >          [<c15433be>] ? unknown_bootoption+0x0/0x1e4
> >          [<c154308f>] i386_start_kernel+0x7e/0xa8
> >          [<c1546471>] xen_start_kernel+0x561/0x5d5
> >         handlers:
> >         [<c1223500>] (ata_sff_interrupt+0x0/0xd0)
> >         [<c128b2b0>] (usb_hcd_irq+0x0/0xe0)
> >         Disabling IRQ #20
> >         
> > I think because you missed the pirq case.
> 
> Mixed the patch up with some other patch. Lets try that again...

Yes I did.  I think this version is better because in case the pirq is
already masked in pirq_eoi it makes sure that it stays that way, thus
preventing cases like the one you described before from happening.

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 175e931..9fb6e0f 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -441,16 +441,23 @@ static void pirq_eoi(unsigned int irq)
 	struct irq_info *info = info_for_irq(irq);
 	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
 	bool need_eoi;
+	struct shared_info *s = HYPERVISOR_shared_info;
+	int need_mask = 0;
 
 	need_eoi = pirq_needs_eoi(irq);
 
-	if (!need_eoi || !pirq_eoi_does_unmask)
-		unmask_evtchn(info->evtchn);
+	if (need_eoi && pirq_eoi_does_unmask)
+		need_mask = sync_test_and_set_bit(info->evtchn, s->evtchn_mask);
 
 	if (need_eoi) {
 		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
 		WARN_ON(rc);
+
+		if (need_mask)
+			mask_evtchn(info->evtchn);
 	}
+
+	clear_evtchn(info->evtchn);
 }
 
 static void pirq_query_unmask(int irq)
@@ -1074,9 +1081,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 				int irq = evtchn_to_irq[port];
 				struct irq_desc *desc;
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1202,7 +1206,7 @@ static void ack_dynirq(unsigned int irq)
 	move_masked_irq(irq);
 
 	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+		clear_evtchn(evtchn);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1384,7 +1388,6 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
 
@@ -1412,7 +1415,6 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.enable		= pirq_eoi,
 	.unmask		= unmask_irq,
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 
 	.eoi		= ack_pirq,

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Jeremy Fitzhardinge]

 On 10/15/2010 10:03 AM, Stefano Stabellini wrote:
> On Fri, 15 Oct 2010, Ian Campbell wrote:
>> On Fri, 2010-10-15 at 17:32 +0100, Ian Campbell wrote:
>>> On Fri, 2010-10-15 at 17:12 +0100, Stefano Stabellini wrote:
>>>> Reading a little bit more about the fasteoi handler, it seems to me that
>>>> a better solution would be not to mask_evtchn and clear_evtchn in
>>>> __xen_evtchn_do_upcall, but just clear_evtchn in the eoi handler.
>>>> This would also be much more similar to the way the fasteoi handler is
>>>> used by the ioapic chip.
>>>> The appended patch has been only smoked tested.
>>> Fails to boot dom0 for me.
>>>         
>>>         irq 20: nobody cared (try booting with the "irqpoll" option)
>>>         Pid: 0, comm: swapper Not tainted 2.6.32.24-x86_32p-xen0-01025-g5238c00-dirty #205
>>>         Call Trace:
>>>          [<c1377034>] ? printk+0x18/0x1c
>>>          [<c106f5c7>] __report_bad_irq+0x27/0x90
>>>          [<c106f78e>] note_interrupt+0x15e/0x1a0
>>>          [<c1169669>] ? _raw_spin_unlock+0x89/0xa0
>>>          [<c106febc>] handle_fasteoi_irq+0xac/0xd0
>>>          [<c11a6a5f>] __xen_evtchn_do_upcall+0x1cf/0x1f0
>>>          [<c11a6ab8>] xen_evtchn_do_upcall+0x28/0x40
>>>          [<c100b767>] xen_do_upcall+0x7/0xc
>>>          [<c10013a7>] ? hypercall_page+0x3a7/0x1010
>>>          [<c1007472>] ? xen_safe_halt+0x12/0x30
>>>          [<c100397f>] xen_idle+0x5f/0x80
>>>          [<c1009c49>] cpu_idle+0x49/0xa0
>>>          [<c1007c40>] ? xen_save_fl_direct+0x0/0xd
>>>          [<c135d913>] rest_init+0x53/0x60
>>>          [<c1543975>] start_kernel+0x382/0x39f
>>>          [<c15433be>] ? unknown_bootoption+0x0/0x1e4
>>>          [<c154308f>] i386_start_kernel+0x7e/0xa8
>>>          [<c1546471>] xen_start_kernel+0x561/0x5d5
>>>         handlers:
>>>         [<c1223500>] (ata_sff_interrupt+0x0/0xd0)
>>>         [<c128b2b0>] (usb_hcd_irq+0x0/0xe0)
>>>         Disabling IRQ #20
>>>         
>>> I think because you missed the pirq case.
>> Mixed the patch up with some other patch. Lets try that again...
> Yes I did.  I think this version is better because in case the pirq is
> already masked in pirq_eoi it makes sure that it stays that way, thus
> preventing cases like the one you described before from happening.

This looks plausible in general, but...

> diff --git a/drivers/xen/events.c b/drivers/xen/events.c
> index 175e931..9fb6e0f 100644
> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -441,16 +441,23 @@ static void pirq_eoi(unsigned int irq)
>  	struct irq_info *info = info_for_irq(irq);
>  	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
>  	bool need_eoi;
> +	struct shared_info *s = HYPERVISOR_shared_info;
> +	int need_mask = 0;
>  
>  	need_eoi = pirq_needs_eoi(irq);
>  
> -	if (!need_eoi || !pirq_eoi_does_unmask)
> -		unmask_evtchn(info->evtchn);
> +	if (need_eoi && pirq_eoi_does_unmask)
> +		need_mask = sync_test_and_set_bit(info->evtchn, s->evtchn_mask);
>  
>  	if (need_eoi) {
>  		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
>  		WARN_ON(rc);
> +
> +		if (need_mask)
> +			mask_evtchn(info->evtchn);
>  	}
> +
> +	clear_evtchn(info->evtchn);

This is all a mess.  This "auto unmask pirq eoi with shared memory"
thing seems pretty pointless if we need to go to all this effort to
remask again, and I didn't see any other point to it aside from that. 
So if this works, we may as well revert "xen/events: use
PHYSDEVOP_pirq_eoi_gmfn to get pirq need-EOI info".

>  }
>  
>  static void pirq_query_unmask(int irq)
> @@ -1074,9 +1081,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
>  				int irq = evtchn_to_irq[port];
>  				struct irq_desc *desc;
>  
> -				mask_evtchn(port);
> -				clear_evtchn(port);
> -
>  				if (irq != -1) {
>  					desc = irq_to_desc(irq);
>  					if (desc)
> @@ -1202,7 +1206,7 @@ static void ack_dynirq(unsigned int irq)
>  	move_masked_irq(irq);
>  
>  	if (VALID_EVTCHN(evtchn))
> -		unmask_evtchn(evtchn);
> +		clear_evtchn(evtchn);
>  }
>  
>  static int retrigger_irq(unsigned int irq)
> @@ -1384,7 +1388,6 @@ void xen_irq_resume(void)
>  static struct irq_chip xen_dynamic_chip __read_mostly = {
>  	.name		= "xen-dyn",
>  
> -	.disable	= mask_irq,
>  	.mask		= mask_irq,
>  	.unmask		= unmask_irq,
>  
> @@ -1412,7 +1415,6 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
>  	.enable		= pirq_eoi,
>  	.unmask		= unmask_irq,
>  
> -	.disable	= mask_irq,
>  	.mask		= mask_irq,
>  
>  	.eoi		= ack_pirq,
>

    J

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Jan Beulich]

>>> On 15.10.10 at 19:03, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> @@ -1074,9 +1081,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs 
> *regs)
>  				int irq = evtchn_to_irq[port];
>  				struct irq_desc *desc;
>  
> -				mask_evtchn(port);
> -				clear_evtchn(port);
> -
>  				if (irq != -1) {
>  					desc = irq_to_desc(irq);
>  					if (desc)
> @@ -1202,7 +1206,7 @@ static void ack_dynirq(unsigned int irq)
>  	move_masked_irq(irq);
>  
>  	if (VALID_EVTCHN(evtchn))
> -		unmask_evtchn(evtchn);
> +		clear_evtchn(evtchn);
>  }
>  
>  static int retrigger_irq(unsigned int irq)

These two hunks together don't look right, for two reasons: First,
ack_dynirq() is used as both .eoi and .ack, but those certainly
have different requirements (and this might have been a problem
already before, though I didn't spend much thought on what may
go wrong). Second, clearing the event channel in the .eoi handler
after it wasn't masked while being handled has the potential of
losing an event (if it got raised between the IRQ handler checking
relevant state and the execution of clear_evtchn()).

Jan

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4838 bytes --]

On Mon, 18 Oct 2010, Jan Beulich wrote:
> >>> On 15.10.10 at 19:03, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > @@ -1074,9 +1081,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs 
> > *regs)
> >  				int irq = evtchn_to_irq[port];
> >  				struct irq_desc *desc;
> >  
> > -				mask_evtchn(port);
> > -				clear_evtchn(port);
> > -
> >  				if (irq != -1) {
> >  					desc = irq_to_desc(irq);
> >  					if (desc)
> > @@ -1202,7 +1206,7 @@ static void ack_dynirq(unsigned int irq)
> >  	move_masked_irq(irq);
> >  
> >  	if (VALID_EVTCHN(evtchn))
> > -		unmask_evtchn(evtchn);
> > +		clear_evtchn(evtchn);
> >  }
> >  
> >  static int retrigger_irq(unsigned int irq)
> 
> These two hunks together don't look right, for two reasons: First,
> ack_dynirq() is used as both .eoi and .ack, but those certainly
> have different requirements (and this might have been a problem
> already before, though I didn't spend much thought on what may
> go wrong).

good point, see below for the fix

> Second, clearing the event channel in the .eoi handler
> after it wasn't masked while being handled has the potential of
> losing an event (if it got raised between the IRQ handler checking
> relevant state and the execution of clear_evtchn()).

This is only true for virqs because xen knows how to handle the case
when a pirq is already inflight.
Considering that this is the way the fasteoi handler is supposed to work
(no ack at the beginning, only at the end) I would keep fasteoi as pirq
handler and switch virqs to edge.
If you look at handle_edge_irq, the ack is always done at the beginning,
no eoi at the end but we don't need it for virqs. Mask and unmask are
done by the handler depending upon IRQ_INPROGRESS and IRQ_DISABLED.
It seems exactly the kind of thing we need as virq handler: we clear the
evtchn in ack and we mask and unmask the evtchn in mask and unmask.
The mapping of xen operations on the irq chip functions is very simple
and natural this way.

Following Jeremy's suggestion I reverted "xen/events: use
PHYSDEVOP_pirq_eoi_gmfn to get pirq need-EOI info"; I am attaching the
patch revert and the new version of this patch (also appended here for
easier read).

---

>From 47b52e85fb0b6edf458e079c604072370a3612f7 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Date: Mon, 18 Oct 2010 15:18:50 +0100
Subject: [PATCH 2/2] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

Do not clear and mask evtchns in __xen_evtchn_do_upcall, rely on the
fasteoi and edge handlers to do that for pirqs and virqs respectively.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/events.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 5db8e98..3c1b7ae 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -436,12 +436,15 @@ static bool identity_mapped_irq(unsigned irq)
 static void pirq_eoi(unsigned int irq)
 {
 	struct physdev_eoi eoi = { .irq = irq };
+	int evtchn = evtchn_from_irq(irq);
 
 	if (pirq_needs_eoi(irq)) {
 		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
 		WARN_ON(rc);
 	}
-	unmask_irq(irq);
+
+	if (VALID_EVTCHN(evtchn))
+		clear_evtchn(evtchn);
 }
 
 static void pirq_query_unmask(int irq)
@@ -500,6 +503,7 @@ static unsigned int startup_pirq(unsigned int irq)
 
  out:
 	pirq_eoi(irq);
+	unmask_irq(irq);
 
 	return 0;
 }
@@ -738,7 +742,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 		irq = find_unbound_irq();
 
 		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_fasteoi_irq, "event");
+					      handle_edge_irq, "event");
 
 		evtchn_to_irq[evtchn] = irq;
 		irq_info[irq] = mk_evtchn_info(evtchn);
@@ -1062,9 +1066,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 				int irq = evtchn_to_irq[port];
 				struct irq_desc *desc;
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1190,7 +1191,7 @@ static void ack_dynirq(unsigned int irq)
 	move_masked_irq(irq);
 
 	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+		clear_evtchn(evtchn);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1362,11 +1363,10 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
+	.ack		= ack_dynirq,
 
-	.eoi		= ack_dynirq,
 	.set_affinity	= set_affinity_irq,
 	.retrigger	= retrigger_irq,
 };
@@ -1387,10 +1387,7 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.startup	= startup_pirq,
 	.shutdown	= shutdown_pirq,
 
-	.enable		= pirq_eoi,
 	.unmask		= unmask_irq,
-
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 
 	.eoi		= ack_pirq,
-- 
1.5.6.5

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name="0001-Revert-xen-events-use-PHYSDEVOP_pirq_eoi_gmfn-to-g.patch", Size: 4743 bytes --]

From 6c79718e811fdecff9f7b1271df7395357054fc2 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Date: Mon, 18 Oct 2010 13:30:25 +0100
Subject: [PATCH 1/2] Revert "xen/events: use PHYSDEVOP_pirq_eoi_gmfn to get pirq need-EOI info"

This reverts commit 2390c371ecd32d9f06e22871636185382bf70ab7.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/events.c            |   45 ++++++--------------------------------
 include/xen/interface/physdev.h |   13 -----------
 2 files changed, 7 insertions(+), 51 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 175e931..5db8e98 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -103,12 +103,9 @@ struct irq_info
 		} pirq;
 	} u;
 };
+#define PIRQ_NEEDS_EOI	(1 << 0)
 #define PIRQ_SHAREABLE	(1 << 1)
 
-/* Bitmap indicating which PIRQs require Xen to be notified on unmask. */
-static bool pirq_eoi_does_unmask;
-static unsigned long *pirq_needs_eoi_bits;
-
 static struct irq_info *irq_info;
 
 static int *evtchn_to_irq;
@@ -251,7 +248,7 @@ static bool pirq_needs_eoi(unsigned irq)
 
 	BUG_ON(info->type != IRQT_PIRQ);
 
-	return test_bit(info->u.pirq.gsi, pirq_needs_eoi_bits);
+	return info->u.pirq.flags & PIRQ_NEEDS_EOI;
 }
 
 static inline unsigned long active_evtchns(unsigned int cpu,
@@ -438,19 +435,13 @@ static bool identity_mapped_irq(unsigned irq)
 
 static void pirq_eoi(unsigned int irq)
 {
-	struct irq_info *info = info_for_irq(irq);
-	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
-	bool need_eoi;
-
-	need_eoi = pirq_needs_eoi(irq);
+	struct physdev_eoi eoi = { .irq = irq };
 
-	if (!need_eoi || !pirq_eoi_does_unmask)
-		unmask_evtchn(info->evtchn);
-
-	if (need_eoi) {
+	if (pirq_needs_eoi(irq)) {
 		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
 		WARN_ON(rc);
 	}
+	unmask_irq(irq);
 }
 
 static void pirq_query_unmask(int irq)
@@ -458,18 +449,15 @@ static void pirq_query_unmask(int irq)
 	struct physdev_irq_status_query irq_status;
 	struct irq_info *info = info_for_irq(irq);
 
-	if (pirq_eoi_does_unmask)
-		return;
-
 	BUG_ON(info->type != IRQT_PIRQ);
 
 	irq_status.irq = info->u.pirq.gsi;
 	if (HYPERVISOR_physdev_op(PHYSDEVOP_irq_status_query, &irq_status))
 		irq_status.flags = 0;
 
-	clear_bit(info->u.pirq.gsi, pirq_needs_eoi_bits);
+	info->u.pirq.flags &= ~PIRQ_NEEDS_EOI;
 	if (irq_status.flags & XENIRQSTAT_needs_eoi)
-		set_bit(info->u.pirq.gsi, pirq_needs_eoi_bits);
+		info->u.pirq.flags |= PIRQ_NEEDS_EOI;
 }
 
 static bool probing_irq(int irq)
@@ -1369,16 +1357,6 @@ void xen_irq_resume(void)
 		restore_cpu_virqs(cpu);
 		restore_cpu_ipis(cpu);
 	}
-
-	if (pirq_eoi_does_unmask) {
-		struct physdev_pirq_eoi_gmfn eoi_gmfn;
-		
-		eoi_gmfn.gmfn = virt_to_mfn(pirq_needs_eoi_bits);
-		if (HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn) != 0) {
-			/* Could recover by reverting to old method...? */
-			BUG();
-		}
-	}
 }
 
 static struct irq_chip xen_dynamic_chip __read_mostly = {
@@ -1462,8 +1440,6 @@ void xen_callback_vector(void) {}
 void __init xen_init_IRQ(void)
 {
 	int i;
-	struct physdev_pirq_eoi_gmfn eoi_gmfn;
-	int nr_pirqs = NR_IRQS;
 
 	cpu_evtchn_mask_p = kcalloc(nr_cpu_ids, sizeof(struct cpu_evtchn_s),
 				    GFP_KERNEL);
@@ -1474,13 +1450,6 @@ void __init xen_init_IRQ(void)
 	for(i = 0; i < NR_EVENT_CHANNELS; i++)
 		evtchn_to_irq[i] = -1;
 
-	i = get_order(sizeof(unsigned long) * BITS_TO_LONGS(nr_pirqs));
-	pirq_needs_eoi_bits = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO, i);
-
- 	eoi_gmfn.gmfn = virt_to_mfn(pirq_needs_eoi_bits);
-	if (HYPERVISOR_physdev_op(PHYSDEVOP_pirq_eoi_gmfn, &eoi_gmfn) == 0)
-		pirq_eoi_does_unmask = true;
-
 	init_evtchn_cpu_bindings();
 
 	/* No event channels are 'live' right now. */
diff --git a/include/xen/interface/physdev.h b/include/xen/interface/physdev.h
index 0703ef6..66122aa 100644
--- a/include/xen/interface/physdev.h
+++ b/include/xen/interface/physdev.h
@@ -39,19 +39,6 @@ struct physdev_eoi {
 };
 
 /*
- * Register a shared page for the hypervisor to indicate whether the guest
- * must issue PHYSDEVOP_eoi. The semantics of PHYSDEVOP_eoi change slightly
- * once the guest used this function in that the associated event channel
- * will automatically get unmasked. The page registered is used as a bit
- * array indexed by Xen's PIRQ value.
- */
-#define PHYSDEVOP_pirq_eoi_gmfn         17
-struct physdev_pirq_eoi_gmfn {
-    /* IN */
-    unsigned long gmfn;
-};
-
-/*
  * Query the status of an IRQ line.
  * @arg == pointer to physdev_irq_status_query structure.
  */
-- 
1.5.6.5


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: Type: text/x-diff; name="0002-xen-do-not-clear-and-mask-evtchns-in-__xen_evtchn_d.patch", Size: 2730 bytes --]

From 47b52e85fb0b6edf458e079c604072370a3612f7 Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Date: Mon, 18 Oct 2010 15:18:50 +0100
Subject: [PATCH 2/2] xen: do not clear and mask evtchns in __xen_evtchn_do_upcall

Do not clear and mask evtchns in __xen_evtchn_do_upcall, rely on the
fasteoi and edge handlers to do that for pirqs and virqs respectively.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 drivers/xen/events.c |   19 ++++++++-----------
 1 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 5db8e98..3c1b7ae 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -436,12 +436,15 @@ static bool identity_mapped_irq(unsigned irq)
 static void pirq_eoi(unsigned int irq)
 {
 	struct physdev_eoi eoi = { .irq = irq };
+	int evtchn = evtchn_from_irq(irq);
 
 	if (pirq_needs_eoi(irq)) {
 		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
 		WARN_ON(rc);
 	}
-	unmask_irq(irq);
+
+	if (VALID_EVTCHN(evtchn))
+		clear_evtchn(evtchn);
 }
 
 static void pirq_query_unmask(int irq)
@@ -500,6 +503,7 @@ static unsigned int startup_pirq(unsigned int irq)
 
  out:
 	pirq_eoi(irq);
+	unmask_irq(irq);
 
 	return 0;
 }
@@ -738,7 +742,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 		irq = find_unbound_irq();
 
 		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_fasteoi_irq, "event");
+					      handle_edge_irq, "event");
 
 		evtchn_to_irq[evtchn] = irq;
 		irq_info[irq] = mk_evtchn_info(evtchn);
@@ -1062,9 +1066,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 				int irq = evtchn_to_irq[port];
 				struct irq_desc *desc;
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1190,7 +1191,7 @@ static void ack_dynirq(unsigned int irq)
 	move_masked_irq(irq);
 
 	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+		clear_evtchn(evtchn);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1362,11 +1363,10 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
+	.ack		= ack_dynirq,
 
-	.eoi		= ack_dynirq,
 	.set_affinity	= set_affinity_irq,
 	.retrigger	= retrigger_irq,
 };
@@ -1387,10 +1387,7 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.startup	= startup_pirq,
 	.shutdown	= shutdown_pirq,
 
-	.enable		= pirq_eoi,
 	.unmask		= unmask_irq,
-
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 
 	.eoi		= ack_pirq,
-- 
1.5.6.5


[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Jan Beulich]

>>> On 18.10.10 at 16:58, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 18 Oct 2010, Jan Beulich wrote:
>> Second, clearing the event channel in the .eoi handler
>> after it wasn't masked while being handled has the potential of
>> losing an event (if it got raised between the IRQ handler checking
>> relevant state and the execution of clear_evtchn()).
> 
> This is only true for virqs because xen knows how to handle the case
> when a pirq is already inflight.

I don't think Xen knowing how to deal with that matters here. It
won't send you a new notification if you would have got one
before clearing the previous instance (and you didn't get another
just because it was already/still pending). Or else I must be missing
something.

> Considering that this is the way the fasteoi handler is supposed to work
> (no ack at the beginning, only at the end) I would keep fasteoi as pirq
> handler and switch virqs to edge.
> If you look at handle_edge_irq, the ack is always done at the beginning,
> no eoi at the end but we don't need it for virqs. Mask and unmask are
> done by the handler depending upon IRQ_INPROGRESS and IRQ_DISABLED.
> It seems exactly the kind of thing we need as virq handler: we clear the
> evtchn in ack and we mask and unmask the evtchn in mask and unmask.
> The mapping of xen operations on the irq chip functions is very simple
> and natural this way.

While that all makes sense, one of the reasons to switch to fasteoi
is because they get along with just a single indirect call. Edge one,
the way you coded it, require three (which can be reduced to two
when using the .mask_ack vector).

Jan

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

On Mon, 18 Oct 2010, Jan Beulich wrote:
> >>> On 18.10.10 at 16:58, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 18 Oct 2010, Jan Beulich wrote:
> >> Second, clearing the event channel in the .eoi handler
> >> after it wasn't masked while being handled has the potential of
> >> losing an event (if it got raised between the IRQ handler checking
> >> relevant state and the execution of clear_evtchn()).
> > 
> > This is only true for virqs because xen knows how to handle the case
> > when a pirq is already inflight.
> 
> I don't think Xen knowing how to deal with that matters here. It
> won't send you a new notification if you would have got one
> before clearing the previous instance (and you didn't get another
> just because it was already/still pending). Or else I must be missing
> something.
> 

If I understand the situation correctly all depends on the ack_type
of the pirq: if it is ACKTYPE_NONE then xen acks the machine irq right
away so there is a possibility of receiving another evtchn while we are
handling the first one.
In this case my patch makes the situation a little worse: previously we
were able to receive a single notification while the evtchn is being
handled while now none.
The other type of pirqs are the ones that have ack_type != ACKTYPE_NONE,
in these cases xen waits for the guest to issue the eoi before acking
the machine irq so it shouldn't be possible to receive any pirqs while
we are handling the first one.

That said, I don't see any simple way to improve this patch so that we
are able to receive one notification while handling a pirq without
changing the semantic of the fasteoi handler or switching to the edge
handler for pirqs that don't need eoi.



> > Considering that this is the way the fasteoi handler is supposed to work
> > (no ack at the beginning, only at the end) I would keep fasteoi as pirq
> > handler and switch virqs to edge.
> > If you look at handle_edge_irq, the ack is always done at the beginning,
> > no eoi at the end but we don't need it for virqs. Mask and unmask are
> > done by the handler depending upon IRQ_INPROGRESS and IRQ_DISABLED.
> > It seems exactly the kind of thing we need as virq handler: we clear the
> > evtchn in ack and we mask and unmask the evtchn in mask and unmask.
> > The mapping of xen operations on the irq chip functions is very simple
> > and natural this way.
> 
> While that all makes sense, one of the reasons to switch to fasteoi
> is because they get along with just a single indirect call. Edge one,
> the way you coded it, require three (which can be reduced to two
> when using the .mask_ack vector).
> 
 
I think two indirect calls is not too bad, I just need to add a mask_ack
implementation.

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

On Mon, 18 Oct 2010, Stefano Stabellini wrote:
> On Mon, 18 Oct 2010, Jan Beulich wrote:
> > >>> On 18.10.10 at 16:58, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > > On Mon, 18 Oct 2010, Jan Beulich wrote:
> > >> Second, clearing the event channel in the .eoi handler
> > >> after it wasn't masked while being handled has the potential of
> > >> losing an event (if it got raised between the IRQ handler checking
> > >> relevant state and the execution of clear_evtchn()).
> > > 
> > > This is only true for virqs because xen knows how to handle the case
> > > when a pirq is already inflight.
> > 
> > I don't think Xen knowing how to deal with that matters here. It
> > won't send you a new notification if you would have got one
> > before clearing the previous instance (and you didn't get another
> > just because it was already/still pending). Or else I must be missing
> > something.
> > 
> 
> If I understand the situation correctly all depends on the ack_type
> of the pirq: if it is ACKTYPE_NONE then xen acks the machine irq right
> away so there is a possibility of receiving another evtchn while we are
> handling the first one.
> In this case my patch makes the situation a little worse: previously we
> were able to receive a single notification while the evtchn is being
> handled while now none.
> The other type of pirqs are the ones that have ack_type != ACKTYPE_NONE,
> in these cases xen waits for the guest to issue the eoi before acking
> the machine irq so it shouldn't be possible to receive any pirqs while
> we are handling the first one.
> 
> That said, I don't see any simple way to improve this patch so that we
> are able to receive one notification while handling a pirq without
> changing the semantic of the fasteoi handler or switching to the edge
> handler for pirqs that don't need eoi.
> 

Actually it has just occurred to me that we can safely have clear_evtchn in
do_upcall and at the same time not mask the evtchn because we are
protected against executing multiple upcalls at the same time anyway by
xed_nesting_count.

I'll try to respin another patch following with idea.

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Jan Beulich]

>>> On 18.10.10 at 22:04, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> Actually it has just occurred to me that we can safely have clear_evtchn in
> do_upcall and at the same time not mask the evtchn because we are
> protected against executing multiple upcalls at the same time anyway by
> xed_nesting_count.

I wouldn't suggest doing so - while this protects against recursion
for the actual handlers, you may still get do_upcall() invoked way too
many times, up to allowing a guest to continuously trigger an event
making do_upcall() get continuously invoked (as long as event
delivery isn't disabled altogether) - the very situation Ian's patch
you're suggesting to revert tried to address.

Jan

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Jan Beulich]

>>> On 18.10.10 at 20:14, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 18 Oct 2010, Jan Beulich wrote:
> The other type of pirqs are the ones that have ack_type != ACKTYPE_NONE,
> in these cases xen waits for the guest to issue the eoi before acking
> the machine irq so it shouldn't be possible to receive any pirqs while
> we are handling the first one.

But even in that case you do the clear after the EOI notification, i.e.
you still have a window where you may lose an event.

> That said, I don't see any simple way to improve this patch so that we
> are able to receive one notification while handling a pirq without
> changing the semantic of the fasteoi handler or switching to the edge
> handler for pirqs that don't need eoi.

What I'm trying to say is that I think the direct mask/clear in do_upcall()
should stay as it is.

Jan

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

On Tue, 19 Oct 2010, Jan Beulich wrote:
> >>> On 18.10.10 at 20:14, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 18 Oct 2010, Jan Beulich wrote:
> > The other type of pirqs are the ones that have ack_type != ACKTYPE_NONE,
> > in these cases xen waits for the guest to issue the eoi before acking
> > the machine irq so it shouldn't be possible to receive any pirqs while
> > we are handling the first one.
> 
> But even in that case you do the clear after the EOI notification, i.e.
> you still have a window where you may lose an event.
> 

New version of this patch:

- I am not reverting PHYSDEVOP_pirq_eoi_gmfn anymore because I found
that Xen unconditionally reports that all the pirqs need eoi if dom0
doesn't use PHYSDEVOP_pirq_eoi_gmfn.

- This causes complications in pirq_eoi because we need to be able to
handle the case in which the pirq has to stay masked. A new hypercall or
fixing the current interface would also be a good idea, because the
current behaviour breaks the interface, look at this comment on top of
the shared_info page:

     * Event channels are addressed by a "port index". Each channel is
	 * associated with two bits of information:
	 *  1. PENDING -- notifies the domain that there is a pending notification
	 *     to be processed. This bit is cleared by the guest.
	 *  2. MASK -- if this bit is clear then a 0->1 transition of PENDING
	 *     will cause an asynchronous upcall to be scheduled. This bit is only
->	 *     updated by the guest. It is read-only within Xen. If a channel
	 *     becomes pending while the channel is masked then the 'edge' is lost
	 *     (i.e., when the channel is unmasked, the guest must manually handle
	 *     pending notifications as no upcall will be scheduled by Xen).
	 *

- I am using handle_edge_irq as irq handler for virqs and pirqs that
don't need eoi (in Xen acktype == ACKTYPE_NONE, that means the machine
irq is actually edge).

- I am using 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.
In any case it would be a good idea to implement a basic "retry
send_guest_pirq" in Xen, in case the first time failed.



This patch 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;


However it is obviously not easy to understand and it has to work around
the limitations of PHYSDEVOP_pirq_eoi_gmfn.


---



diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 175e931..717b30e 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -436,21 +436,50 @@ static bool identity_mapped_irq(unsigned irq)
 	return irq < get_nr_hw_irqs();
 }
 
-static void pirq_eoi(unsigned int irq)
+static void eoi_pirq(unsigned int irq)
 {
 	struct irq_info *info = info_for_irq(irq);
 	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
-	bool need_eoi;
+	int evtchn = evtchn_from_irq(irq);
+	int rc = 0, need_mask = 0;
+	struct shared_info *s = HYPERVISOR_shared_info;
 
-	need_eoi = pirq_needs_eoi(irq);
+	if (!VALID_EVTCHN(evtchn))
+		return;
 
-	if (!need_eoi || !pirq_eoi_does_unmask)
-		unmask_evtchn(info->evtchn);
+	move_masked_irq(irq);
 
-	if (need_eoi) {
-		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
+	/* If the pirq doesn't need an eoi, just clear the evtchn and exit.
+	 * If the pirq is currently unmasked, or !pirq_eoi_does_unmask,
+	 * clear the evtchn and continue because the hypercall won't affect
+	 * us anyway.
+	 * If the pirq needs an eoi AND pirq_eoi_does_unmask AND the pirq is
+	 * currently masked than we have a problem because the eoi hypercall
+	 * will automatically unmasked the pirq. That means we cannot clear
+	 * the evtchn right away because we could receive an unwanted evtchn
+	 * notification after the hypercall and before masking the pirq
+	 * again. Therefore in this case we clear the evtchn after the
+	 * hypercall. */
+	if (pirq_needs_eoi(irq)) {
+		if (pirq_eoi_does_unmask)
+			need_mask = sync_test_bit(evtchn, &s->evtchn_mask[0]);
+		if (!need_mask)
+			clear_evtchn(evtchn);
+
+		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
 		WARN_ON(rc);
-	}
+		if (need_mask) {
+			mask_evtchn(evtchn);
+			clear_evtchn(evtchn);
+		}
+	} else
+		clear_evtchn(evtchn);
+}
+
+static void mask_ack_pirq(unsigned int irq)
+{
+	mask_irq(irq);
+	eoi_pirq(irq);
 }
 
 static void pirq_query_unmask(int irq)
@@ -481,6 +510,7 @@ static bool probing_irq(int irq)
 
 static unsigned int startup_pirq(unsigned int irq)
 {
+	struct irq_desc *desc;
 	struct evtchn_bind_pirq bind_pirq;
 	struct irq_info *info = info_for_irq(irq);
 	int evtchn = evtchn_from_irq(irq);
@@ -510,8 +540,25 @@ static unsigned int startup_pirq(unsigned int irq)
 	bind_evtchn_to_cpu(evtchn, 0);
 	info->evtchn = evtchn;
 
+	/* If the pirq does not need an eoi than we can use handle_edge_irq
+	 * and ack it right away, clearing the evtchn before calling
+	 * handle_IRQ_event. If the pirq does need an eoi than we can use
+	 * the fasteoi handler without loosing any interrupts because the
+	 * physical interrupt will still be waiting for an eoi as well.
+	 *
+	 * Only after EVTCHNOP_bind_pirq Xen reliably tells us what
+	 * kind of pirq this is, so we have to wait until now to make the
+	 * choice.
+	 * Afterwards Xen might temporarily set the needs_eoi flag for a
+	 * particular pirq, but that doesn't affect our choice here that
+	 * depends on the nature of the interrupt. */
+	desc = irq_to_desc(irq);
+	if (!pirq_needs_eoi(irq))
+		desc->handle_irq = handle_edge_irq;
+
  out:
-	pirq_eoi(irq);
+	eoi_pirq(irq);
+	unmask_irq(irq);
 
 	return 0;
 }
@@ -538,29 +585,6 @@ static void shutdown_pirq(unsigned int irq)
 	info->evtchn = 0;
 }
 
-static void ack_pirq(unsigned int irq)
-{
-	move_masked_irq(irq);
-	
-	pirq_eoi(irq);
-}
-
-static void end_pirq(unsigned int irq)
-{
-	int evtchn = evtchn_from_irq(irq);
-	struct irq_desc *desc = irq_to_desc(irq);
-
-	if (WARN_ON(!desc))
-		return;
-
-	if ((desc->status & (IRQ_DISABLED|IRQ_PENDING)) ==
-	    (IRQ_DISABLED|IRQ_PENDING)) {
-		shutdown_pirq(irq);
-	} else if (VALID_EVTCHN(evtchn)) {
-		pirq_eoi(irq);
-	}
-}
-
 static int find_irq_by_gsi(unsigned gsi)
 {
 	int irq;
@@ -750,7 +774,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 		irq = find_unbound_irq();
 
 		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_fasteoi_irq, "event");
+					      handle_edge_irq, "event");
 
 		evtchn_to_irq[evtchn] = irq;
 		irq_info[irq] = mk_evtchn_info(evtchn);
@@ -1074,9 +1098,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 				int irq = evtchn_to_irq[port];
 				struct irq_desc *desc;
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1202,7 +1223,13 @@ static void ack_dynirq(unsigned int irq)
 	move_masked_irq(irq);
 
 	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+		clear_evtchn(evtchn);
+}
+
+static void mask_ack_dynirq(unsigned int irq)
+{
+	mask_irq(irq);
+	ack_dynirq(irq);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1384,11 +1411,11 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
 
-	.eoi		= ack_dynirq,
+	.ack		= ack_dynirq,
+	.mask_ack	= mask_ack_dynirq,
 	.set_affinity	= set_affinity_irq,
 	.retrigger	= retrigger_irq,
 };
@@ -1409,14 +1436,12 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.startup	= startup_pirq,
 	.shutdown	= shutdown_pirq,
 
-	.enable		= pirq_eoi,
-	.unmask		= unmask_irq,
-
-	.disable	= mask_irq,
 	.mask		= mask_irq,
+	.unmask		= unmask_irq,
 
-	.eoi		= ack_pirq,
-	.end		= end_pirq,
+	.ack		= eoi_pirq,
+	.eoi		= eoi_pirq,
+	.mask_ack	= mask_ack_pirq,
 
 	.set_affinity	= set_affinity_irq,

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Jan Beulich]

>>> On 21.10.10 at 15:36, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> New version of this patch:

Looks consistent now, with one possible exception (I'm not sure here
really). I'm not going to ack this nevertheless as I continue to not be
convinced of the switch to handle_edge_irq(), the more that your
patch (as you write yourself) complicates the code.

> --- a/drivers/xen/events.c
> +++ b/drivers/xen/events.c
> @@ -436,21 +436,50 @@ static bool identity_mapped_irq(unsigned irq)
>  	return irq < get_nr_hw_irqs();
>  }
>  
> -static void pirq_eoi(unsigned int irq)
> +static void eoi_pirq(unsigned int irq)
>  {
>  	struct irq_info *info = info_for_irq(irq);
>  	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
> -	bool need_eoi;
> +	int evtchn = evtchn_from_irq(irq);
> +	int rc = 0, need_mask = 0;
> +	struct shared_info *s = HYPERVISOR_shared_info;
>  
> -	need_eoi = pirq_needs_eoi(irq);
> +	if (!VALID_EVTCHN(evtchn))
> +		return;
>  
> -	if (!need_eoi || !pirq_eoi_does_unmask)
> -		unmask_evtchn(info->evtchn);
> +	move_masked_irq(irq);

It's not clear whether calling this function is valid when the IRQ isn't
really masked.

In any case I'd suggest adding an IRQ_DISABLED check matching
move_native_irq()'s.

Jan

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Ian Campbell]

On Fri, 2010-10-22 at 08:41 +0100, Jan Beulich wrote: 
> >>> On 21.10.10 at 15:36, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/drivers/xen/events.c
> > +++ b/drivers/xen/events.c
> > @@ -436,21 +436,50 @@ static bool identity_mapped_irq(unsigned irq)
> >  	return irq < get_nr_hw_irqs();
> >  }
> >  
> > -static void pirq_eoi(unsigned int irq)
> > +static void eoi_pirq(unsigned int irq)
> >  {
> >  	struct irq_info *info = info_for_irq(irq);
> >  	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
> > -	bool need_eoi;
> > +	int evtchn = evtchn_from_irq(irq);
> > +	int rc = 0, need_mask = 0;
> > +	struct shared_info *s = HYPERVISOR_shared_info;
> >  
> > -	need_eoi = pirq_needs_eoi(irq);
> > +	if (!VALID_EVTCHN(evtchn))
> > +		return;
> >  
> > -	if (!need_eoi || !pirq_eoi_does_unmask)
> > -		unmask_evtchn(info->evtchn);
> > +	move_masked_irq(irq);
> 
> It's not clear whether calling this function is valid when the IRQ isn't
> really masked.
> 
> In any case I'd suggest adding an IRQ_DISABLED check matching
> move_native_irq()'s.

Why not just call move_native_irq instead of move_masked_irq as
appropriate?

Ian.

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Jan Beulich]

>>> On 22.10.10 at 10:07, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> On Fri, 2010-10-22 at 08:41 +0100, Jan Beulich wrote: 
>> >>> On 21.10.10 at 15:36, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
>> > --- a/drivers/xen/events.c
>> > +++ b/drivers/xen/events.c
>> > @@ -436,21 +436,50 @@ static bool identity_mapped_irq(unsigned irq)
>> >  	return irq < get_nr_hw_irqs();
>> >  }
>> >  
>> > -static void pirq_eoi(unsigned int irq)
>> > +static void eoi_pirq(unsigned int irq)
>> >  {
>> >  	struct irq_info *info = info_for_irq(irq);
>> >  	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
>> > -	bool need_eoi;
>> > +	int evtchn = evtchn_from_irq(irq);
>> > +	int rc = 0, need_mask = 0;
>> > +	struct shared_info *s = HYPERVISOR_shared_info;
>> >  
>> > -	need_eoi = pirq_needs_eoi(irq);
>> > +	if (!VALID_EVTCHN(evtchn))
>> > +		return;
>> >  
>> > -	if (!need_eoi || !pirq_eoi_does_unmask)
>> > -		unmask_evtchn(info->evtchn);
>> > +	move_masked_irq(irq);
>> 
>> It's not clear whether calling this function is valid when the IRQ isn't
>> really masked.
>> 
>> In any case I'd suggest adding an IRQ_DISABLED check matching
>> move_native_irq()'s.
> 
> Why not just call move_native_irq instead of move_masked_irq as
> appropriate?

That was what I implied with the first part of my response. But
I think the second part applies if a (the conditional) call to
move_masked_irq() would stay.

Jan

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Ian Campbell]

On Fri, 2010-10-22 at 09:29 +0100, Jan Beulich wrote:
> >>> On 22.10.10 at 10:07, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> > On Fri, 2010-10-22 at 08:41 +0100, Jan Beulich wrote: 
> >> >>> On 21.10.10 at 15:36, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> >> > --- a/drivers/xen/events.c
> >> > +++ b/drivers/xen/events.c
> >> > @@ -436,21 +436,50 @@ static bool identity_mapped_irq(unsigned irq)
> >> >  	return irq < get_nr_hw_irqs();
> >> >  }
> >> >  
> >> > -static void pirq_eoi(unsigned int irq)
> >> > +static void eoi_pirq(unsigned int irq)
> >> >  {
> >> >  	struct irq_info *info = info_for_irq(irq);
> >> >  	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
> >> > -	bool need_eoi;
> >> > +	int evtchn = evtchn_from_irq(irq);
> >> > +	int rc = 0, need_mask = 0;
> >> > +	struct shared_info *s = HYPERVISOR_shared_info;
> >> >  
> >> > -	need_eoi = pirq_needs_eoi(irq);
> >> > +	if (!VALID_EVTCHN(evtchn))
> >> > +		return;
> >> >  
> >> > -	if (!need_eoi || !pirq_eoi_does_unmask)
> >> > -		unmask_evtchn(info->evtchn);
> >> > +	move_masked_irq(irq);
> >> 
> >> It's not clear whether calling this function is valid when the IRQ isn't
> >> really masked.
> >> 
> >> In any case I'd suggest adding an IRQ_DISABLED check matching
> >> move_native_irq()'s.
> > 
> > Why not just call move_native_irq instead of move_masked_irq as
> > appropriate?
> 
> That was what I implied with the first part of my response.

Too subtle for me ;-)

> But I think the second part applies if a (the conditional) call to
> move_masked_irq() would stay.

Agreed, although I suspect we should know if the evtchn is masked or not
at this point depending on the already known properties of the
particular pirq (need_eoi, pirq_eoi_does_unmask etc).

Ian.

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

On Fri, 22 Oct 2010, Ian Campbell wrote:
> On Fri, 2010-10-22 at 09:29 +0100, Jan Beulich wrote:
> > >>> On 22.10.10 at 10:07, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
> > > On Fri, 2010-10-22 at 08:41 +0100, Jan Beulich wrote: 
> > >> >>> On 21.10.10 at 15:36, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > >> > --- a/drivers/xen/events.c
> > >> > +++ b/drivers/xen/events.c
> > >> > @@ -436,21 +436,50 @@ static bool identity_mapped_irq(unsigned irq)
> > >> >  	return irq < get_nr_hw_irqs();
> > >> >  }
> > >> >  
> > >> > -static void pirq_eoi(unsigned int irq)
> > >> > +static void eoi_pirq(unsigned int irq)
> > >> >  {
> > >> >  	struct irq_info *info = info_for_irq(irq);
> > >> >  	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
> > >> > -	bool need_eoi;
> > >> > +	int evtchn = evtchn_from_irq(irq);
> > >> > +	int rc = 0, need_mask = 0;
> > >> > +	struct shared_info *s = HYPERVISOR_shared_info;
> > >> >  
> > >> > -	need_eoi = pirq_needs_eoi(irq);
> > >> > +	if (!VALID_EVTCHN(evtchn))
> > >> > +		return;
> > >> >  
> > >> > -	if (!need_eoi || !pirq_eoi_does_unmask)
> > >> > -		unmask_evtchn(info->evtchn);
> > >> > +	move_masked_irq(irq);
> > >> 
> > >> It's not clear whether calling this function is valid when the IRQ isn't
> > >> really masked.
> > >> 
> > >> In any case I'd suggest adding an IRQ_DISABLED check matching
> > >> move_native_irq()'s.
> > > 
> > > Why not just call move_native_irq instead of move_masked_irq as
> > > appropriate?
> > 
> > That was what I implied with the first part of my response.
> 
> Too subtle for me ;-)
> 
> > But I think the second part applies if a (the conditional) call to
> > move_masked_irq() would stay.
> 
> Agreed, although I suspect we should know if the evtchn is masked or not
> at this point depending on the already known properties of the
> particular pirq (need_eoi, pirq_eoi_does_unmask etc).
> 
 
In none of these cases the evtchn would be masked for sure, so I'll have
to test if the evtchn is masked and call move_masked_irq or
move_native_irq accordingly.  I preferred to test the evtchn_mask
directly as opposed to the irq flags because the test is easier to
understand (we could arrive in eoi_pirq from both handle_edge_irq or
handle_fasteoi_irq, the latter doesn't set IRQ_MASKED when masks
an irq).

---

xen: use 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;

However it is obviously not easy to understand and it has to work around
the limitations of PHYSDEVOP_pirq_eoi_gmfn.

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

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 175e931..30224c8 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -436,21 +436,53 @@ static bool identity_mapped_irq(unsigned irq)
 	return irq < get_nr_hw_irqs();
 }
 
-static void pirq_eoi(unsigned int irq)
+static void eoi_pirq(unsigned int irq)
 {
 	struct irq_info *info = info_for_irq(irq);
 	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
-	bool need_eoi;
-
-	need_eoi = pirq_needs_eoi(irq);
+	int evtchn = evtchn_from_irq(irq);
+	int rc = 0, need_mask = 0;
+	struct shared_info *s = HYPERVISOR_shared_info;
 
-	if (!need_eoi || !pirq_eoi_does_unmask)
-		unmask_evtchn(info->evtchn);
+	if (!VALID_EVTCHN(evtchn))
+		return;
 
-	if (need_eoi) {
-		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
+	if (unlikely(sync_test_bit(evtchn, &s->evtchn_mask[0])))
+		move_masked_irq(irq);
+	else
+		move_native_irq(irq);
+
+	/* If the pirq doesn't need an eoi, just clear the evtchn and exit.
+	 * If the pirq is currently unmasked, or !pirq_eoi_does_unmask,
+	 * clear the evtchn and continue because the hypercall won't affect
+	 * us anyway.
+	 * If the pirq needs an eoi AND pirq_eoi_does_unmask AND the pirq is
+	 * currently masked than we have a problem because the eoi hypercall
+	 * will automatically unmasked the pirq. That means we cannot clear
+	 * the evtchn right away because we could receive an unwanted evtchn
+	 * notification after the hypercall and before masking the pirq
+	 * again. Therefore in this case we clear the evtchn after the
+	 * hypercall. */
+	if (pirq_needs_eoi(irq)) {
+		if (pirq_eoi_does_unmask)
+			need_mask = sync_test_bit(evtchn, &s->evtchn_mask[0]);
+		if (!need_mask)
+			clear_evtchn(evtchn);
+
+		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
 		WARN_ON(rc);
-	}
+		if (need_mask) {
+			mask_evtchn(evtchn);
+			clear_evtchn(evtchn);
+		}
+	} else
+		clear_evtchn(evtchn);
+}
+
+static void mask_ack_pirq(unsigned int irq)
+{
+	mask_irq(irq);
+	eoi_pirq(irq);
 }
 
 static void pirq_query_unmask(int irq)
@@ -481,6 +513,7 @@ static bool probing_irq(int irq)
 
 static unsigned int startup_pirq(unsigned int irq)
 {
+	struct irq_desc *desc;
 	struct evtchn_bind_pirq bind_pirq;
 	struct irq_info *info = info_for_irq(irq);
 	int evtchn = evtchn_from_irq(irq);
@@ -510,8 +543,25 @@ static unsigned int startup_pirq(unsigned int irq)
 	bind_evtchn_to_cpu(evtchn, 0);
 	info->evtchn = evtchn;
 
+	/* If the pirq does not need an eoi than we can use handle_edge_irq
+	 * and ack it right away, clearing the evtchn before calling
+	 * handle_IRQ_event. If the pirq does need an eoi than we can use
+	 * the fasteoi handler without loosing any interrupts because the
+	 * physical interrupt will still be waiting for an eoi as well.
+	 *
+	 * Only after EVTCHNOP_bind_pirq Xen reliably tells us what
+	 * kind of pirq this is, so we have to wait until now to make the
+	 * choice.
+	 * Afterwards Xen might temporarily set the needs_eoi flag for a
+	 * particular pirq, but that doesn't affect our choice here that
+	 * depends on the nature of the interrupt. */
+	desc = irq_to_desc(irq);
+	if (!pirq_needs_eoi(irq))
+		desc->handle_irq = handle_edge_irq;
+
  out:
-	pirq_eoi(irq);
+	eoi_pirq(irq);
+	unmask_irq(irq);
 
 	return 0;
 }
@@ -538,29 +588,6 @@ static void shutdown_pirq(unsigned int irq)
 	info->evtchn = 0;
 }
 
-static void ack_pirq(unsigned int irq)
-{
-	move_masked_irq(irq);
-	
-	pirq_eoi(irq);
-}
-
-static void end_pirq(unsigned int irq)
-{
-	int evtchn = evtchn_from_irq(irq);
-	struct irq_desc *desc = irq_to_desc(irq);
-
-	if (WARN_ON(!desc))
-		return;
-
-	if ((desc->status & (IRQ_DISABLED|IRQ_PENDING)) ==
-	    (IRQ_DISABLED|IRQ_PENDING)) {
-		shutdown_pirq(irq);
-	} else if (VALID_EVTCHN(evtchn)) {
-		pirq_eoi(irq);
-	}
-}
-
 static int find_irq_by_gsi(unsigned gsi)
 {
 	int irq;
@@ -750,7 +777,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 		irq = find_unbound_irq();
 
 		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_fasteoi_irq, "event");
+					      handle_edge_irq, "event");
 
 		evtchn_to_irq[evtchn] = irq;
 		irq_info[irq] = mk_evtchn_info(evtchn);
@@ -1074,9 +1101,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 				int irq = evtchn_to_irq[port];
 				struct irq_desc *desc;
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1198,11 +1222,23 @@ int resend_irq_on_evtchn(unsigned int irq)
 static void ack_dynirq(unsigned int irq)
 {
 	int evtchn = evtchn_from_irq(irq);
+	struct shared_info *s = HYPERVISOR_shared_info;
 
-	move_masked_irq(irq);
+	if (!VALID_EVTCHN(evtchn))
+		return;
 
-	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+	if (unlikely(sync_test_bit(evtchn, &s->evtchn_mask[0])))
+		move_masked_irq(irq);
+	else
+		move_native_irq(irq);
+
+	clear_evtchn(evtchn);
+}
+
+static void mask_ack_dynirq(unsigned int irq)
+{
+	mask_irq(irq);
+	ack_dynirq(irq);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1384,11 +1420,11 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
 
-	.eoi		= ack_dynirq,
+	.ack		= ack_dynirq,
+	.mask_ack	= mask_ack_dynirq,
 	.set_affinity	= set_affinity_irq,
 	.retrigger	= retrigger_irq,
 };
@@ -1409,14 +1445,12 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.startup	= startup_pirq,
 	.shutdown	= shutdown_pirq,
 
-	.enable		= pirq_eoi,
-	.unmask		= unmask_irq,
-
-	.disable	= mask_irq,
 	.mask		= mask_irq,
+	.unmask		= unmask_irq,
 
-	.eoi		= ack_pirq,
-	.end		= end_pirq,
+	.ack		= eoi_pirq,
+	.eoi		= eoi_pirq,
+	.mask_ack	= mask_ack_pirq,
 
 	.set_affinity	= set_affinity_irq,

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Jan Beulich]

>>> On 22.10.10 at 15:57, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> In none of these cases the evtchn would be masked for sure, so I'll have
> to test if the evtchn is masked and call move_masked_irq or
> move_native_irq accordingly.  I preferred to test the evtchn_mask
> directly as opposed to the irq flags because the test is easier to
> understand (we could arrive in eoi_pirq from both handle_edge_irq or
> handle_fasteoi_irq, the latter doesn't set IRQ_MASKED when masks
> an irq).

But you still didn't add and IRQ_DISABLED check around the call
to move_masked_irq() - do you have any particular reason not to?

Jan

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

On Fri, 22 Oct 2010, Jan Beulich wrote:
> >>> On 22.10.10 at 15:57, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > In none of these cases the evtchn would be masked for sure, so I'll have
> > to test if the evtchn is masked and call move_masked_irq or
> > move_native_irq accordingly.  I preferred to test the evtchn_mask
> > directly as opposed to the irq flags because the test is easier to
> > understand (we could arrive in eoi_pirq from both handle_edge_irq or
> > handle_fasteoi_irq, the latter doesn't set IRQ_MASKED when masks
> > an irq).
> 
> But you still didn't add and IRQ_DISABLED check around the call
> to move_masked_irq() - do you have any particular reason not to?
> 

Yes, you are right, a check for IRQ_DISABLED is also needed.

---

xen: use 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;

However it is obviously not easy to understand and it has to work around
the limitations of PHYSDEVOP_pirq_eoi_gmfn.

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

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 175e931..61bc35c 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -436,21 +436,56 @@ static bool identity_mapped_irq(unsigned irq)
 	return irq < get_nr_hw_irqs();
 }
 
-static void pirq_eoi(unsigned int irq)
+static void eoi_pirq(unsigned int irq)
 {
 	struct irq_info *info = info_for_irq(irq);
 	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
-	bool need_eoi;
+	int evtchn = evtchn_from_irq(irq);
+	int rc = 0, need_mask = 0;
+	struct shared_info *s = HYPERVISOR_shared_info;
+	struct irq_desc *desc = irq_to_desc(irq);
 
-	need_eoi = pirq_needs_eoi(irq);
+	if (!VALID_EVTCHN(evtchn))
+		return;
 
-	if (!need_eoi || !pirq_eoi_does_unmask)
-		unmask_evtchn(info->evtchn);
+	if (likely(!(desc->status & IRQ_DISABLED))) {
+		if (unlikely(sync_test_bit(evtchn, &s->evtchn_mask[0])))
+			move_masked_irq(irq);
+		else
+			move_native_irq(irq);
+	}
 
-	if (need_eoi) {
-		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
+	/* If the pirq doesn't need an eoi, just clear the evtchn and exit.
+	 * If the pirq is currently unmasked, or !pirq_eoi_does_unmask,
+	 * clear the evtchn and continue because the hypercall won't affect
+	 * us anyway.
+	 * If the pirq needs an eoi AND pirq_eoi_does_unmask AND the pirq is
+	 * currently masked than we have a problem because the eoi hypercall
+	 * will automatically unmasked the pirq. That means we cannot clear
+	 * the evtchn right away because we could receive an unwanted evtchn
+	 * notification after the hypercall and before masking the pirq
+	 * again. Therefore in this case we clear the evtchn after the
+	 * hypercall. */
+	if (pirq_needs_eoi(irq)) {
+		if (pirq_eoi_does_unmask)
+			need_mask = sync_test_bit(evtchn, &s->evtchn_mask[0]);
+		if (!need_mask)
+			clear_evtchn(evtchn);
+
+		rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
 		WARN_ON(rc);
-	}
+		if (need_mask) {
+			mask_evtchn(evtchn);
+			clear_evtchn(evtchn);
+		}
+	} else
+		clear_evtchn(evtchn);
+}
+
+static void mask_ack_pirq(unsigned int irq)
+{
+	mask_irq(irq);
+	eoi_pirq(irq);
 }
 
 static void pirq_query_unmask(int irq)
@@ -481,6 +516,7 @@ static bool probing_irq(int irq)
 
 static unsigned int startup_pirq(unsigned int irq)
 {
+	struct irq_desc *desc;
 	struct evtchn_bind_pirq bind_pirq;
 	struct irq_info *info = info_for_irq(irq);
 	int evtchn = evtchn_from_irq(irq);
@@ -510,8 +546,25 @@ static unsigned int startup_pirq(unsigned int irq)
 	bind_evtchn_to_cpu(evtchn, 0);
 	info->evtchn = evtchn;
 
+	/* If the pirq does not need an eoi than we can use handle_edge_irq
+	 * and ack it right away, clearing the evtchn before calling
+	 * handle_IRQ_event. If the pirq does need an eoi than we can use
+	 * the fasteoi handler without loosing any interrupts because the
+	 * physical interrupt will still be waiting for an eoi as well.
+	 *
+	 * Only after EVTCHNOP_bind_pirq Xen reliably tells us what
+	 * kind of pirq this is, so we have to wait until now to make the
+	 * choice.
+	 * Afterwards Xen might temporarily set the needs_eoi flag for a
+	 * particular pirq, but that doesn't affect our choice here that
+	 * depends on the nature of the interrupt. */
+	desc = irq_to_desc(irq);
+	if (!pirq_needs_eoi(irq))
+		desc->handle_irq = handle_edge_irq;
+
  out:
-	pirq_eoi(irq);
+	eoi_pirq(irq);
+	unmask_irq(irq);
 
 	return 0;
 }
@@ -538,29 +591,6 @@ static void shutdown_pirq(unsigned int irq)
 	info->evtchn = 0;
 }
 
-static void ack_pirq(unsigned int irq)
-{
-	move_masked_irq(irq);
-	
-	pirq_eoi(irq);
-}
-
-static void end_pirq(unsigned int irq)
-{
-	int evtchn = evtchn_from_irq(irq);
-	struct irq_desc *desc = irq_to_desc(irq);
-
-	if (WARN_ON(!desc))
-		return;
-
-	if ((desc->status & (IRQ_DISABLED|IRQ_PENDING)) ==
-	    (IRQ_DISABLED|IRQ_PENDING)) {
-		shutdown_pirq(irq);
-	} else if (VALID_EVTCHN(evtchn)) {
-		pirq_eoi(irq);
-	}
-}
-
 static int find_irq_by_gsi(unsigned gsi)
 {
 	int irq;
@@ -750,7 +780,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 		irq = find_unbound_irq();
 
 		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_fasteoi_irq, "event");
+					      handle_edge_irq, "event");
 
 		evtchn_to_irq[evtchn] = irq;
 		irq_info[irq] = mk_evtchn_info(evtchn);
@@ -1074,9 +1104,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 				int irq = evtchn_to_irq[port];
 				struct irq_desc *desc;
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1198,11 +1225,26 @@ int resend_irq_on_evtchn(unsigned int irq)
 static void ack_dynirq(unsigned int irq)
 {
 	int evtchn = evtchn_from_irq(irq);
+	struct shared_info *s = HYPERVISOR_shared_info;
+	struct irq_desc *desc = irq_to_desc(irq);
 
-	move_masked_irq(irq);
+	if (!VALID_EVTCHN(evtchn))
+		return;
 
-	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+	if (likely(!(desc->status & IRQ_DISABLED))) {
+		if (unlikely(sync_test_bit(evtchn, &s->evtchn_mask[0])))
+			move_masked_irq(irq);
+		else
+			move_native_irq(irq);
+	}
+
+	clear_evtchn(evtchn);
+}
+
+static void mask_ack_dynirq(unsigned int irq)
+{
+	mask_irq(irq);
+	ack_dynirq(irq);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1384,11 +1426,11 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
 
-	.eoi		= ack_dynirq,
+	.ack		= ack_dynirq,
+	.mask_ack	= mask_ack_dynirq,
 	.set_affinity	= set_affinity_irq,
 	.retrigger	= retrigger_irq,
 };
@@ -1409,14 +1451,12 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.startup	= startup_pirq,
 	.shutdown	= shutdown_pirq,
 
-	.enable		= pirq_eoi,
-	.unmask		= unmask_irq,
-
-	.disable	= mask_irq,
 	.mask		= mask_irq,
+	.unmask		= unmask_irq,
 
-	.eoi		= ack_pirq,
-	.end		= end_pirq,
+	.ack		= eoi_pirq,
+	.eoi		= eoi_pirq,
+	.mask_ack	= mask_ack_pirq,
 
 	.set_affinity	= set_affinity_irq,

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

I realized that this patch was too complicated so I did some refactoring
and I came up with a simpler patch, easier to understand.

The catch is that I gave up trying to prevent any unwanted notifications
between the eoi hypercall and the following mask_irq, but the current
code has the same problem anyway (actually it is worse because it
doesn't even realize that the irq is supposed to stay masked).
Thus it doesn't introduce any regressions, solves the bug IanC reported
(the virq storm) and improves things for pirqs as well.

---

xen: use 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;

However it is obviously not easy to understand and it has to work around
the limitations of PHYSDEVOP_pirq_eoi_gmfn.

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

diff --git a/drivers/xen/events.c b/drivers/xen/events.c
index 175e931..a33106c 100644
--- a/drivers/xen/events.c
+++ b/drivers/xen/events.c
@@ -436,21 +436,71 @@ static bool identity_mapped_irq(unsigned irq)
 	return irq < get_nr_hw_irqs();
 }
 
-static void pirq_eoi(unsigned int irq)
+static void xen_move_irq(unsigned int irq)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+	int evtchn = evtchn_from_irq(irq);
+	struct shared_info *s = HYPERVISOR_shared_info;
+
+	if (likely(!(desc->status & IRQ_DISABLED))) {
+		if (unlikely(sync_test_bit(evtchn, &s->evtchn_mask[0])))
+			move_masked_irq(irq);
+		else
+			move_native_irq(irq);
+	}
+}
+
+static void eoi_pirq(unsigned int irq)
 {
 	struct irq_info *info = info_for_irq(irq);
 	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
-	bool need_eoi;
+	int evtchn = evtchn_from_irq(irq);
+	int rc = 0;
+	struct irq_desc *desc = irq_to_desc(irq);
 
-	need_eoi = pirq_needs_eoi(irq);
+	if (!VALID_EVTCHN(evtchn))
+		return;
 
-	if (!need_eoi || !pirq_eoi_does_unmask)
-		unmask_evtchn(info->evtchn);
+	xen_move_irq(irq);
 
-	if (need_eoi) {
-		int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
-		WARN_ON(rc);
-	}
+	clear_evtchn(evtchn);
+
+	if (!pirq_needs_eoi(irq))
+		return;
+
+	rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
+	WARN_ON(rc);
+
+	/* when PHYSDEVOP_eoi won't automatically unmask the evtchn we won't
+	 * need this anymore */
+	if (pirq_eoi_does_unmask && desc->status & IRQ_DISABLED)
+		mask_irq(irq);
+}
+
+static void mask_ack_pirq(unsigned int irq)
+{
+	struct irq_info *info = info_for_irq(irq);
+	struct physdev_eoi eoi = { .irq = info->u.pirq.gsi };
+	int evtchn = evtchn_from_irq(irq);
+	int rc;
+
+	if (!VALID_EVTCHN(evtchn))
+		return;
+
+	mask_irq(irq);
+	move_masked_irq(irq);
+	clear_evtchn(evtchn);
+
+	if (!pirq_needs_eoi(irq))
+		return;
+
+	rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi);
+	WARN_ON(rc);
+
+	/* when PHYSDEVOP_eoi won't automatically unmask the evtchn we won't
+	 * need this anymore */
+	if (pirq_eoi_does_unmask)
+		mask_irq(irq);
 }
 
 static void pirq_query_unmask(int irq)
@@ -481,6 +531,7 @@ static bool probing_irq(int irq)
 
 static unsigned int startup_pirq(unsigned int irq)
 {
+	struct irq_desc *desc;
 	struct evtchn_bind_pirq bind_pirq;
 	struct irq_info *info = info_for_irq(irq);
 	int evtchn = evtchn_from_irq(irq);
@@ -510,8 +561,25 @@ static unsigned int startup_pirq(unsigned int irq)
 	bind_evtchn_to_cpu(evtchn, 0);
 	info->evtchn = evtchn;
 
+	/* If the pirq does not need an eoi than we can use handle_edge_irq
+	 * and ack it right away, clearing the evtchn before calling
+	 * handle_IRQ_event. If the pirq does need an eoi than we can use
+	 * the fasteoi handler without loosing any interrupts because the
+	 * physical interrupt will still be waiting for an eoi as well.
+	 *
+	 * Only after EVTCHNOP_bind_pirq Xen reliably tells us what
+	 * kind of pirq this is, so we have to wait until now to make the
+	 * choice.
+	 * Afterwards Xen might temporarily set the needs_eoi flag for a
+	 * particular pirq, but that doesn't affect our choice here that
+	 * depends on the nature of the interrupt. */
+	desc = irq_to_desc(irq);
+	if (!pirq_needs_eoi(irq))
+		desc->handle_irq = handle_edge_irq;
+
  out:
-	pirq_eoi(irq);
+	eoi_pirq(irq);
+	unmask_irq(irq);
 
 	return 0;
 }
@@ -538,29 +606,6 @@ static void shutdown_pirq(unsigned int irq)
 	info->evtchn = 0;
 }
 
-static void ack_pirq(unsigned int irq)
-{
-	move_masked_irq(irq);
-	
-	pirq_eoi(irq);
-}
-
-static void end_pirq(unsigned int irq)
-{
-	int evtchn = evtchn_from_irq(irq);
-	struct irq_desc *desc = irq_to_desc(irq);
-
-	if (WARN_ON(!desc))
-		return;
-
-	if ((desc->status & (IRQ_DISABLED|IRQ_PENDING)) ==
-	    (IRQ_DISABLED|IRQ_PENDING)) {
-		shutdown_pirq(irq);
-	} else if (VALID_EVTCHN(evtchn)) {
-		pirq_eoi(irq);
-	}
-}
-
 static int find_irq_by_gsi(unsigned gsi)
 {
 	int irq;
@@ -750,7 +795,7 @@ int bind_evtchn_to_irq(unsigned int evtchn)
 		irq = find_unbound_irq();
 
 		set_irq_chip_and_handler_name(irq, &xen_dynamic_chip,
-					      handle_fasteoi_irq, "event");
+					      handle_edge_irq, "event");
 
 		evtchn_to_irq[evtchn] = irq;
 		irq_info[irq] = mk_evtchn_info(evtchn);
@@ -1074,9 +1119,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs)
 				int irq = evtchn_to_irq[port];
 				struct irq_desc *desc;
 
-				mask_evtchn(port);
-				clear_evtchn(port);
-
 				if (irq != -1) {
 					desc = irq_to_desc(irq);
 					if (desc)
@@ -1199,10 +1241,17 @@ static void ack_dynirq(unsigned int irq)
 {
 	int evtchn = evtchn_from_irq(irq);
 
-	move_masked_irq(irq);
+	if (!VALID_EVTCHN(evtchn))
+		return;
 
-	if (VALID_EVTCHN(evtchn))
-		unmask_evtchn(evtchn);
+	xen_move_irq(irq);
+	clear_evtchn(evtchn);
+}
+
+static void mask_ack_dynirq(unsigned int irq)
+{
+	mask_irq(irq);
+	ack_dynirq(irq);
 }
 
 static int retrigger_irq(unsigned int irq)
@@ -1384,11 +1433,11 @@ void xen_irq_resume(void)
 static struct irq_chip xen_dynamic_chip __read_mostly = {
 	.name		= "xen-dyn",
 
-	.disable	= mask_irq,
 	.mask		= mask_irq,
 	.unmask		= unmask_irq,
 
-	.eoi		= ack_dynirq,
+	.ack		= ack_dynirq,
+	.mask_ack	= mask_ack_dynirq,
 	.set_affinity	= set_affinity_irq,
 	.retrigger	= retrigger_irq,
 };
@@ -1409,14 +1458,12 @@ static struct irq_chip xen_pirq_chip __read_mostly = {
 	.startup	= startup_pirq,
 	.shutdown	= shutdown_pirq,
 
-	.enable		= pirq_eoi,
-	.unmask		= unmask_irq,
-
-	.disable	= mask_irq,
 	.mask		= mask_irq,
+	.unmask		= unmask_irq,
 
-	.eoi		= ack_pirq,
-	.end		= end_pirq,
+	.ack		= eoi_pirq,
+	.eoi		= eoi_pirq,
+	.mask_ack	= mask_ack_pirq,
 
 	.set_affinity	= set_affinity_irq,

Re: [PATCH] xen: do not unmask disabled IRQ on eoi.

[Stefano Stabellini]

On Fri, 22 Oct 2010, Jan Beulich wrote:
> >>> On 21.10.10 at 15:36, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > New version of this patch:
> 
> Looks consistent now, with one possible exception (I'm not sure here
> really). I'm not going to ack this nevertheless as I continue to not be
> convinced of the switch to handle_edge_irq(), the more that your
> patch (as you write yourself) complicates the code.

In general I am always for the simplest solution, however in this case
it is not only a matter of simplicity but also a matter of correctness
and integration with the rest of linux irq handling infrastructure.

Obviously if something in this patch is inconsistent we could get less
correct and more complicated code so thanks for helping out making sure
it is consistent! If you find any more flaws please let me know :)